Clip abspos descendants correctly in all columns (not just the first).

Clip abspos descendants correctly in all columns (not just the first).

Cannot use localToContainerPoint() for multicol content, since the flowthread
hasn't yet been sliced and translated into columns. Use convertToLayerCoords()
instead. Ideally, I believe everyone should do that, but that's currently not
possible without introducing regressions.

BUG=527709
R=chrishtr@chromium.org

[mstensho (USE GERRIT), 2015-11-19 17:00:14]

ve been hoping to find the time to figure out how to solve this properly for a
long time, but now this bug has so many duplicates, that I don't think we should
wait any longer. So here's a quick fix.

Some time ago, I tried to change from using localToContainerPoint() to using
convertToLayerCoords() for everyone, but that causes regressions:
https://codereview.chromium.org/1322653002/

An alternative solution would be to introduce a flag to localToContainerPoint(),
to tell it to NOT translate from flowtread coordinates to visual coordinates,
but that sounds like even more of a hack than what I'm doing here.

I think this needs attention from a layer and compositing expert in order to be
fixed cleanly and properly.

[mstensho (USE GERRIT), 2015-11-19 18:21:10]

I've been hoping to find the time to figure out how to solve this properly for a
long time, but now this bug has so many duplicates, that I don't think we should
wait any longer. So here's a quick fix.

Some time ago, I tried to change from using localToContainerPoint() to using
convertToLayerCoords() for everyone, but that causes regressions:
https://codereview.chromium.org/1322653002/

An alternative solution would be to introduce a flag to localToContainerPoint(),
to tell it to NOT translate from flowtread coordinates to visual coordinates,
but that sounds like even more of a hack than what I'm doing here.

I think this needs attention from a layer and compositing expert in order to be
fixed cleanly and properly.

[chrishtr, 2015-11-19 23:31:41]

https://codereview.chromium.org/1459943002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/multicol/abspos-in-clipped-relpos-in-second-column-expected.html
(right):

https://codereview.chromium.org/1459943002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/multicol/abspos-in-clipped-relpos-in-second-column-expected.html:2:
<p>There should be a green square below, and no red.</p>
Why should there be a green square? both squares should be clipped to 100px..

https://codereview.chromium.org/1459943002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right):

https://codereview.chromium.org/1459943002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:278: //
TODO(mstensho): Switch to always using convertToLayerCoords() (see code further
localToContainerPoint is actually incorrect here, it should be
localToContainerQuad :) see crbug.com/548184 if you are curious. But
localToContainerQuad makes even less sense for fragmentation, because the result
would have to be multiple quads.

Is the problem that you want to compute clip rects up to the boundary of the
flowthread->visual coordinates conversion? The rootLayer in your use case is the
pagination container, right?

[mstensho (USE GERRIT), 2015-11-20 10:37:15]

https://codereview.chromium.org/1459943002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/multicol/abspos-in-clipped-relpos-in-second-column-expected.html
(right):

https://codereview.chromium.org/1459943002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/multicol/abspos-in-clipped-relpos-in-second-column-expected.html:2:
<p>There should be a green square below, and no red.</p>
On 2015/11/19 23:31:41, chrishtr wrote:
> Why should there be a green square? both squares should be clipped to 100px..

Moving discussion to the test file. This is the ref.

https://codereview.chromium.org/1459943002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/multicol/abspos-in-clipped-relpos-in-second-column.html
(right):

https://codereview.chromium.org/1459943002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/multicol/abspos-in-clipped-relpos-in-second-column.html:2:
<p>There should be a green square below, and no red.</p>
chrishtr wrote (in abspos-in-clipped-relpos-in-second-column-expected.html):

> Why should there be a green square? both squares should be clipped to 100px..

The 100x100 green square is absolutely positioned, and painted on top of the
100x100 red square.

https://codereview.chromium.org/1459943002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right):

https://codereview.chromium.org/1459943002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:278: //
TODO(mstensho): Switch to always using convertToLayerCoords() (see code further
On 2015/11/19 23:31:41, chrishtr wrote:
> localToContainerPoint is actually incorrect here, it should be
> localToContainerQuad :) see crbug.com/548184 if you are curious.

Makes sense.

> But
> localToContainerQuad makes even less sense for fragmentation, because the
result
> would have to be multiple quads.

Exactly.

> Is the problem that you want to compute clip rects up to the boundary of the
> flowthread->visual coordinates conversion? The rootLayer in your use case is
the
> pagination container, right?

The rootLayer could be anything (if there are no transforms, it will typically
be that of LayoutView). There's normally nothing special when it comes to
setting rootLayer for multicol, unless we have a transform inside a multicol
container.

Clip rects are calculated before we slice and translate the flow thread into
columns, which is why we don't want to be multicol-aware here.

See PaintLayer::collectFragments(). We call clipper().calculateRects(). Then we
intersect layerBoundingBoxInFlowThread with the background clip rectangle. Then
we pass this to enclosingFlowThread->collectLayerFragments(), which will slice
the flow thread into fragments.

[mstensho (USE GERRIT), 2015-12-04 14:15:37]

ping

[mstensho (USE GERRIT), 2015-12-08 16:25:37]

@chrishtr / anyone: ping again. People keep asking for a fix for this.

[chrishtr, 2015-12-08 16:36:38]

On 2015/12/08 at 16:25:37, mstensho wrote:
> @chrishtr / anyone: ping again. People keep asking for a fix for this.

Sorry for the delay, will review again today.

[chrishtr, 2015-12-08 22:56:07]

https://codereview.chromium.org/1459943002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right):

https://codereview.chromium.org/1459943002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:278: //
TODO(mstensho): Switch to always using convertToLayerCoords() (see code further
On 2015/11/20 at 10:37:15, mstensho wrote:
> On 2015/11/19 23:31:41, chrishtr wrote:
> > localToContainerPoint is actually incorrect here, it should be
> > localToContainerQuad :) see crbug.com/548184 if you are curious.
> 
> Makes sense.
> 
> > But
> > localToContainerQuad makes even less sense for fragmentation, because the
result
> > would have to be multiple quads.
> 
> Exactly.
> 
> > Is the problem that you want to compute clip rects up to the boundary of the
> > flowthread->visual coordinates conversion? The rootLayer in your use case is
the
> > pagination container, right?
> 
> The rootLayer could be anything (if there are no transforms, it will typically
be that of LayoutView). There's normally nothing special when it comes to
setting rootLayer for multicol, unless we have a transform inside a multicol
container.
> 
> Clip rects are calculated before we slice and translate the flow thread into
columns, which is why we don't want to be multicol-aware here.
> 
> See PaintLayer::collectFragments(). We call clipper().calculateRects(). Then
we intersect layerBoundingBoxInFlowThread with the background clip rectangle.
Then we pass this to enclosingFlowThread->collectLayerFragments(), which will
slice the flow thread into fragments.


The point I was getting at with asking what the rootLayer is, is this: it
doesn't really make sense to compute pass clip rects across pagination
boundaries. Instead, why not change the code to compute clip rects internally to
the pagination layer, and not cross its boundary? That way the clip rects have a
well-defined meaning.

Any clip rects which come in from outside the pagination container should I
think be dealt with post-fragmentation.

[mstensho (USE GERRIT), 2015-12-09 16:13:44]

@chrishtr - please take a look. New patch. Not sure if I want to land the code
changes in core/layout/ like that, but I'm interested in knowing what you think
about the change in core/paint/

[chrishtr, 2015-12-14 21:39:51]

https://codereview.chromium.org/1459943002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1459943002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:955: // up. But
when walking downwards (like here), we don't want that. So this isn't really a
Why is it generally not desired to adjust when walking downwards? Isn't it
well-defined
to ask that question?

Also, this fix is independent of the PaintLayerClipper one right?

https://codereview.chromium.org/1459943002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right):

https://codereview.chromium.org/1459943002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:276: layerToStopAt =
paginationLayer;
Why not just require users of PaintLayerClipper to not ask questions across
pagination boundaries?

[mstensho (USE GERRIT), 2016-01-05 08:55:25]

https://codereview.chromium.org/1459943002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1459943002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:955: // up. But
when walking downwards (like here), we don't want that. So this isn't really a
On 2015/12/14 21:39:51, chrishtr wrote:
> Why is it generally not desired to adjust when walking downwards? Isn't it
> well-defined
> to ask that question?

When walking downwards into a multicol container, you have visual coordinates
and ENTER a flow thread coordinate space. When walking upwards out of a multicol
container, you have flow thread coordinates and LEAVE a flow thread coordinate
space.
 
> Also, this fix is independent of the PaintLayerClipper one right?

Well, this makes the code behave to PaintLayerClipper's liking. :)

But yeah, better clean up this code in separate CLs first. A new quest has begun
in https://codereview.chromium.org/1516003003/ . When everything is cleaned up,
it should be more easy to figure out how to fix this bug cleanly.