New issue
Advanced search Search tips

Issue 897288 link

Starred by 3 users
Status: Fixed
Owner:
bokan@chromium.org
Closed: Nov 5
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 3
Type: Task



Sign in to add a comment

Move scroll customization callbacks from Element to Node

Project Member Reported by bokan@chromium.org, Oct 19 
The fact that scroll-customization callbacks are associated with an Element rather than a node add significant complexity to scrolling code, since document-level scrolling works on the LayoutView (i.e. the Document Node, rather than Element).

This means making page/scrolling/scroll_customization_callbacks.h store a map from Node->Callback rather than Element->Callback.
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 24 

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

commit 9f54d240af5d366b1d6a4d9b84438192a9f0a11c
Author: David Bokan <bokan@chromium.org>
Date: Wed Oct 24 16:12:46 2018

Move ScrollCustomization to work on Nodes

ScrollCustomization currently places the API surface on Element rather
than Node, meaning a callback can only be set on Elements. This becomes
a problem as the Document node is not an Element but is the node that's
used to perform scrolling for the frame's viewport. This causes all
kinds of contortions in various places in scrolling code. One example
where this is causing an issue is https://crrev.com/c/1287176. See also
ScrollManager::RecomputeScrollChain.

This CL is a mechanical move of ScrollCustomization to use Node as the
primitive on which callbacks operate. This will allow us to simplify
scrolling code in several places; however, this patch is just a
mechanical movement of the code from Element to Node. Future patches
will actually modify the behavior to make use of the fact that we can
now use the document node.

Bug:  897288 
Change-Id: I0cab17888d30f981b731848f5ba6e499f966efca
Reviewed-on: https://chromium-review.googlesource.com/c/1297576
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602358}
[modify] https://crrev.com/9f54d240af5d366b1d6a4d9b84438192a9f0a11c/third_party/blink/renderer/core/dom/element.cc
[modify] https://crrev.com/9f54d240af5d366b1d6a4d9b84438192a9f0a11c/third_party/blink/renderer/core/dom/element.h
[modify] https://crrev.com/9f54d240af5d366b1d6a4d9b84438192a9f0a11c/third_party/blink/renderer/core/dom/element.idl
[modify] https://crrev.com/9f54d240af5d366b1d6a4d9b84438192a9f0a11c/third_party/blink/renderer/core/dom/node.cc
[modify] https://crrev.com/9f54d240af5d366b1d6a4d9b84438192a9f0a11c/third_party/blink/renderer/core/dom/node.h
[modify] https://crrev.com/9f54d240af5d366b1d6a4d9b84438192a9f0a11c/third_party/blink/renderer/core/dom/node.idl
[modify] https://crrev.com/9f54d240af5d366b1d6a4d9b84438192a9f0a11c/third_party/blink/renderer/core/input/scroll_manager.cc
[modify] https://crrev.com/9f54d240af5d366b1d6a4d9b84438192a9f0a11c/third_party/blink/renderer/core/page/scrolling/root_scroller_util.cc
[modify] https://crrev.com/9f54d240af5d366b1d6a4d9b84438192a9f0a11c/third_party/blink/renderer/core/page/scrolling/root_scroller_util.h
[modify] https://crrev.com/9f54d240af5d366b1d6a4d9b84438192a9f0a11c/third_party/blink/renderer/core/page/scrolling/scroll_customization_callbacks.cc
[modify] https://crrev.com/9f54d240af5d366b1d6a4d9b84438192a9f0a11c/third_party/blink/renderer/core/page/scrolling/scroll_customization_callbacks.h
[modify] https://crrev.com/9f54d240af5d366b1d6a4d9b84438192a9f0a11c/third_party/blink/renderer/core/page/scrolling/scroll_state.cc
[modify] https://crrev.com/9f54d240af5d366b1d6a4d9b84438192a9f0a11c/third_party/blink/renderer/core/page/scrolling/scroll_state.h
[modify] https://crrev.com/9f54d240af5d366b1d6a4d9b84438192a9f0a11c/third_party/blink/renderer/core/page/scrolling/scroll_state_test.cc
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 25 

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

commit 92aa42c2a5abb0842a83e306e287eb4467b48f56
Author: David Bokan <bokan@chromium.org>
Date: Thu Oct 25 12:52:34 2018

Fix IsEffectiveRootScroller bit for default

This bit was added in crrev.com/2685a6b3e82be7711c6d3b02f225b1b90bb9bf46
and is set whenever we change the effective root scroller. This misses
an important case: if we never change the default root scroller then we
never set the bit.

We default the effective root scroller to the Document node. This
happens initially during construction time, before the LayoutView is
created. This means we cannot set the bit during construction.

This CL sets the bit when we construct the LayoutView.

Bug:  897288 
Change-Id: Icba6e26678b32b1bcf1318b2a5087bd80be6907e
Reviewed-on: https://chromium-review.googlesource.com/c/1298502
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602680}
[modify] https://crrev.com/92aa42c2a5abb0842a83e306e287eb4467b48f56/third_party/blink/renderer/core/dom/document.h
[modify] https://crrev.com/92aa42c2a5abb0842a83e306e287eb4467b48f56/third_party/blink/renderer/core/layout/layout_view.cc
[modify] https://crrev.com/92aa42c2a5abb0842a83e306e287eb4467b48f56/third_party/blink/renderer/core/page/scrolling/root_scroller_test.cc
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 25 

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

commit 5c08cd277470af2b39e4eabd2534404db63fd16d
Author: David Bokan <bokan@chromium.org>
Date: Thu Oct 25 18:22:11 2018

Convert TopDocumentRootScrollerController to use Node

In https://crrev.com/9f54d240af5d366b1d6a4d9b84438192a9f0a11c we moved
ScrollCustomization from using Elements to Node. This allows us to
simplify scrolling code since we've had to do much disambiguation
between the documentElement and the document node. Scrolling actually
occurs on the Document node but the "viewport scrolling actions"
callback has to be registered on the documentElement.

The main beneficiary of allowing us to register the viewport callback on
a Node rather than Element is the TopDocumentRootScrollerController.
Since it's the component responsible for setting the callback, it has
much logic to do these Element <-> Node swaps.

This CL prepares for a larger behavioral change by swapping the relevant
types in TopDocumentRootScrollerController from Node to Element. The
document node <-> document element swaps still remain for now to keep
this change mechanical. A followup patch will make the behavioral
changes that actually simplify the code.

Bug:  897288 
Change-Id: I93e133a71b16d2d409c9af0d8529a6ecf02b3cad
Reviewed-on: https://chromium-review.googlesource.com/c/1299197
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602798}
[modify] https://crrev.com/5c08cd277470af2b39e4eabd2534404db63fd16d/third_party/blink/renderer/core/page/scrolling/root_scroller_test.cc
[modify] https://crrev.com/5c08cd277470af2b39e4eabd2534404db63fd16d/third_party/blink/renderer/core/page/scrolling/top_document_root_scroller_controller.cc
[modify] https://crrev.com/5c08cd277470af2b39e4eabd2534404db63fd16d/third_party/blink/renderer/core/page/scrolling/top_document_root_scroller_controller.h
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 25 

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

commit f51a68b073770244754b996f0d9e7599f1c09e99
Author: David Bokan <bokan@chromium.org>
Date: Thu Oct 25 23:54:40 2018

Convert scrolling code to use Node

In https://crrev.com/9f54d240af5d366b1d6a4d9b84438192a9f0a11c we moved
ScrollCustomization from using Elements to Node. This allows us to use
Nodes in the scrolling code which is much more natural since the
viewport scrolls using the LayoutView (document Node)'s scrollable area.

The main change in this CL is letting the Document node be part of the
scroll chain, instead of using the documentElement in its place. This
removes some special-casing all the way through the scrolling pipeline.

As a result the Global Root Scroller now sets the viewport scroll
callback on the document node.

Bug:  897288 
Change-Id: I899e6007b676efb94675d94b3d97702c8f1f2a94
Reviewed-on: https://chromium-review.googlesource.com/c/1299205
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602935}
[modify] https://crrev.com/f51a68b073770244754b996f0d9e7599f1c09e99/third_party/blink/renderer/core/dom/node.cc
[modify] https://crrev.com/f51a68b073770244754b996f0d9e7599f1c09e99/third_party/blink/renderer/core/dom/node.h
[modify] https://crrev.com/f51a68b073770244754b996f0d9e7599f1c09e99/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/f51a68b073770244754b996f0d9e7599f1c09e99/third_party/blink/renderer/core/input/scroll_manager.cc
[modify] https://crrev.com/f51a68b073770244754b996f0d9e7599f1c09e99/third_party/blink/renderer/core/input/scroll_manager.h
[modify] https://crrev.com/f51a68b073770244754b996f0d9e7599f1c09e99/third_party/blink/renderer/core/page/scrolling/root_scroller_controller.cc
[modify] https://crrev.com/f51a68b073770244754b996f0d9e7599f1c09e99/third_party/blink/renderer/core/page/scrolling/root_scroller_controller.h
[modify] https://crrev.com/f51a68b073770244754b996f0d9e7599f1c09e99/third_party/blink/renderer/core/page/scrolling/root_scroller_test.cc
[modify] https://crrev.com/f51a68b073770244754b996f0d9e7599f1c09e99/third_party/blink/renderer/core/page/scrolling/root_scroller_util.cc
[modify] https://crrev.com/f51a68b073770244754b996f0d9e7599f1c09e99/third_party/blink/renderer/core/page/scrolling/root_scroller_util.h
[modify] https://crrev.com/f51a68b073770244754b996f0d9e7599f1c09e99/third_party/blink/renderer/core/page/scrolling/scroll_state.cc
[modify] https://crrev.com/f51a68b073770244754b996f0d9e7599f1c09e99/third_party/blink/renderer/core/page/scrolling/top_document_root_scroller_controller.cc
[modify] https://crrev.com/f51a68b073770244754b996f0d9e7599f1c09e99/third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc

Comment 5 by woxxom@gmail.com, Oct 27 

r602935 seems to disable scrolling in chrome://extensions pages
(the browser window should be small enough to have a vertical scrollbar).
Should I open a new issue?

Comment 6 by bokan@chromium.org, Oct 29

Status: Fixed (was: Assigned)
Re #5: Yes please

Comment 7 by bokan@chromium.org, Oct 29 

Though it may be a dup of  issue 899161 ?

Comment 8 by woxxom@gmail.com, Oct 29 

> dup of  issue 899161 ?

seems so!
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 29 

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

commit d4424640365874fbc63eeb94674841aa3f2487f2
Author: David Bokan <bokan@chromium.org>
Date: Mon Oct 29 21:08:34 2018

Revert "Convert scrolling code to use Node"

This reverts commit f51a68b073770244754b996f0d9e7599f1c09e99.

Reason for revert: breaks scrolling on some pages:  https://crbug.com/899161 

Original change's description:
> Convert scrolling code to use Node
>
> In https://crrev.com/9f54d240af5d366b1d6a4d9b84438192a9f0a11c we moved
> ScrollCustomization from using Elements to Node. This allows us to use
> Nodes in the scrolling code which is much more natural since the
> viewport scrolls using the LayoutView (document Node)'s scrollable area.
>
> The main change in this CL is letting the Document node be part of the
> scroll chain, instead of using the documentElement in its place. This
> removes some special-casing all the way through the scrolling pipeline.
>
> As a result the Global Root Scroller now sets the viewport scroll
> callback on the document node.
>
> Bug:  897288 
> Change-Id: I899e6007b676efb94675d94b3d97702c8f1f2a94
> Reviewed-on: https://chromium-review.googlesource.com/c/1299205
> Commit-Queue: David Bokan <bokan@chromium.org>
> Reviewed-by: Steve Kobes <skobes@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#602935}

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

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

Bug:  897288 , 899161 
Change-Id: Ic75c23a1bcde6d86e554c2b5bdb5548b37d57e5f
Reviewed-on: https://chromium-review.googlesource.com/c/1305820
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603626}
[modify] https://crrev.com/d4424640365874fbc63eeb94674841aa3f2487f2/third_party/blink/renderer/core/dom/node.cc
[modify] https://crrev.com/d4424640365874fbc63eeb94674841aa3f2487f2/third_party/blink/renderer/core/dom/node.h
[modify] https://crrev.com/d4424640365874fbc63eeb94674841aa3f2487f2/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/d4424640365874fbc63eeb94674841aa3f2487f2/third_party/blink/renderer/core/input/scroll_manager.cc
[modify] https://crrev.com/d4424640365874fbc63eeb94674841aa3f2487f2/third_party/blink/renderer/core/input/scroll_manager.h
[modify] https://crrev.com/d4424640365874fbc63eeb94674841aa3f2487f2/third_party/blink/renderer/core/page/scrolling/root_scroller_controller.cc
[modify] https://crrev.com/d4424640365874fbc63eeb94674841aa3f2487f2/third_party/blink/renderer/core/page/scrolling/root_scroller_controller.h
[modify] https://crrev.com/d4424640365874fbc63eeb94674841aa3f2487f2/third_party/blink/renderer/core/page/scrolling/root_scroller_test.cc
[modify] https://crrev.com/d4424640365874fbc63eeb94674841aa3f2487f2/third_party/blink/renderer/core/page/scrolling/root_scroller_util.cc
[modify] https://crrev.com/d4424640365874fbc63eeb94674841aa3f2487f2/third_party/blink/renderer/core/page/scrolling/root_scroller_util.h
[modify] https://crrev.com/d4424640365874fbc63eeb94674841aa3f2487f2/third_party/blink/renderer/core/page/scrolling/scroll_state.cc
[modify] https://crrev.com/d4424640365874fbc63eeb94674841aa3f2487f2/third_party/blink/renderer/core/page/scrolling/top_document_root_scroller_controller.cc
[modify] https://crrev.com/d4424640365874fbc63eeb94674841aa3f2487f2/third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc

Comment 10 by bokan@chromium.org, Oct 30

Status: Started (was: Fixed)
Patch was reverted, reopening
Project Member

Comment 11 by bugdroid1@chromium.org, Nov 5 

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

commit 469e7e4845955edfe85b5427914df43ae53268ba
Author: David Bokan <bokan@chromium.org>
Date: Mon Nov 05 14:50:29 2018

[Reland] Convert scrolling code to use Node

In https://crrev.com/9f54d240af5d366b1d6a4d9b84438192a9f0a11c we moved
ScrollCustomization from using Elements to Node. This allows us to use
Nodes in the scrolling code which is much more natural since the
viewport scrolls using the LayoutView (document Node)'s scrollable area.

The main change in this CL is letting the Document node be part of the
scroll chain, instead of using the documentElement in its place. This
removes some special-casing all the way through the scrolling pipeline.

As a result the Global Root Scroller now sets the viewport scroll
callback on the document node.

Reland node: This relands the original patch with the fix reviewed
in https://crrev.com/c/1302856. Originally landed patch is uploaded in
Patchset 1 for easy comparison of changes.

TBR=dcheng@chromium.org,kenrb@chromium.org,lfg@chromium.org

Bug:  897288 , 899161 
Change-Id: Ib6b61eb3a193877abb61f896a9d1de4747496514
Reviewed-on: https://chromium-review.googlesource.com/c/1307843
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605327}
[modify] https://crrev.com/469e7e4845955edfe85b5427914df43ae53268ba/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/469e7e4845955edfe85b5427914df43ae53268ba/third_party/blink/renderer/core/dom/node.cc
[modify] https://crrev.com/469e7e4845955edfe85b5427914df43ae53268ba/third_party/blink/renderer/core/dom/node.h
[modify] https://crrev.com/469e7e4845955edfe85b5427914df43ae53268ba/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/469e7e4845955edfe85b5427914df43ae53268ba/third_party/blink/renderer/core/input/scroll_manager.cc
[modify] https://crrev.com/469e7e4845955edfe85b5427914df43ae53268ba/third_party/blink/renderer/core/input/scroll_manager.h
[modify] https://crrev.com/469e7e4845955edfe85b5427914df43ae53268ba/third_party/blink/renderer/core/page/scrolling/root_scroller_controller.cc
[modify] https://crrev.com/469e7e4845955edfe85b5427914df43ae53268ba/third_party/blink/renderer/core/page/scrolling/root_scroller_controller.h
[modify] https://crrev.com/469e7e4845955edfe85b5427914df43ae53268ba/third_party/blink/renderer/core/page/scrolling/root_scroller_test.cc
[modify] https://crrev.com/469e7e4845955edfe85b5427914df43ae53268ba/third_party/blink/renderer/core/page/scrolling/root_scroller_util.cc
[modify] https://crrev.com/469e7e4845955edfe85b5427914df43ae53268ba/third_party/blink/renderer/core/page/scrolling/root_scroller_util.h
[modify] https://crrev.com/469e7e4845955edfe85b5427914df43ae53268ba/third_party/blink/renderer/core/page/scrolling/scroll_state.cc
[modify] https://crrev.com/469e7e4845955edfe85b5427914df43ae53268ba/third_party/blink/renderer/core/page/scrolling/top_document_root_scroller_controller.cc
[modify] https://crrev.com/469e7e4845955edfe85b5427914df43ae53268ba/third_party/blink/renderer/core/page/scrolling/top_document_root_scroller_controller.h
[modify] https://crrev.com/469e7e4845955edfe85b5427914df43ae53268ba/third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc

Comment 12 by bokan@chromium.org, Nov 5

Status: Fixed (was: Started)
►

Sign in to add a comment