New issue
Advanced search Search tips

Issue 853485 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

'vertical-scroll' policy shouldn't scroll iframes which are not allowed to block vertical scroll.

Project Member Reported by flackr@chromium.org, Jun 16 2018

Issue description

The '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.
 
Components: Blink>FeaturePolicy
Labels: -Pri-3 Pri-2
Status: Assigned (was: Untriaged)
Thanks for the detailed examples! All the proposed issues makes sense. My 2 cents:

1- This is an issue I have been wondering lately and couldn't quite come up with a proper solution. The proposed approach however essentially disables scrolling inside such frames which might be too limiting. I wonder it makes sense to 

  1-1 ensure some scroll reaches the parent frame instead. By that I mean (hand-waving), keep tab on how much deltas a frame used or, make sure (hand-waving more) some deltas reach the parent frame.

  1-2 Instead have a new policy on the content size of the document. This could have other benefits. We could then makes sure "vertical-scroll" also activates the content document size policy.


2- I think this is part of scripted scrolling which should have been blocked by "vertical-scroll"; the same way "scrollIntoView()" is blocked. We could ignore the calls to scrollTo or setting scrollOffset of elements.

3- (undesirable case): I don't know about this one. Maybe add a no-scroll feature which would enforce the scroll position inside a frame would not change?

But all in all the suggested solution is much simpler than introducing extra features with the undesirable side effect of actually disabling vertically scrolling contents of such frames.
Project Member

Comment 2 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)
Closing the bug following comment #2.
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?
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.



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.
Cc: bokan@chromium.org
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).
I filed  issue 898151 .

Sign in to add a comment