Issue 303253004: Allow proper highlighting on universal overflow scroll.

Issue 303253004: Allow proper highlighting on universal overflow scroll. (Closed)

Created:
6 years, 6 months ago by hartmanng

Modified:
6 years, 6 months ago

Reviewers:
Ian Vollick, abarth-chromium, Julien - ping for review, jchaffraix+rendering, eseidel

CC:
Julien - ping for review, abarth-chromium, blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, leviw_travelin_and_unemployed, pdr., rune+blink, yosin_UTC9, zoltan1

Base URL:
svn://svn.chromium.org/blink/trunk

Visibility:
Public.

More Reviews

Description

Allow proper highlighting on universal overflow scroll. Universal accelerated overflow scroll doesn't paint highlight gaps between scroll children because the scrolling contents layer isn't always set up to draw content. BUG=381185, 381187 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176243

Patch Set 1 #

Patch Set 2 : #

Total comments: 8

Patch Set 3 : #

Patch Set 4 : #

Total comments: 4

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : rebaseline tests #

Total comments: 4

Patch Set 9 : review comments #

Patch Set 10 : style fix in test expectation #

Patch Set 11 : #

Patch Set 12 : add m_scrollingBlockSelectionLayer #

Patch Set 13 : rebase #

Patch Set 14 : TestExpectations #

Total comments: 15

Patch Set 15 : review comments #

Patch Set 16 : remove -expected.txt #

Total comments: 2

Patch Set 17 : nope #

Total comments: 8

Patch Set 18 : fixme #

Total comments: 2

Patch Set 19 : CompositingUpdateAfterCompositingInputChange #

Total comments: 4

Patch Set 20 : rebase #

Patch Set 21 : add commented lifecycle assert #

Created: 6 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+453 lines, -7 lines)			Patch
M	LayoutTests/TestExpectations	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	1 chunk	+43 lines, -0 lines	0 comments	Download
A	LayoutTests/compositing/overflow/selection-gaps.html	View	1 2 3 4 5 6 7 8	1 chunk	+44 lines, -0 lines	0 comments	Download
A	LayoutTests/compositing/overflow/selection-gaps-after-removing-scrolling-contents.html	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+79 lines, -0 lines	0 comments	Download
A	LayoutTests/compositing/overflow/selection-gaps-expected.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+45 lines, -0 lines	0 comments	Download
A	LayoutTests/compositing/overflow/selection-gaps-toggling.html	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+75 lines, -0 lines	0 comments	Download
A	LayoutTests/compositing/overflow/selection-gaps-toggling-with-scrolling-contents.html	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+78 lines, -0 lines	0 comments	Download
M	Source/core/page/scrolling/ScrollingCoordinator.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/RenderBlock.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	2 chunks	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/RenderLayer.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/rendering/RenderLayer.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	3 chunks	+27 lines, -1 line	0 comments	Download
M	Source/core/rendering/compositing/CompositedLayerMapping.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	5 chunks	+6 lines, -0 lines	0 comments	Download
M	Source/core/rendering/compositing/CompositedLayerMapping.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	11 chunks	+47 lines, -3 lines	0 comments	Download
M	Source/platform/graphics/CompositingReasons.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13	2 chunks	+5 lines, -1 line	0 comments	Download

Messages

Total messages: 30 (0 generated)

Expand Messages | Collapse Messages

hartmanng

Ian, please take a look. Unfortunately, the test is a pixel test. I tried to ...

6 years, 6 months ago (2014-05-30 20:48:26 UTC) #1

Ian Vollick

https://codereview.chromium.org/303253004/diff/10001/LayoutTests/TestExpectations File LayoutTests/TestExpectations (right): https://codereview.chromium.org/303253004/diff/10001/LayoutTests/TestExpectations#newcode186 LayoutTests/TestExpectations:186: Bug(hartmanng) virtual/gpu/compositedscrolling/overflow/selection-gaps.html [ NeedsRebaseline ] I wonder if we ...

6 years, 6 months ago (2014-05-31 01:17:35 UTC) #2

hartmanng

The layout test failures last time were expected, since we were drawing contents much more ...

6 years, 6 months ago (2014-06-02 20:23:58 UTC) #3

The layout test failures last time were expected, since we were drawing contents
much more than normal. The newer patch (patch set 4) is more conservative, I
don't expect those failures this time.

I'm planning on writing a few more test shortly to verify that we're
setting/unsetting drawsContents at the right times.

https://codereview.chromium.org/303253004/diff/10001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/303253004/diff/10001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:186: Bug(hartmanng)
virtual/gpu/compositedscrolling/overflow/selection-gaps.html [ NeedsRebaseline ]
On 2014/05/31 01:17:36, Ian Vollick wrote:
> I wonder if we could make this a ref test, and in the -expected.html, we could
> make the background opaque white rather than turning off accelerated overflow
> scrolling. That way we should have the same stuff being composited and we
> shouldn't (hopefully) run into the same AA-related pixel diffs.

I like it. Done.

https://codereview.chromium.org/303253004/diff/10001/LayoutTests/compositing/...
File LayoutTests/compositing/overflow/selection-gaps.html (right):

https://codereview.chromium.org/303253004/diff/10001/LayoutTests/compositing/...
LayoutTests/compositing/overflow/selection-gaps.html:2: <html>
On 2014/05/31 01:17:36, Ian Vollick wrote:
> You can skip the html, head and body.

Done.

https://codereview.chromium.org/303253004/diff/10001/LayoutTests/compositing/...
LayoutTests/compositing/overflow/selection-gaps.html:23: function doTest()
On 2014/05/31 01:17:36, Ian Vollick wrote:
> nit: I find this a little cuter.
> 
> onload = function() {
>   ...
> };

Done.

https://codereview.chromium.org/303253004/diff/10001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/303253004/diff/10001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1064:
m_scrollingContentsLayer->setDrawsContent(true);
On 2014/05/31 01:17:36, Ian Vollick wrote:
> This might be the only reasonable way to do this, but it is a pretty big
hammer;
> the scrolling contents layer can be enormous. I'd really like to be convinced
> that other approaches won't work before we go with this. Perhaps we could
store
> a bool on the CLM -- m_scrollingContentsAreEmpty, say -- and when we add/clear
> block selection gaps bounds in RenderLayer, we could setDrawsContent true or
> false if m_scrollingContentsAreEmpty? It would also be nice to call
> setContentsRect on the scrollingContentsLayer in this case so that cc knows
that
> the layer only has content in the selection rect.

Done. However, in my patch I do setContentsRect(IntRect()), and the block
selection still appears. Maybe I'm not understanding setContentsRect()? (Or
calling it too late in the frame?)

Ian Vollick

https://codereview.chromium.org/303253004/diff/40001/Source/core/rendering/RenderLayer.cpp File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/303253004/diff/40001/Source/core/rendering/RenderLayer.cpp#newcode3207 Source/core/rendering/RenderLayer.cpp:3207: return static_cast<IntRect>(gapRects); Please be explicit about this conversion. Eg, ...

6 years, 6 months ago (2014-06-03 17:11:46 UTC) #4

hartmanng

https://codereview.chromium.org/303253004/diff/40001/Source/core/rendering/compositing/CompositedLayerMapping.cpp File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/303253004/diff/40001/Source/core/rendering/compositing/CompositedLayerMapping.cpp#newcode1066 Source/core/rendering/compositing/CompositedLayerMapping.cpp:1066: m_scrollingContentsLayer->setContentsRect(IntRect()); On 2014/06/03 17:11:47, Ian Vollick wrote: > It's ...

6 years, 6 months ago (2014-06-04 15:32:32 UTC) #5

Ian Vollick

On 2014/06/04 15:32:32, hartmanng wrote: > https://codereview.chromium.org/303253004/diff/40001/Source/core/rendering/compositing/CompositedLayerMapping.cpp > File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right): > > https://codereview.chromium.org/303253004/diff/40001/Source/core/rendering/compositing/CompositedLayerMapping.cpp#newcode1066 > ...

6 years, 6 months ago (2014-06-04 15:35:52 UTC) #6

hartmanng

I got the geometry mostly working in patch set 5, but unfortunately it breaks hit ...

6 years, 6 months ago (2014-06-05 15:59:37 UTC) #7

hartmanng

My tests were failing on non-linux bots due to minor sizing/positioning issues unrelated to the ...

6 years, 6 months ago (2014-06-05 16:51:09 UTC) #8

Ian Vollick

https://codereview.chromium.org/303253004/diff/120001/LayoutTests/compositing/overflow/selection-gaps-after-removing-scrolling-contents.html File LayoutTests/compositing/overflow/selection-gaps-after-removing-scrolling-contents.html (right): https://codereview.chromium.org/303253004/diff/120001/LayoutTests/compositing/overflow/selection-gaps-after-removing-scrolling-contents.html#newcode38 LayoutTests/compositing/overflow/selection-gaps-after-removing-scrolling-contents.html:38: onload = function doTest() nit: I don't think this ...

6 years, 6 months ago (2014-06-05 18:28:42 UTC) #9

hartmanng

6 years, 6 months ago (2014-06-05 20:28:03 UTC) #10

hartmanng

On 2014/06/05 20:28:03, hartmanng wrote: > https://codereview.chromium.org/303253004/diff/120001/Source/core/rendering/RenderLayer.h#newcode185 > Source/core/rendering/RenderLayer.h:185: PresenceOfBlockSelectionGaps > rendererHasBlockSelectionGaps(); > On 2014/06/05 ...

6 years, 6 months ago (2014-06-09 14:18:48 UTC) #11

Ian Vollick

On 2014/06/09 14:18:48, hartmanng wrote: > On 2014/06/05 20:28:03, hartmanng wrote: > > > https://codereview.chromium.org/303253004/diff/120001/Source/core/rendering/RenderLayer.h#newcode185 ...

6 years, 6 months ago (2014-06-09 14:20:47 UTC) #12

hartmanng

PTAL I've updated this patch so that we add a new (small) layer instead of ...

6 years, 6 months ago (2014-06-12 14:17:11 UTC) #13

Ian Vollick

It's so great that you were able to get the tiny layer for the selection! ...

6 years, 6 months ago (2014-06-12 14:54:57 UTC) #14

abarth-chromium

https://codereview.chromium.org/303253004/diff/240001/Source/core/rendering/compositing/GraphicsLayerTreeBuilder.cpp File Source/core/rendering/compositing/GraphicsLayerTreeBuilder.cpp (right): https://codereview.chromium.org/303253004/diff/240001/Source/core/rendering/compositing/GraphicsLayerTreeBuilder.cpp#newcode89 Source/core/rendering/compositing/GraphicsLayerTreeBuilder.cpp:89: childList.append(currentCompositedLayerMapping->scrollingBlockSelectionLayer()); Note: If you have this logic here, you ...

6 years, 6 months ago (2014-06-13 04:12:02 UTC) #15

abarth-chromium

https://codereview.chromium.org/303253004/diff/240001/Source/core/rendering/RenderLayer.cpp File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/303253004/diff/240001/Source/core/rendering/RenderLayer.cpp#newcode3186 Source/core/rendering/RenderLayer.cpp:3186: m_compositedLayerMapping->updateScrollingBlockSelection(); Yeah, you should be able to just: setNeedsCompositingInputsUpdate(); ...

6 years, 6 months ago (2014-06-13 04:31:05 UTC) #16

hartmanng

https://codereview.chromium.org/303253004/diff/240001/Source/core/rendering/RenderLayer.cpp File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/303253004/diff/240001/Source/core/rendering/RenderLayer.cpp#newcode3185 Source/core/rendering/RenderLayer.cpp:3185: if (needsCompositedScrolling() && hasCompositedLayerMapping()) On 2014/06/12 14:54:57, Ian Vollick ...

6 years, 6 months ago (2014-06-13 14:51:38 UTC) #17

https://codereview.chromium.org/303253004/diff/240001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/303253004/diff/240001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:3185: if (needsCompositedScrolling() &&
hasCompositedLayerMapping())
On 2014/06/12 14:54:57, Ian Vollick wrote:
> This looks like a chicken-and-egg thing, but I think it might be avoidable.
Can
> we schedule a compositing update of the composited-scroll variety and ensure
> that the scrolling block selection stuff is freshened during those updates?

Done.

https://codereview.chromium.org/303253004/diff/240001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:3186:
m_compositedLayerMapping->updateScrollingBlockSelection();
On 2014/06/13 04:31:05, abarth wrote:
> Yeah, you should be able to just:
> 
> setNeedsCompositingInputsUpdate();
> compositor()->setNeedsCompositingUpdate(CompositingUpdateRebuildTree);

Would
compositor()->setNeedsCompositingUpdate(CompositingUpdateOnCompositedScroll);
work now that I don't need to mess around with GraphicsLayerTreeBuilder?

https://codereview.chromium.org/303253004/diff/240001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/303253004/diff/240001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.h:183: };
On 2014/06/12 14:54:57, Ian Vollick wrote:
> I think you could just use a boolean and get rid of this enum. The enum would
be
> better if this were being passed into a function as a parameter, but a boolean
> return value is fine, especially when the function is as clearly named as
> hasBlockSelectionGapBounds is.

Done.

https://codereview.chromium.org/303253004/diff/240001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/303253004/diff/240001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1028:
m_scrollingBlockSelectionLayer->setDrawsContent(false);
On 2014/06/12 14:54:57, Ian Vollick wrote:
> A comment explaining that the selection will be painted into
> m_scrollingContentsLayer would be nice here.

Done.

https://codereview.chromium.org/303253004/diff/240001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1057: bool
hasScrollingPaintedContent = m_owningLayer.hasVisibleContent() &&
(renderer()->hasBackground() || paintsChildren());
On 2014/06/12 14:54:57, Ian Vollick wrote:
> I don't think you need the temporary if you're going to stuff this value in
> m_scrollingContentsAreEmpty

Done.

https://codereview.chromium.org/303253004/diff/240001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1445:
m_scrollingBlockSelectionLayer =
createGraphicsLayer(CompositingReasonLayerForScrollingBlockSelection);
On 2014/06/12 14:54:57, Ian Vollick wrote:
> Is it possible to create this only when we have a selection?

I'm not really sure. I tried for a while, but couldn't find a good way. IIUC,
we'd have to trigger one of the heavier compositing updates for every change in
selection in order to get into the graphics layer configuration update code?
(ie, we would have to dirty m_needToUpdateGraphicsLayer, which would trigger a
lot of work through updateGraphicsLayerConfiguration and
updateInternalHierarchy).

https://codereview.chromium.org/303253004/diff/240001/Source/core/rendering/c...
File Source/core/rendering/compositing/GraphicsLayerTreeBuilder.cpp (right):

https://codereview.chromium.org/303253004/diff/240001/Source/core/rendering/c...
Source/core/rendering/compositing/GraphicsLayerTreeBuilder.cpp:89:
childList.append(currentCompositedLayerMapping->scrollingBlockSelectionLayer());
On 2014/06/13 04:12:02, abarth wrote:
> Note: If you have this logic here, you need to schedule a
> CompositingUpdateRebuildTree to make sure we actually rebuild the tree.

Turns out we can get away without this part by fixing up CLM a bit. I think it's
a better solution anyway, so I don't need to touch GraphcisLayerTreeBuilder
anymore.

https://codereview.chromium.org/303253004/diff/280001/Source/core/rendering/R...
File Source/core/rendering/RenderLayerScrollableArea.cpp (left):

https://codereview.chromium.org/303253004/diff/280001/Source/core/rendering/R...
Source/core/rendering/RenderLayerScrollableArea.cpp:402: &&
!layer()->hasBlockSelectionGapBounds()
This is no longer needed, because the block selection gaps bounds will now never
have to repaint on scroll. They will either be part of the scrolling contents
layer, or part of the block selection layer, but never part of the background.

This removal should also fix the scroll-update chicken-egg problems, so this
patch shouldn't be blocked anymore.

hartmanng

https://codereview.chromium.org/303253004/diff/280001/Source/core/rendering/RenderLayerScrollableArea.cpp File Source/core/rendering/RenderLayerScrollableArea.cpp (left): https://codereview.chromium.org/303253004/diff/280001/Source/core/rendering/RenderLayerScrollableArea.cpp#oldcode402 Source/core/rendering/RenderLayerScrollableArea.cpp:402: && !layer()->hasBlockSelectionGapBounds() On 2014/06/13 14:51:38, hartmanng wrote: > This ...

6 years, 6 months ago (2014-06-13 16:43:56 UTC) #18

Ian Vollick

Compositing stuff lgtm, but you'll need approval for the RenderBlock change and the RenderLayer change ...

6 years, 6 months ago (2014-06-13 17:07:50 UTC) #19

hartmanng

Thanks! abarth, PTAL. Do you know enough about RenderBlock to be comfortable reviewing the changes ...

6 years, 6 months ago (2014-06-13 17:55:13 UTC) #20

abarth-chromium

https://codereview.chromium.org/303253004/diff/320001/Source/core/rendering/RenderLayer.cpp File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/303253004/diff/320001/Source/core/rendering/RenderLayer.cpp#newcode3152 Source/core/rendering/RenderLayer.cpp:3152: compositor()->setNeedsCompositingUpdate(CompositingUpdateOnCompositedScroll); This compositing update type isn't sufficient. You need ...

6 years, 6 months ago (2014-06-13 18:17:50 UTC) #21

hartmanng

https://codereview.chromium.org/303253004/diff/320001/Source/core/rendering/RenderLayer.cpp File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/303253004/diff/320001/Source/core/rendering/RenderLayer.cpp#newcode3152 Source/core/rendering/RenderLayer.cpp:3152: compositor()->setNeedsCompositingUpdate(CompositingUpdateOnCompositedScroll); On 2014/06/13 18:17:50, abarth wrote: > This compositing ...

6 years, 6 months ago (2014-06-13 18:42:59 UTC) #22

abarth-chromium

https://codereview.chromium.org/303253004/diff/340001/Source/core/rendering/RenderLayer.cpp File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/303253004/diff/340001/Source/core/rendering/RenderLayer.cpp#newcode3209 Source/core/rendering/RenderLayer.cpp:3209: return toRenderBlock(renderer())->shouldPaintSelectionGaps(); LGTM, but I'm not 100% sure about ...

6 years, 6 months ago (2014-06-13 21:55:54 UTC) #23

eseidel

Levi or Julien are the correct "selection gaps" reviewers.

6 years, 6 months ago (2014-06-16 15:58:29 UTC) #24

Ian Vollick

On 2014/06/16 15:58:29, eseidel (OOO until June 16) wrote: > Levi or Julien are the ...

6 years, 6 months ago (2014-06-16 18:13:49 UTC) #25

Julien - ping for review

(sad) lgtm for the selection gaps change https://codereview.chromium.org/303253004/diff/340001/Source/core/rendering/RenderLayer.cpp File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/303253004/diff/340001/Source/core/rendering/RenderLayer.cpp#newcode3204 Source/core/rendering/RenderLayer.cpp:3204: // at ...

6 years, 6 months ago (2014-06-16 18:30:24 UTC) #26

hartmanng

Thanks everyone! https://codereview.chromium.org/303253004/diff/340001/Source/core/rendering/RenderLayer.cpp File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/303253004/diff/340001/Source/core/rendering/RenderLayer.cpp#newcode3204 Source/core/rendering/RenderLayer.cpp:3204: // at the moment because it causes ...

6 years, 6 months ago (2014-06-16 19:19:14 UTC) #27

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hartmanng@chromium.org/303253004/380001

6 years, 6 months ago (2014-06-16 19:20:14 UTC) #29

Message was sent while issue was closed.

Change committed as 176243

Expand Messages | Collapse Messages