New issue
Advanced search Search tips

Issue 838683 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 6
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug

Blocking:
issue 798719



Sign in to add a comment

Make implicit root scroller work on AMP pages

Project Member Reported by bokan@chromium.org, May 1 2018

Issue description

Chrome Version       : 68.0.3409.2

We should implicitly promote the current main iframe in an AMP carousel. This is a tough case since it uses all kinds of transforms and complex DOM so if we can make this work it the implicit root scroller should be in good shape.
 

Comment 1 by bokan@chromium.org, May 1 2018

Blocking: 798719
Project Member

Comment 2 by bugdroid1@chromium.org, May 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6dba6e70c23fa5b80ca417168933f70f0ccb710a

commit 6dba6e70c23fa5b80ca417168933f70f0ccb710a
Author: David Bokan <bokan@chromium.org>
Date: Wed May 02 21:14:04 2018

Reevaluate implicit root scroller on iframe load

When an iframe loads its first or a new document we might swap
out the FrameView. When this happens, the validity of a
candidate for implicit promotion will change so we should
reevaluate at that time whether any candidates should be
promoted.

Also disabled implicit root scroller in frameElement-frame test
since it causes promotion of the frame which leads to pixel
differences in the output. When/if this feature ships we should
rebaseline this test.

Bug:  838683 
Change-Id: I55f3a49af8670897a31e63fd58469f5121b32671
Reviewed-on: https://chromium-review.googlesource.com/1036843
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555535}
[modify] https://crrev.com/6dba6e70c23fa5b80ca417168933f70f0ccb710a/third_party/WebKit/LayoutTests/fast/frames/frameElement-frame.html
[modify] https://crrev.com/6dba6e70c23fa5b80ca417168933f70f0ccb710a/third_party/blink/renderer/core/page/scrolling/root_scroller_controller.cc
[modify] https://crrev.com/6dba6e70c23fa5b80ca417168933f70f0ccb710a/third_party/blink/renderer/core/page/scrolling/root_scroller_test.cc

Project Member

Comment 3 by bugdroid1@chromium.org, May 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e952fd3e13420f487cc512c01528a6bf93121cca

commit e952fd3e13420f487cc512c01528a6bf93121cca
Author: David Bokan <bokan@chromium.org>
Date: Thu May 17 11:53:47 2018

[root-scroller] Select implicit from scrollable elements

To make implicit root scrollers promote and demote more reliably we
need to separate the notion of being a valid candidate from being a
valid root scroller. Currently, we build a list of candidates at
layout that we restrict to only valid implicit root scrollers. When
layout is finished we evaluate this list to determine which of the
elements will be promoted. Those that are no longer valid are removed
from the list.

Unfortunately this doesn't work too well since validity can change for
non-local reasons. For example, a parent may change its transform,
making its child viewport-filling. Thus, to make this work we'd have
to reevaluate candidates and validity everytime virtually anything on
the page changes.

This patch changes this method to make any currently scrollable element
a valid candidate. The list of candidates is maintained by listening to
PaintLayerScrollableAreas for when they become scrollable (allow
scrolling and have overflow), rather than hooking into layout for every
object. This list should be fairly small but it's more stable over the
life of a page so we can be more confident it truly contains all the
candidates we care about when we get to reevaluating which, if any,
should be promoted.

Bug:  838683 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I6d5c3d876d885fec638b46e992f3026e418737b7
Reviewed-on: https://chromium-review.googlesource.com/1038648
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559495}
[modify] https://crrev.com/e952fd3e13420f487cc512c01528a6bf93121cca/third_party/blink/renderer/core/layout/layout_block.cc
[modify] https://crrev.com/e952fd3e13420f487cc512c01528a6bf93121cca/third_party/blink/renderer/core/layout/layout_iframe.cc
[modify] https://crrev.com/e952fd3e13420f487cc512c01528a6bf93121cca/third_party/blink/renderer/core/layout/layout_iframe.h
[modify] https://crrev.com/e952fd3e13420f487cc512c01528a6bf93121cca/third_party/blink/renderer/core/page/scrolling/README.md
[modify] https://crrev.com/e952fd3e13420f487cc512c01528a6bf93121cca/third_party/blink/renderer/core/page/scrolling/root_scroller_controller.cc
[modify] https://crrev.com/e952fd3e13420f487cc512c01528a6bf93121cca/third_party/blink/renderer/core/page/scrolling/root_scroller_controller.h
[modify] https://crrev.com/e952fd3e13420f487cc512c01528a6bf93121cca/third_party/blink/renderer/core/page/scrolling/root_scroller_test.cc
[modify] https://crrev.com/e952fd3e13420f487cc512c01528a6bf93121cca/third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc

Project Member

Comment 4 by bugdroid1@chromium.org, May 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3583de23ea2e0cf11f626d63822181c6c36b2f64

commit 3583de23ea2e0cf11f626d63822181c6c36b2f64
Author: David Bokan <bokan@chromium.org>
Date: Thu May 24 11:15:54 2018

[root-scroller] Select effective by frame tree walk

Currently, the effective root scroller for a frame is selected immediately after that
frame performs a layout or style update. This is problematic for
selecting an implicit root scroller since changes in a child frame can
affect whcih element we select in the parent. Since promotion can cause
a layout, this leads to potential infinite loops, lifecycle violations
and a generally messy integration with the document lifecycle.

This patch removes the style and layout hooks in favor of a full frame
tree walk after the layout step of the main frame. This has a few
important implications:

- Selection is done in post order, i.e. child frames perform selection
  before parents. This means selection is performed after we've resolved
  everything it depends on. Thus, selection doesn't affect the lifecycle
  in any frames but the one being selected.

- Since selection occurs after layout, when each document lifecycle is
  in LayoutClean, any changes need to be synchronously updated to ensure
  we leave selection in LayoutClean.

- Selection can still be initiated from frame navigations which happen
  outside the document lifecycle. In those cases we don't need to
  synchronously update any changes.

- Synchronous, script-initiated layout updates no longer cause a frame
  to perform selection. e.g. If a page calls document.rootScroller = a;
  The root scroller wont be effective until the next time a full
  lifecycle update is performed at BeginMainFrame. Causing a layout,
  via element.scrollTop for example, will not cause promotion. This
  required updating several tests to ensure we update the lifecycle
  before making assertions. (Some non-rootscroller tests also had to be
  updated since they weren't performing a lifecycle update between
  navigations and the layout viewport wasn't updated.)

Bug:  838683 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I0649a50832432c03bf1bf638b6d46e440854bc1c
Reviewed-on: https://chromium-review.googlesource.com/1060032
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561460}
[modify] https://crrev.com/3583de23ea2e0cf11f626d63822181c6c36b2f64/third_party/WebKit/LayoutTests/fast/scrolling/editor-command-scroll-page-scale.html
[modify] https://crrev.com/3583de23ea2e0cf11f626d63822181c6c36b2f64/third_party/WebKit/LayoutTests/rootscroller/gesture-scroll-document-not-root-scroller.html
[add] https://crrev.com/3583de23ea2e0cf11f626d63822181c6c36b2f64/third_party/WebKit/LayoutTests/rootscroller/resources/rootscroller-util.js
[modify] https://crrev.com/3583de23ea2e0cf11f626d63822181c6c36b2f64/third_party/WebKit/LayoutTests/rootscroller/rootscroller-during-fullscreen.html
[modify] https://crrev.com/3583de23ea2e0cf11f626d63822181c6c36b2f64/third_party/WebKit/LayoutTests/rootscroller/set-root-scroller.html
[modify] https://crrev.com/3583de23ea2e0cf11f626d63822181c6c36b2f64/third_party/blink/renderer/core/dom/document.cc
[modify] https://crrev.com/3583de23ea2e0cf11f626d63822181c6c36b2f64/third_party/blink/renderer/core/exported/web_view_test.cc
[modify] https://crrev.com/3583de23ea2e0cf11f626d63822181c6c36b2f64/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/3583de23ea2e0cf11f626d63822181c6c36b2f64/third_party/blink/renderer/core/frame/visual_viewport_test.cc
[modify] https://crrev.com/3583de23ea2e0cf11f626d63822181c6c36b2f64/third_party/blink/renderer/core/page/scrolling/README.md
[modify] https://crrev.com/3583de23ea2e0cf11f626d63822181c6c36b2f64/third_party/blink/renderer/core/page/scrolling/root_scroller_controller.cc
[modify] https://crrev.com/3583de23ea2e0cf11f626d63822181c6c36b2f64/third_party/blink/renderer/core/page/scrolling/root_scroller_controller.h
[modify] https://crrev.com/3583de23ea2e0cf11f626d63822181c6c36b2f64/third_party/blink/renderer/core/page/scrolling/root_scroller_test.cc

Comment 5 by bokan@chromium.org, May 30 2018

Status: Fixed (was: Started)

Comment 6 by bokan@chromium.org, May 31 2018

Status: Started (was: Fixed)
Reopening as this isn't applying in some situations.
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ace3f1f79bf62fe4f30ae014d8d19caecc94e997

commit ace3f1f79bf62fe4f30ae014d8d19caecc94e997
Author: David Bokan <bokan@chromium.org>
Date: Wed Aug 08 00:13:43 2018

[RootScroller] Allow main document horizontal overflow

When implicitly promoting an element to be the root scroller, we first
check if the main document has any overflow. If it does, we err on the
side of not promoting since it is likely the main document's content is
important and meant to be scrollable.

This heuristic turns out to be too restrictive. For example, this makes
it hard to build a root scrolling carousel. This CL loosens the
restriction to allow horizontal overflow.

Bug:  838683 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I6007f6faa8e2006799a9dca6240b68ff0de7f09a
Reviewed-on: https://chromium-review.googlesource.com/1165859
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581408}
[modify] https://crrev.com/ace3f1f79bf62fe4f30ae014d8d19caecc94e997/third_party/blink/renderer/core/page/scrolling/root_scroller_controller.cc
[modify] https://crrev.com/ace3f1f79bf62fe4f30ae014d8d19caecc94e997/third_party/blink/renderer/core/page/scrolling/root_scroller_test.cc
[modify] https://crrev.com/ace3f1f79bf62fe4f30ae014d8d19caecc94e997/third_party/blink/renderer/core/paint/paint_layer_scrollable_area.h

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a61bc12b11e325035ec4f2aca54eda34a12abcd6

commit a61bc12b11e325035ec4f2aca54eda34a12abcd6
Author: Peter Kasting <pkasting@chromium.org>
Date: Sat Aug 11 07:49:11 2018

Revert "[RootScroller] Allow main document horizontal overflow"

This reverts commit ace3f1f79bf62fe4f30ae014d8d19caecc94e997.

Reason for revert: Checking to see if this caused flaky scrolling failures on Mac 10.13 ( https://bugs.chromium.org/p/chromium/issues/detail?id=873435 )

Original change's description:
> [RootScroller] Allow main document horizontal overflow
> 
> When implicitly promoting an element to be the root scroller, we first
> check if the main document has any overflow. If it does, we err on the
> side of not promoting since it is likely the main document's content is
> important and meant to be scrollable.
> 
> This heuristic turns out to be too restrictive. For example, this makes
> it hard to build a root scrolling carousel. This CL loosens the
> restriction to allow horizontal overflow.
> 
> Bug:  838683 
> Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
> Change-Id: I6007f6faa8e2006799a9dca6240b68ff0de7f09a
> Reviewed-on: https://chromium-review.googlesource.com/1165859
> Commit-Queue: David Bokan <bokan@chromium.org>
> Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#581408}

TBR=bokan@chromium.org,dtapuska@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  838683 
Change-Id: I14e6a0158bb8b06f2344c3b30b38b86c6eabd583
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Reviewed-on: https://chromium-review.googlesource.com/1172004
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582439}
[modify] https://crrev.com/a61bc12b11e325035ec4f2aca54eda34a12abcd6/third_party/blink/renderer/core/page/scrolling/root_scroller_controller.cc
[modify] https://crrev.com/a61bc12b11e325035ec4f2aca54eda34a12abcd6/third_party/blink/renderer/core/page/scrolling/root_scroller_test.cc
[modify] https://crrev.com/a61bc12b11e325035ec4f2aca54eda34a12abcd6/third_party/blink/renderer/core/paint/paint_layer_scrollable_area.h

Project Member

Comment 9 by bugdroid1@chromium.org, Aug 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/801a32fcffd68fe5c1a3841be3608d18046c6175

commit 801a32fcffd68fe5c1a3841be3608d18046c6175
Author: David Bokan <bokan@chromium.org>
Date: Sat Aug 11 20:51:41 2018

Reland "[RootScroller] Allow main document horizontal overflow"

This is a reland of ace3f1f79bf62fe4f30ae014d8d19caecc94e997

Original change's description:
> [RootScroller] Allow main document horizontal overflow
>
> When implicitly promoting an element to be the root scroller, we first
> check if the main document has any overflow. If it does, we err on the
> side of not promoting since it is likely the main document's content is
> important and meant to be scrollable.
>
> This heuristic turns out to be too restrictive. For example, this makes
> it hard to build a root scrolling carousel. This CL loosens the
> restriction to allow horizontal overflow.
>
> Bug:  838683 
> Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
> Change-Id: I6007f6faa8e2006799a9dca6240b68ff0de7f09a
> Reviewed-on: https://chromium-review.googlesource.com/1165859
> Commit-Queue: David Bokan <bokan@chromium.org>
> Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#581408}

Bug:  838683 
Change-Id: I886908a56db7a463c619135ed08712312ccacbfc
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
TBR: bokan, dtapuska
Reviewed-on: https://chromium-review.googlesource.com/1171717
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582458}
[modify] https://crrev.com/801a32fcffd68fe5c1a3841be3608d18046c6175/third_party/blink/renderer/core/page/scrolling/root_scroller_controller.cc
[modify] https://crrev.com/801a32fcffd68fe5c1a3841be3608d18046c6175/third_party/blink/renderer/core/page/scrolling/root_scroller_test.cc
[modify] https://crrev.com/801a32fcffd68fe5c1a3841be3608d18046c6175/third_party/blink/renderer/core/paint/paint_layer_scrollable_area.h

Project Member

Comment 10 by bugdroid1@chromium.org, Aug 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/98721216e6738276fb1756dadf82c789ab2c12ba

commit 98721216e6738276fb1756dadf82c789ab2c12ba
Author: David Bokan <bokan@chromium.org>
Date: Thu Aug 16 14:41:57 2018

[RootScroller] Fix precision issue when comparing bounds

A common pattern on the web is to resize a container to the window's
innerWidth and innerHeight so that it matches the viewport size and
accounts for the URL bar's presence. Unfortunately, these values are do
not report fractional CSS pixels, they're floored. So on high DPI
devices, innerHeight * devicePixelRatio != initial containing block's
size.

This patch allows applying the root scroller on pages following this
pattern by ceil'ing the bounds values before comparing them to the ICB.

Two additional unrelated changes:

- Remove unused parameter in FillsViewport

This is always true since some changes were made recently.

- Early out for non-main frames in ProcessImplicitCandidates

We avoid adding implicit candidates for non-main frames already so this
is a no-op but this early out is convenient for debugging and logging.

Bug:  838683 
Change-Id: I740816184a26aa067e135041543170356e90fed5
Reviewed-on: https://chromium-review.googlesource.com/1176550
Reviewed-by: Ella Ge <eirage@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583646}
[modify] https://crrev.com/98721216e6738276fb1756dadf82c789ab2c12ba/third_party/blink/renderer/core/page/scrolling/root_scroller_controller.cc
[modify] https://crrev.com/98721216e6738276fb1756dadf82c789ab2c12ba/third_party/blink/renderer/core/page/scrolling/root_scroller_test.cc

Status: Fixed (was: Started)
This appears to be working on AMP viewer pretty well today so I don't think we need any more changes here.

Sign in to add a comment