Keyboard scrolling does not bubble from OOPIFs
Reported by
russell....@gmail.com,
Jun 14 2018
|
||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3396.87 Safari/537.36 Example URL: https://thewirecutter.com/reviews/best-patio-furniture/ Steps to reproduce the problem: 1. Load a page with an iframe (e.g., https://thewirecutter.com/reviews/best-patio-furniture/) 2. Click inside the iframe to give it focus (for the site above, click in the Disqus comments at the bottom) 3. Press the up or down arrow keys to scroll the page. What is the expected behavior? The page scrolls like normal. What went wrong? The page doesn't scroll. Nothing happens. Does it occur on multiple sites: Yes Is it a problem with a plugin? No Did this work before? N/A Does this work in other browsers? Yes Chrome version: 67.0.3396.87 Channel: stable OS Version: OS X 10.13.4 Flash Version: I've confirmed this still occurs even in a Guest session with no extensions. Firefox and Safari do not have this problem.
,
Jun 14 2018
I think this is a site isolation problem. I can repro this with --site-per-process on a Linux ToT build, and things work fine if I turn site isolation off. Might be an issue with scroll bubbling; the iframe on the page has a "scrolling=no" attribute, and it seems keyboard arrows should be scrolling the main frame even when the iframe has focus. Ken/James/Ria - any ideas? russell.davis@: can you temporarily set chrome://flags/#site-isolation-trial-opt-out to "Opt out" and report if that solves the problem for you?
,
Jun 14 2018
Confirmed: that setting makes the problem go away. (Thanks for the quick response!)
,
Jun 15 2018
I can confirm this as well on Windows with Site Isolation, both on Stable (67.0.3396.87) and Canary (69.0.3460.0). Kevin, would you be able to take a look at how the keyboard case is handled (or help find an owner in the input team)? I think James mentioned he was going to be busy with something on Friday. Also CC'ing Sahel from previous scroll bubbling fixes. Thanks! In the meantime, a mediocre workaround is clicking outside the iframe to put focus back in the main frame before scrolling with the keyboard, but we'll see if we can get a fix soon.
,
Jun 15 2018
ScrollManager::BubblingScroll only bubbles logical scrolls when the parent is a local frame. Since this doesn't use GestureScroll events, our scroll bubbling logic in RenderWidgetHostViewChildFrame::GestureEventAck isn't involved. https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/input/scroll_manager.cc?rcl=ea6664aaa01b1242333b380744fc605c85cb6f2a&l=279
,
Jun 15 2018
We've been meaning to convert keyboard scrolling to use gesture events for a while now. That's tracked in issue 125223.
,
Jun 15 2018
Ah, thanks! Sahel, it looks like you've marked issue 125223 as started. Would you be able to help get this working? (Feel free to reassign as needed, if mcnee@, bokan@, or anyone else is able to help.)
,
Jun 15 2018
,
Jun 15 2018
From dtapuska@'s comment on issue 125223, it sounds like that requires a large refactor. Are there any shorter term options for handling the FIXME in ScrollManager::BubblingScroll, like sending an IPC to the browser process if the parent frame is remote?
,
Jun 15 2018
re comment #7: I have done some work in Q1, but we decided to prioritize other things in Q2, I'll talk to bokan@ to see if we plan to resume working on this in Q3.
,
Jun 15 2018
As a short term fix, perhaps we could just resend unconsumed keyboard events which can cause scroll to the parent view. It's not very elegant, but I just tried it and it seems to work.
,
Jun 15 2018
We could reprioritize but I think we'll need a short term fix regardless. As dtapuska@ mentioned, similar efforts have taken a long time. I expect keyboard scrolls to be a bit easier but still non trivial.
,
Jun 15 2018
Thanks all! Kevin, it sounds like your short term fix may be worth pursuing. Would you be able to get it ready for review?
,
Jun 20 2018
Quick update: Kevin has a CL in progress for this at https://chromium-review.googlesource.com/1106279.
,
Jun 29 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e21d23b8427576ae3877a2d1616fec42dc861ed1 commit e21d23b8427576ae3877a2d1616fec42dc861ed1 Author: Kevin McNee <mcnee@chromium.org> Date: Fri Jun 29 15:25:04 2018 Bubble logical scrolling from OOPIFs. Currently, ScrollManager::BubblingScroll only bubbles logical scrolls from an iframe when the parent is a local frame. Now, when bubbling reaches a local root, we send an IPC to the parent's process to continue bubbling from the frame owner element. Bug: 852932 Change-Id: I312aed8eea353957b7d77cea3caad60a2c3493d6 Reviewed-on: https://chromium-review.googlesource.com/1106279 Commit-Queue: Kevin McNee <mcnee@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: David Bokan <bokan@chromium.org> Cr-Commit-Position: refs/heads/master@{#571484} [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/content/browser/DEPS [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/content/browser/frame_host/render_frame_proxy_host.cc [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/content/browser/frame_host/render_frame_proxy_host.h [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/content/browser/site_per_process_browsertest.cc [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/content/common/DEPS [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/content/common/frame_messages.h [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/content/renderer/render_frame_impl.cc [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/content/renderer/render_frame_impl.h [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/content/renderer/render_frame_proxy.cc [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/content/renderer/render_frame_proxy.h [add] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/content/test/data/frame_tree/page_with_iframe_in_scrollable_div.html [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/third_party/blink/public/BUILD.gn [add] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/third_party/blink/public/platform/web_scroll_types.h [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/third_party/blink/public/web/web_local_frame_client.h [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/third_party/blink/public/web/web_remote_frame.h [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/third_party/blink/renderer/core/exported/local_frame_client_impl.cc [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/third_party/blink/renderer/core/exported/local_frame_client_impl.h [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/third_party/blink/renderer/core/exported/web_remote_frame_impl.cc [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/third_party/blink/renderer/core/exported/web_remote_frame_impl.h [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/third_party/blink/renderer/core/frame/frame.h [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/third_party/blink/renderer/core/frame/local_frame.cc [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/third_party/blink/renderer/core/frame/local_frame.h [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/third_party/blink/renderer/core/frame/local_frame_client.h [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/third_party/blink/renderer/core/frame/remote_frame.cc [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/third_party/blink/renderer/core/frame/remote_frame.h [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/third_party/blink/renderer/core/input/scroll_manager.cc [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/third_party/blink/renderer/core/loader/empty_clients.h [modify] https://crrev.com/e21d23b8427576ae3877a2d1616fec42dc861ed1/third_party/blink/renderer/platform/scroll/scroll_types.h
,
Jun 29 2018
Thanks for the fix! Once r571484 is in a Canary, let's verify it and determine if it can be merged to M68.
,
Jul 3
The NextAction date has arrived: 2018-07-03
,
Jul 3
Requesting merge of https://chromium.googlesource.com/chromium/src.git/+/e21d23b8427576ae3877a2d1616fec42dc861ed1 to M68. I have a local checkout of M68 and can confirm that the patch (and the test) works there as well.
,
Jul 3
This bug requires manual review: DEPS changes referenced in bugdroid comments. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 3
This seems like a fairly large CL. Can you please comment on how safe this is overall? What are the implications if we wait until M69 for this? We only have 2 more beta cycles remaining for M68.
,
Jul 3
If an out-of-process iframe has focus, keyboard scrolling can only scroll within the frame. For a simple repro to illustrate: 1) visit http://csreis.github.io/tests/cross-site-iframe.html 2) zoom or resize the window as needed so that the page is scrollable 3) press the "Go cross-site (simple page)" button 4) click inside the subframe 5) use the up/down arrow keys to try to scroll Since the subframe isn't scrollable, we would expect the scroll to apply to the main frame, but instead nothing happens. As noted in c#4, there is a workaround where the user can focus the desired frame before attempting to scroll with the keyboard. Most of the CL is just plumbing values out of blink, to the browser, and back into blink in the parent frame's renderer. It doesn't really touch existing code paths except for scroll_manager.cc where we call the new code path instead of giving up.
,
Jul 6
Thanks for more feedback. Approving merge for M68. Branch:3440
,
Jul 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0 commit a9e064ea43ccf0f81517a63da34cc5295cb4f6b0 Author: Kevin McNee <mcnee@chromium.org> Date: Fri Jul 06 19:33:27 2018 Merge to M68: Bubble logical scrolling from OOPIFs. TBR=dcheng@chromium.org,creis@chromium.org,bokan@chromium.org Bubble logical scrolling from OOPIFs. Currently, ScrollManager::BubblingScroll only bubbles logical scrolls from an iframe when the parent is a local frame. Now, when bubbling reaches a local root, we send an IPC to the parent's process to continue bubbling from the frame owner element. (cherry picked from commit e21d23b8427576ae3877a2d1616fec42dc861ed1) Bug: 852932 Change-Id: I312aed8eea353957b7d77cea3caad60a2c3493d6 Reviewed-on: https://chromium-review.googlesource.com/1106279 Commit-Queue: Kevin McNee <mcnee@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: David Bokan <bokan@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#571484} Reviewed-on: https://chromium-review.googlesource.com/1128211 Reviewed-by: Kevin McNee <mcnee@chromium.org> Cr-Commit-Position: refs/branch-heads/3440@{#604} Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733} [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/content/browser/DEPS [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/content/browser/frame_host/render_frame_proxy_host.cc [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/content/browser/frame_host/render_frame_proxy_host.h [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/content/browser/site_per_process_browsertest.cc [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/content/common/DEPS [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/content/common/frame_messages.h [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/content/renderer/render_frame_impl.cc [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/content/renderer/render_frame_impl.h [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/content/renderer/render_frame_proxy.cc [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/content/renderer/render_frame_proxy.h [add] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/content/test/data/frame_tree/page_with_iframe_in_scrollable_div.html [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/third_party/blink/public/BUILD.gn [add] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/third_party/blink/public/platform/web_scroll_types.h [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/third_party/blink/public/web/web_frame_client.h [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/third_party/blink/public/web/web_remote_frame.h [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/third_party/blink/renderer/core/exported/local_frame_client_impl.cc [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/third_party/blink/renderer/core/exported/local_frame_client_impl.h [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/third_party/blink/renderer/core/exported/web_remote_frame_impl.cc [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/third_party/blink/renderer/core/exported/web_remote_frame_impl.h [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/third_party/blink/renderer/core/frame/frame.h [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/third_party/blink/renderer/core/frame/local_frame.cc [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/third_party/blink/renderer/core/frame/local_frame.h [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/third_party/blink/renderer/core/frame/local_frame_client.h [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/third_party/blink/renderer/core/frame/remote_frame.cc [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/third_party/blink/renderer/core/frame/remote_frame.h [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/third_party/blink/renderer/core/input/scroll_manager.cc [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/third_party/blink/renderer/core/loader/empty_clients.h [modify] https://crrev.com/a9e064ea43ccf0f81517a63da34cc5295cb4f6b0/third_party/blink/renderer/platform/scroll/scroll_types.h |
||||||||||||||
►
Sign in to add a comment |
||||||||||||||
Comment 1 by rtoy@chromium.org
, Jun 14 2018Status: Untriaged (was: Unconfirmed)