'vertical-scroll' policy shouldn't scroll iframes which are not allowed to block vertical scroll. |
|||
Issue descriptionThe 'vertical-scroll' policy allows vertical scroll even if the frame attempts to prevent it. However, there are a couple other ways that even if events can't block vertical scroll an iframe could still block scroll (whether intentionally or not). 1. Long content in a frame essentially blocks continued vertical scrolling: http://jsbin.com/zacetib/edit?html,css,js,output 2. Content which tries to keep scroll position at the top: https://jsbin.com/tofano/edit?html,output And allowing vertical scroll could cause undesirable scrolling on frames which expect not to scroll: https://jsbin.com/ficoyut/edit?html,output For all of these reasons, I think scroll targeting should ignore frames which are not allowed to prevent scroll in the axis the scroll was started in. I.e. we shouldn't actually scroll the frame's content and target the scroller in the root frame.
,
Oct 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/10ee9149b870f4f4a31070ce39e256ec606ed0e7 commit 10ee9149b870f4f4a31070ce39e256ec606ed0e7 Author: Ehsan Karamad <ekaramad@chromium.org> Date: Tue Oct 09 19:23:42 2018 No vertical scroll if kVerticalScroll disabled 'vertical-scroll' policy is a policy-controlled feature which allows developers to control scrollability of their web pages and disallow embedded content from blocking vertical scrolling. In line with the implementation of the feature, this CL ensures that any content inside a document which has the feature disabled will not be able to consume vertical scroll deltas. Note that the current implementation of the policy only deals with methods of blocking input from turning into scroll gesture. This CL complements that controlling the consumption of such gestures. Bug: 853485 Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel Change-Id: I7db68e5897998ba29f2ad75cef44660563d6b95b Reviewed-on: https://chromium-review.googlesource.com/c/1252530 Reviewed-by: David Bokan <bokan@chromium.org> Reviewed-by: Ehsan Karamad <ekaramad@chromium.org> Commit-Queue: Ehsan Karamad <ekaramad@chromium.org> Cr-Commit-Position: refs/heads/master@{#598035} [add] https://crrev.com/10ee9149b870f4f4a31070ce39e256ec606ed0e7/third_party/WebKit/LayoutTests/external/wpt/feature-policy/experimental-features/resources/vertical-scroll-scrollable-content.html [add] https://crrev.com/10ee9149b870f4f4a31070ce39e256ec606ed0e7/third_party/WebKit/LayoutTests/external/wpt/feature-policy/experimental-features/vertical-scroll-disabled-frame-no-scroll-manual.tentative.html [add] https://crrev.com/10ee9149b870f4f4a31070ce39e256ec606ed0e7/third_party/WebKit/LayoutTests/external/wpt_automation/feature-policy/experimental-features/vertical-scroll-disabled-frame-no-scroll-manual.tentative-automation.js [modify] https://crrev.com/10ee9149b870f4f4a31070ce39e256ec606ed0e7/third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc
,
Oct 11
Closing the bug following comment #2.
,
Oct 15
BTW, regarding #1 I'm becoming increasingly convinced (eg. talking with AMP team) that we should treat the issue of long scrollers separately from this feature. Maybe wait and see which problems actually emerge there? Or at most, add some UKM metrics to try to find URLs where this seems to happen (a lot of sub-scrolling occurs in the context of a vertical-scroll policy), and investigate specific cases?
,
Oct 15
So, the current version as of comment #2 does not do that anymore. I am thinking, perhaps this is something we can address in the long run through parameterized features (list values), e.g.: <iframe allow="vertical-scroll foo.com(DisableUserScroll) baz.com(DisableGestureCancel) baz.com(Enabled)" ></iframe> To me it sounds like if we want to guarantee a page is scrollable and some embedded content does not block it we should respect #1.
,
Oct 15
There are a lot of edge cases with allowing scroll but not allowing it to be prevented. Ideally, the embedded page can prevent scrolling within its scrollers and if it does you would still scroll the top frame. However, you can't do that without delaying the scroll until you know the event disposition - which as I understand is one of motivations of this feature policy. Otherwise, if the embedded page scrolls but can't prevent any scrolling you could break existing content. Admittedly this is probably rare. Also, to scroll the embedded page you need to correctly target the scroller. In some cases this can result in going to the embedded page's main thread to do the hit test, again resulting in delayed scrolling. If there is a concern about content abusively blocking scroll, I imagine these alternate ways of blocking the scroll will become more common if we block the obvious one. TLDR; not saying I disagree, just that we won't be able to guarantee performance and correctness.
,
Oct 17
After this change, looking at a sample page: https://output.jsbin.com/nifipiy/13 I think we are now introducing a subtle UX problem here (on desktop). The blue frame has wheel handlers that prevent default and also vertical-scroll 'none'. However, if I click on the horizontal scrollbar it changes scroll position (although it sometimes doesn't work) but clicking on vertical scrollbar does nothing (it also does not look like disabled to me).
,
Oct 23
I filed issue 898151 . |
|||
►
Sign in to add a comment |
|||
Comment 1 by ekaramad@chromium.org
, Jun 18 2018Labels: -Pri-3 Pri-2
Status: Assigned (was: Untriaged)