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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 1 day ago by kenrb
Modified:
13 hours, 41 minutes ago
Reviewers:
dmazzoni, aboxhall
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, sof, creis+watch_chromium.org, kinuko+watch, mlamouri+watch-blink_chromium.org, eae+blinkwatch, nasko+codewatch_chromium.org, jam, dcheng, blink-reviews-api_chromium.org, blink-reviews-dom_chromium.org, dglazkov+blink, darin-cc_chromium.org, blink-reviews, blink-reviews-frames_chromium.org, blink-reviews-html_chromium.org, rwlbuis, 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

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: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+108 lines, -4 lines) Patch
M content/browser/frame_host/cross_process_frame_connector.h View 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/frame_host/cross_process_frame_connector.cc View 2 chunks +6 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 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 1 chunk +5 lines, -0 lines 0 comments Download
M content/common/frame_messages.h View 1 chunk +3 lines, -0 lines 0 comments Download
M content/common/view_messages.h View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
M content/renderer/render_frame_proxy.h View 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/render_frame_proxy.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M content/renderer/render_widget.h View 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/render_widget.cc View 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 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/frame/Frame.h View 1 2 3 4 5 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/LocalFrame.h View 1 2 3 4 5 6 2 chunks +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/LocalFrame.cpp View 1 2 3 4 5 6 2 chunks +13 lines, -0 lines 1 comment Download
M third_party/WebKit/Source/core/frame/RemoteFrame.h View 2 chunks +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/RemoteFrame.cpp View 2 chunks +8 lines, -1 line 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/html/HTMLDialogElement.cpp View 1 2 3 4 5 3 chunks +7 lines, -1 line 1 comment Download
M third_party/WebKit/Source/core/loader/EmptyClients.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/web/RemoteFrameClientImpl.h View 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/web/RemoteFrameClientImpl.cpp View 1 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebFrameWidgetImpl.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp View 1 2 3 4 5 6 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebFrameWidget.h View 1 2 3 4 5 6 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
Trybot results:  linux_chromium_rel_ng   mac_chromium_rel_ng   win_clang   win_chromium_x64_rel_ng   win_chromium_compile_dbg_ng   mac_chromium_rel_ng   win_chromium_rel_ng   ios-simulator-xcode-clang   mac_chromium_compile_dbg_ng   ios-device-xcode-clang   ios-simulator   ios-device   linux_chromium_tsan_rel_ng   linux_site_isolation   linux_chromium_compile_dbg_ng   linux_chromium_chromeos_rel_ng   linux_chromium_rel_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_asan_rel_ng   chromium_presubmit   chromeos_daisy_chromium_compile_only_ng   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_linux   linux_android_rel_ng   android_cronet   android_n5x_swarming_rel   cast_shell_android   android_arm64_dbg_recipe   android_compile_dbg   android_clang_dbg_recipe   mac_chromium_rel_ng   linux_chromium_chromeos_rel_ng   win_chromium_x64_rel_ng   win_clang   win_chromium_rel_ng   win_chromium_compile_dbg_ng   mac_chromium_compile_dbg_ng   mac_chromium_rel_ng   ios-simulator   ios-simulator-xcode-clang   ios-device   ios-device-xcode-clang   linux_chromium_rel_ng   linux_chromium_tsan_rel_ng   linux_site_isolation   linux_chromium_chromeos_rel_ng   linux_chromium_compile_dbg_ng   linux_chromium_asan_rel_ng   linux_chromium_chromeos_ozone_rel_ng   chromium_presubmit   chromeos_daisy_chromium_compile_only_ng   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_linux   cast_shell_android   linux_android_rel_ng   android_cronet   android_n5x_swarming_rel   android_compile_dbg   android_clang_dbg_recipe   android_arm64_dbg_recipe  Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 39 (30 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 ...
1 week, 1 day 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 ...
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 ...
6 days, 1 hour 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 ...
5 days, 17 hours 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 ...
5 days, 14 hours 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 ...
4 days, 12 hours 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): ...
4 days, 11 hours 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 ...
1 day, 8 hours ago (2017-05-22 21:53:00 UTC) #34
dmazzoni
1 day, 8 hours ago (2017-05-22 21:53:00 UTC) #35
On Fri, May 19, 2017 at 10:46 AM <kenrb@chromium.org> wrote:

> Thanks. I have added some comments in the most recent patchset, and have
> modified the test as suggested by aboxhall.
>
> Unfortunately there isn't an OOPIF-test yet. I can have a look at the flaky
> OOPIF tests for accessibility also. I know how tricky these can be to
> write.
>

I re-enabled the dialog in iframe accessibility tests. It should be trivial
to add oopif variants of the same tests, just copy what we do
in iframe-cross-process.html and the rest should be automatic.

A follow-up change is fine.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
Sign in to reply to this message.

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