Don't return false for usesCompositedScrolling for "intrinsic scrollability". (issue 2271883002 by chrishtr@chromium.org)

1 view
Skip to first unread message

chri...@chromium.org

unread,
Aug 23, 2016, 7:10:21 PM8/23/16
to bo...@chromium.org, dtap...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org
Reviewers: bokan, dtapuska
CL: https://codereview.chromium.org/2271883002/

Message:
WDYT? I'm a bit confused about how this particular condition could be needed to
force
main-thread scrolling, but not be part of
PaintLayerScrollableArea::shouldScrollOnMainThread.

Description:
Don't return false for usesCompositedScrolling for "intrinsic scrollability".

This code was added in https://chromiumcodereview.appspot.com/22488004 to
support proper touch event bubbling. However, the test that went with it still
passes with this change. ScrollCoordinator calls usesCompositedScrolling to
determine the rect for computeShouldHandleScrollGestureOnMainThreadRegion.

Should this code be part of PaintLayerScrollableArea::shouldScrollOnMainThread?

BUG=635724
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+125, -4 lines):
A third_party/WebKit/LayoutTests/paint/selection/selection-in-composited-scrolling-container.html
A third_party/WebKit/LayoutTests/paint/selection/selection-in-composited-scrolling-container-expected.txt
M third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp


chri...@chromium.org

unread,
Aug 23, 2016, 9:26:57 PM8/23/16
to bo...@chromium.org, dtap...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org
FYI I can't commit this as-is due to another problem with carets, but could you
answer my
question about whether this change is ok or somehow breaks things, and if so
how?

https://codereview.chromium.org/2271883002/

dtap...@chromium.org

unread,
Aug 24, 2016, 9:35:31 AM8/24/16
to chri...@chromium.org, bo...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org
On 2016/08/24 at 01:26:57, chrishtr wrote:
> FYI I can't commit this as-is due to another problem with carets, but could
you answer my
> question about whether this change is ok or somehow breaks things, and if so
how?

bo...@chromium.org

unread,
Aug 24, 2016, 11:31:05 AM8/24/16
to chri...@chromium.org, dtap...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org
On 2016/08/23 23:10:20, chrishtr wrote:
> WDYT? I'm a bit confused about how this particular condition could be needed
to
> force
> main-thread scrolling, but not be part of
> PaintLayerScrollableArea::shouldScrollOnMainThread.

Hey Chris,

It looks to me like PLSA::shouldScrollOnMainThread is used to decide where to
run scroll animations while PLSA::usesCompositedScrolling is used to calculate
the main thread scrolling regions for the compositor. It seems strange that we
have two separate methods for this. I think the reason that condition being only
in the one method hasn't been a problem is because this was meant for touch
scrolling which doesn't use scroll animations. Adding it to
shouldScrollOnMainThread wouldn't hurt though.

I think the reason the test continues to pass is that LayoutTests send events to
the main thread, not CC, so the usesCompositedScrolling decision is moot since
scrolling will happen on the main thread in the test. It's the
userInputScrollable change in my CL that actually made it work once the scroll
happened on the main thread. With this CL, can you touch scroll an input field
with overflowing text? It's possible enough has changed that this isn't actually
needed anymore.

FYI, I think this was the first CL I landed in Chrome so I didn't have a great
understanding of how things worked. There's probably a better way to make the
input box scrollable. IIRC, the reason it didn't work was because
userInputScrollable would check the box's overflow style but the input box
wasn't styled as overflowing; instead, it hid some child elements that were. We
could probably rearrange that make scrolling inputs less special. Maybe it could
even scroll on the compositor, though I'm not sure about how that would work
with selection/carets and the like.

https://codereview.chromium.org/2271883002/

chri...@chromium.org

unread,
Aug 25, 2016, 5:13:02 PM8/25/16
to bo...@chromium.org, p...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org
@bokan: I confirmed that touch scrolling works for an input box (in particular,
the one
referenced as a reduced testcase in the bug attached to this CL). Does the
change to
PaintLayerScrollableArea look ok to you?

@pdr: other than that, the problem was that (a) carets for scrollable inoput
incorrectly included
scroll offset of the input element, and (b) it needs to have its own display
item client,
to preserve separate cacheability from m_graphicsLayer where the rest of the
LayoutBlock
paints. wkorman did the same thing in https://codereview.chromium.org/2254893005

https://codereview.chromium.org/2271883002/

p...@chromium.org

unread,
Aug 25, 2016, 6:07:12 PM8/25/16
to chri...@chromium.org, bo...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org
This looks pretty good! The caret-outside-block.html failure looks like a
trivial fix. Just some minor nits / questions.


https://codereview.chromium.org/2271883002/diff/60001/third_party/WebKit/LayoutTests/editing/caret/caret-color.html
File third_party/WebKit/LayoutTests/editing/caret/caret-color.html
(right):

https://codereview.chromium.org/2271883002/diff/60001/third_party/WebKit/LayoutTests/editing/caret/caret-color.html#newcode16
third_party/WebKit/LayoutTests/editing/caret/caret-color.html:16:
moveSelectionForwardByLineCommand();
nit

https://codereview.chromium.org/2271883002/diff/60001/third_party/WebKit/Source/core/editing/CaretBase.cpp
File third_party/WebKit/Source/core/editing/CaretBase.cpp (right):

https://codereview.chromium.org/2271883002/diff/60001/third_party/WebKit/Source/core/editing/CaretBase.cpp#newcode148
third_party/WebKit/Source/core/editing/CaretBase.cpp:148: if
(caretLayoutBlock->usesCompositedScrolling())
usesCompositedScrolling() can change dynamically. Do we need to tell the
old client to invalidate or paint when this occurs?

https://codereview.chromium.org/2271883002/diff/60001/third_party/WebKit/Source/core/editing/CaretBase.cpp#newcode167
third_party/WebKit/Source/core/editing/CaretBase.cpp:167: // // FIXME:
We should not allow paint invalidation out of paint invalidation state.
crbug.com/457415
Nit: Your comment has a comment

https://codereview.chromium.org/2271883002/diff/60001/third_party/WebKit/Source/core/paint/BlockPainter.cpp
File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right):

https://codereview.chromium.org/2271883002/diff/60001/third_party/WebKit/Source/core/paint/BlockPainter.cpp#newcode197
third_party/WebKit/Source/core/paint/BlockPainter.cpp:197: const
DisplayItemClient& displayItemClient =
m_layoutBlock.usesCompositedScrolling() ? static_cast<const
DisplayItemClient&>(*m_layoutBlock.layer()->graphicsLayerBackingForScrolling())
Nit: are these static cats needed?

https://codereview.chromium.org/2271883002/

bo...@chromium.org

unread,
Aug 26, 2016, 11:38:35 AM8/26/16
to chri...@chromium.org, p...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org

https://codereview.chromium.org/2271883002/diff/60001/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
(right):

https://codereview.chromium.org/2271883002/diff/60001/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp#newcode1480
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1480:
// Scroll form controls on the main thread so they exhibit correct touch
scroll event bubbling
This comment is backwards now since we return "don't scroll on main
thread" but I think we could just get rid of this condition altogether.

https://codereview.chromium.org/2271883002/

chri...@chromium.org

unread,
Aug 26, 2016, 11:42:36 AM8/26/16
to bo...@chromium.org, p...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org

https://codereview.chromium.org/2271883002/diff/60001/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
(right):

https://codereview.chromium.org/2271883002/diff/60001/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp#newcode1480
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1480:
// Scroll form controls on the main thread so they exhibit correct touch
scroll event bubbling
On 2016/08/26 at 15:38:32, bokan wrote:
> This comment is backwards now since we return "don't scroll on main
thread" but I think we could just get rid of this condition altogether.

bo...@chromium.org

unread,
Aug 26, 2016, 11:44:43 AM8/26/16
to chri...@chromium.org, p...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org

chri...@chromium.org

unread,
Aug 26, 2016, 12:02:26 PM8/26/16
to bo...@chromium.org, p...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org

https://codereview.chromium.org/2271883002/diff/60001/third_party/WebKit/LayoutTests/editing/caret/caret-color.html
File third_party/WebKit/LayoutTests/editing/caret/caret-color.html
(right):

https://codereview.chromium.org/2271883002/diff/60001/third_party/WebKit/LayoutTests/editing/caret/caret-color.html#newcode16
third_party/WebKit/LayoutTests/editing/caret/caret-color.html:16:
moveSelectionForwardByLineCommand();
On 2016/08/25 at 22:07:11, pdr. wrote:
> nit

Done.
https://codereview.chromium.org/2271883002/diff/60001/third_party/WebKit/Source/core/editing/CaretBase.cpp#newcode167
third_party/WebKit/Source/core/editing/CaretBase.cpp:167: // // FIXME:
We should not allow paint invalidation out of paint invalidation state.
crbug.com/457415
On 2016/08/25 at 22:07:11, pdr. wrote:
> Nit: Your comment has a comment

Done.


https://codereview.chromium.org/2271883002/diff/60001/third_party/WebKit/Source/core/paint/BlockPainter.cpp
File third_party/WebKit/Source/core/paint/BlockPainter.cpp (right):

https://codereview.chromium.org/2271883002/diff/60001/third_party/WebKit/Source/core/paint/BlockPainter.cpp#newcode197
third_party/WebKit/Source/core/paint/BlockPainter.cpp:197: const
DisplayItemClient& displayItemClient =
m_layoutBlock.usesCompositedScrolling() ? static_cast<const
DisplayItemClient&>(*m_layoutBlock.layer()->graphicsLayerBackingForScrolling())
On 2016/08/25 at 22:07:11, pdr. wrote:
> Nit: are these static cats needed?

In fact, I had forgotten to remove this code now that it's in CaretBase.
Done.

https://codereview.chromium.org/2271883002/

p...@chromium.org

unread,
Aug 26, 2016, 2:27:14 PM8/26/16
to chri...@chromium.org, bo...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 26, 2016, 2:28:50 PM8/26/16
to chri...@chromium.org, bo...@chromium.org, p...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 26, 2016, 4:08:27 PM8/26/16
to chri...@chromium.org, bo...@chromium.org, p...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org
Try jobs failed on following builders:
linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/130890)

https://codereview.chromium.org/2271883002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 26, 2016, 4:12:07 PM8/26/16
to chri...@chromium.org, bo...@chromium.org, p...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 26, 2016, 5:57:10 PM8/26/16
to chri...@chromium.org, bo...@chromium.org, p...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org
Try jobs failed on following builders:
linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 26, 2016, 6:36:48 PM8/26/16
to chri...@chromium.org, bo...@chromium.org, p...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 26, 2016, 9:20:42 PM8/26/16
to chri...@chromium.org, bo...@chromium.org, p...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org
Try jobs failed on following builders:

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 27, 2016, 12:46:39 PM8/27/16
to chri...@chromium.org, bo...@chromium.org, p...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 27, 2016, 2:18:39 PM8/27/16
to chri...@chromium.org, bo...@chromium.org, p...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org
Try jobs failed on following builders:

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 27, 2016, 7:44:27 PM8/27/16
to chri...@chromium.org, bo...@chromium.org, p...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 27, 2016, 9:11:29 PM8/27/16
to chri...@chromium.org, bo...@chromium.org, p...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org
Try jobs failed on following builders:
linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 27, 2016, 10:07:25 PM8/27/16
to chri...@chromium.org, bo...@chromium.org, p...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 27, 2016, 11:21:36 PM8/27/16
to chri...@chromium.org, bo...@chromium.org, p...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org
Try jobs failed on following builders:
linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 29, 2016, 12:59:48 AM8/29/16
to chri...@chromium.org, bo...@chromium.org, p...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 29, 2016, 1:48:34 AM8/29/16
to chri...@chromium.org, bo...@chromium.org, p...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org
Try jobs failed on following builders:
linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 29, 2016, 10:44:32 AM8/29/16
to chri...@chromium.org, bo...@chromium.org, p...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 29, 2016, 11:37:48 AM8/29/16
to chri...@chromium.org, bo...@chromium.org, p...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org
Committed patchset #15 (id:280001)

https://codereview.chromium.org/2271883002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Aug 29, 2016, 11:42:33 AM8/29/16
to chri...@chromium.org, bo...@chromium.org, p...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org
Patchset 15 (id:??) landed as
https://crrev.com/b1d694f3370e4b6d6e723382c086617f85c94e93
Cr-Commit-Position: refs/heads/master@{#415013}

https://codereview.chromium.org/2271883002/

fla...@chromium.org

unread,
Aug 29, 2016, 1:00:20 PM8/29/16
to chri...@chromium.org, bo...@chromium.org, p...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org
A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/2286203004/ by fla...@chromium.org.

The reason for reverting is: Broke DisplayItemTest.DebugStringsExist on debug
probably due to adding a new display item type without updating
DisplayItem::typeAsDebugString.

https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Mac10.11%20(dbg)/builds/4251/steps/blink_platform_unittests/logs/DisplayItemTest.DebugStringsExist.

https://codereview.chromium.org/2271883002/
Reply all
Reply to author
Forward
0 new messages