Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 2883033003: Propagate inert state to OOPIFs when a modal dialog is active (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 1 week ago by kenrb
Modified:
2 months ago
CC:
blink-reviews, blink-reviews-api_chromium.org, blink-reviews-dom_chromium.org, blink-reviews-frames_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dcheng, dglazkov+blink, eae+blinkwatch, jam, kinuko+watch, mlamouri+watch-content_chromium.org, mlamouri+watch-blink_chromium.org, nasko+codewatch_chromium.org, rwlbuis, sof, site-isolation-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Propagate inert state to OOPIFs when a modal dialog is active When showModal is called on a <dialog> element, the rest of the Document becomes inert which prevents it from receiving events or taking focus. When the Document contains an out-of-process iframe, however, its contents are not aware of the modal dialog in a remote ancestor. This CL caches the current inert state on each LocalFrame, which is changed when a modal dialog because active or inactive. The bit is plumbed to remote frame children so that OOPIFs will respect inertness. Also it prevents having to search up the entire frame tree when an element checks whether it is inert. BUG=719788 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2883033003 Cr-Commit-Position: refs/heads/master@{#481761} Committed: https://chromium.googlesource.com/chromium/src/+/04323789a189306e523b5982a9cd49dcebdc88f5

Patch Set 1 #

Total comments: 1

Patch Set 2 : Rebase #

Patch Set 3 : Call InertSubtreesChanged on dialog removal #

Patch Set 4 : Another rebase #

Patch Set 5 : Trying to fix patch application problem #

Total comments: 7

Patch Set 6 : Review comments addressed, test modified #

Patch Set 7 : Rebase only #

Total comments: 4

Patch Set 8 : Fix test crash #

Total comments: 1

Patch Set 9 : Rebase only #

Patch Set 10 : Added test. #

Patch Set 11 : Account for Inert attribute #

Total comments: 2

Patch Set 12 : Changed SetTimeout in test, + bug fix #

Patch Set 13 : Bug fix #

Patch Set 14 : Set Frame's inert bit on style calculation #

Total comments: 5

Patch Set 15 : Small cleanup #

Patch Set 16 : Rebase only #

Patch Set 17 : Restore earlier approach + UpdateDistribution() #

Total comments: 38

Patch Set 18 : alexmos comments addressed #

Total comments: 7

Patch Set 19 : Rebase only #

Patch Set 20 : more alexmos comments addressed #

Patch Set 21 : nit addressed #

Unified diffs Side-by-side diffs Delta from patch set Stats (+295 lines, -5 lines) Patch
M content/browser/frame_host/cross_process_frame_connector.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +4 lines, -0 lines 0 comments Download
M content/browser/frame_host/cross_process_frame_connector.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +7 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_widget_host_view_child_frame.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_widget_host_view_child_frame.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +18 lines, -1 line 0 comments Download
M content/browser/site_per_process_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +138 lines, -0 lines 0 comments Download
M content/common/frame_messages.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +3 lines, -0 lines 0 comments Download
M content/common/view_messages.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +3 lines, -0 lines 0 comments Download
M content/renderer/render_frame_proxy.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/render_frame_proxy.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +4 lines, -0 lines 0 comments Download
M content/renderer/render_widget.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/render_widget.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +8 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/html/dialog/inert-focus-in-frames.html View 1 2 3 4 5 1 chunk +5 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/html/dialog/inert-focus-in-frames-expected.txt View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/html/dialog/resources/inert-focus-in-frames-frame1.html View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Node.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/frame/Frame.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +12 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/frame/Frame.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +13 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/LocalFrame.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/LocalFrame.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +20 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/RemoteFrame.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/RemoteFrame.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/RemoteFrameClient.h View 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/RemoteFrameClientImpl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/RemoteFrameClientImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLDialogElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +9 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/html/HTMLElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/loader/EmptyClients.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebFrameWidgetImpl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +6 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebFrameWidget.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebRemoteFrameClient.h View 1 chunk +3 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 112 (77 generated)
kenrb
https://codereview.chromium.org/2883033003/diff/1/third_party/WebKit/Source/core/html/HTMLDialogElement.cpp File third_party/WebKit/Source/core/html/HTMLDialogElement.cpp (right): https://codereview.chromium.org/2883033003/diff/1/third_party/WebKit/Source/core/html/HTMLDialogElement.cpp#newcode192 third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:192: // FIXME: We should call inertSubtreesChanged() here. Does this ...
3 months, 1 week ago (2017-05-15 20:51:54 UTC) #4
kenrb
aboxhall, dmazzoni: Can one or both of you PTAL? This is an attempt to implement ...
3 months, 1 week ago (2017-05-16 17:02:33 UTC) #18
aboxhall
https://codereview.chromium.org/2883033003/diff/60002/third_party/WebKit/Source/core/html/HTMLDialogElement.cpp File third_party/WebKit/Source/core/html/HTMLDialogElement.cpp (right): https://codereview.chromium.org/2883033003/diff/60002/third_party/WebKit/Source/core/html/HTMLDialogElement.cpp#newcode89 third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:89: document.GetFrame()->SetIsInert(false); When does SetIsInert(true) get called? Also, you might ...
3 months ago (2017-05-18 04:47:27 UTC) #21
kenrb
Thanks for the feedback. https://codereview.chromium.org/2883033003/diff/60002/third_party/WebKit/Source/core/html/HTMLDialogElement.cpp File third_party/WebKit/Source/core/html/HTMLDialogElement.cpp (right): https://codereview.chromium.org/2883033003/diff/60002/third_party/WebKit/Source/core/html/HTMLDialogElement.cpp#newcode89 third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:89: document.GetFrame()->SetIsInert(false); On 2017/05/18 04:47:27, aboxhall ...
3 months ago (2017-05-18 12:21:49 UTC) #22
dmazzoni
Design question: what happens if a child frame calls showModal, does that affect the parent ...
3 months ago (2017-05-18 15:47:06 UTC) #23
kenrb
Thanks. I have added some comments in the most recent patchset, and have modified the ...
3 months ago (2017-05-19 17:46:28 UTC) #28
dmazzoni
lgtm, I understand how it all works now, just some suggestions! https://codereview.chromium.org/2883033003/diff/110001/third_party/WebKit/Source/core/frame/LocalFrame.cpp File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): ...
3 months ago (2017-05-19 18:58:19 UTC) #31
dmazzoni
On Fri, May 19, 2017 at 10:46 AM <kenrb@chromium.org> wrote: > Thanks. I have added ...
3 months ago (2017-05-22 21:53:00 UTC) #34
dmazzoni
On Fri, May 19, 2017 at 10:46 AM <kenrb@chromium.org> wrote: > Thanks. I have added ...
3 months ago (2017-05-22 21:53:00 UTC) #35
kenrb
PTAL again? I have added a browsertest, and also had to make some changes to ...
2 months, 3 weeks ago (2017-05-31 18:32:01 UTC) #46
kenrb
Friendly ping. dmazzoni or aboxhall, do either of you have any more thoughts before I ...
2 months, 3 weeks ago (2017-06-02 21:20:52 UTC) #47
dmazzoni
Taking another look now
2 months, 2 weeks ago (2017-06-05 17:23:33 UTC) #48
dmazzoni
https://codereview.chromium.org/2883033003/diff/190001/content/browser/site_per_process_browsertest.cc File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2883033003/diff/190001/content/browser/site_per_process_browsertest.cc#newcode10272 content/browser/site_per_process_browsertest.cc:10272: // The setTimeout ensures that the inert bit can ...
2 months, 2 weeks ago (2017-06-05 17:32:43 UTC) #49
kenrb
https://codereview.chromium.org/2883033003/diff/190001/content/browser/site_per_process_browsertest.cc File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2883033003/diff/190001/content/browser/site_per_process_browsertest.cc#newcode10272 content/browser/site_per_process_browsertest.cc:10272: // The setTimeout ensures that the inert bit can ...
2 months, 2 weeks ago (2017-06-05 19:25:50 UTC) #54
kenrb
It looks like there are bugs with the changes that I made to support the ...
2 months, 2 weeks ago (2017-06-05 21:36:38 UTC) #57
kenrb
I have PS#14 up now, which passes all tests, but I am not certain this ...
2 months, 2 weeks ago (2017-06-07 01:41:12 UTC) #66
dmazzoni
On 2017/06/07 01:41:12, kenrb wrote: > I have PS#14 up now, which passes all tests, ...
2 months, 2 weeks ago (2017-06-07 16:39:04 UTC) #67
kenrb
tkent@: PTAL WebKit/?
2 months, 2 weeks ago (2017-06-08 20:20:51 UTC) #69
tkent
https://codereview.chromium.org/2883033003/diff/250001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2883033003/diff/250001/third_party/WebKit/Source/core/dom/Document.cpp#newcode2271 third_party/WebKit/Source/core/dom/Document.cpp:2271: GetFrame()->SetIsInert(owner->IsInert()); This was added to fix "!node.NeedsDistributionRecalc()" dcheck failures, ...
2 months, 2 weeks ago (2017-06-08 23:53:49 UTC) #71
kenrb
https://codereview.chromium.org/2883033003/diff/250001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2883033003/diff/250001/third_party/WebKit/Source/core/dom/Document.cpp#newcode2271 third_party/WebKit/Source/core/dom/Document.cpp:2271: GetFrame()->SetIsInert(owner->IsInert()); On 2017/06/08 23:53:48, tkent wrote: > This was ...
2 months, 2 weeks ago (2017-06-09 15:15:48 UTC) #72
kenrb
https://codereview.chromium.org/2883033003/diff/250001/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2883033003/diff/250001/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp#newcode776 third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:776: DCHECK(local_root_->Parent() && local_root_->Parent()->IsWebRemoteFrame()); On 2017/06/08 23:53:49, tkent wrote: > ...
2 months, 2 weeks ago (2017-06-09 19:26:31 UTC) #75
kenrb
hayato@: PTAL? See comment by tkent@ above. Thanks!
2 months, 1 week ago (2017-06-12 20:23:58 UTC) #82
hayato
https://codereview.chromium.org/2883033003/diff/250001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2883033003/diff/250001/third_party/WebKit/Source/core/dom/Document.cpp#newcode2271 third_party/WebKit/Source/core/dom/Document.cpp:2271: GetFrame()->SetIsInert(owner->IsInert()); On 2017/06/09 at 15:15:48, kenrb wrote: > On ...
2 months, 1 week ago (2017-06-13 09:48:00 UTC) #83
kenrb
Okay, I have gone back to the first way of doing this, and added calls ...
2 months, 1 week ago (2017-06-14 01:40:37 UTC) #88
hayato
On 2017/06/14 at 01:40:37, kenrb wrote: > Okay, I have gone back to the first ...
2 months, 1 week ago (2017-06-14 01:55:14 UTC) #89
kenrb
On 2017/06/14 01:55:14, hayato wrote: > On 2017/06/14 at 01:40:37, kenrb wrote: > > Okay, ...
2 months, 1 week ago (2017-06-14 14:21:29 UTC) #90
tkent
> tkent@: Can you review the Blink side of this? lgtm
2 months, 1 week ago (2017-06-15 07:17:49 UTC) #91
kenrb
alexmos@: PTAL content/?
2 months, 1 week ago (2017-06-15 14:16:13 UTC) #93
alexmos
Thanks Ken! A few comments/questions below. https://codereview.chromium.org/2883033003/diff/310001/content/browser/frame_host/render_widget_host_view_child_frame.cc File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2883033003/diff/310001/content/browser/frame_host/render_widget_host_view_child_frame.cc#newcode388 content/browser/frame_host/render_widget_host_view_child_frame.cc:388: host_->Send(new ViewMsg_SetIsInert(host_->GetRoutingID(), inert)); ...
2 months, 1 week ago (2017-06-16 02:17:58 UTC) #94
kenrb
Thanks for the review. Responses below, and I've tried to address your suggestions in PS#17. ...
2 months ago (2017-06-19 19:26:23 UTC) #99
alexmos
Thanks Ken for all the explanations! One more question about a potentially problematic case below. ...
2 months ago (2017-06-20 18:46:10 UTC) #100
kenrb
Thanks for all the good comments. New PS uploaded. https://codereview.chromium.org/2883033003/diff/310001/content/browser/frame_host/render_widget_host_view_child_frame.cc File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2883033003/diff/310001/content/browser/frame_host/render_widget_host_view_child_frame.cc#newcode388 content/browser/frame_host/render_widget_host_view_child_frame.cc:388: ...
2 months ago (2017-06-22 15:33:35 UTC) #101
alexmos
Thanks, LGTM! https://codereview.chromium.org/2883033003/diff/310001/content/browser/frame_host/render_widget_host_view_child_frame.cc File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2883033003/diff/310001/content/browser/frame_host/render_widget_host_view_child_frame.cc#newcode388 content/browser/frame_host/render_widget_host_view_child_frame.cc:388: host_->Send(new ViewMsg_SetIsInert(host_->GetRoutingID(), inert)); On 2017/06/22 15:33:35, kenrb ...
2 months ago (2017-06-22 17:09:28 UTC) #104
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2883033003/390001
2 months ago (2017-06-22 20:31:56 UTC) #109
commit-bot: I haz the power
2 months ago (2017-06-23 01:23:55 UTC) #112
Message was sent while issue was closed.
Committed patchset #21 (id:390001) as
https://chromium.googlesource.com/chromium/src/+/04323789a189306e523b5982a9cd...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b