Make implicit root scroller work on AMP pages |
||||
Issue descriptionChrome 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.
,
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
,
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
,
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
,
May 30 2018
,
May 31 2018
Reopening as this isn't applying in some situations.
,
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
,
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
,
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
,
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
,
Nov 6
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 |
||||
Comment 1 by bokan@chromium.org
, May 1 2018