Fix position of floating layer whose containing block is also floating

Fix position of layer with floating ancestor within inline parent layer

<span style="position: relative">
  ...
    <div style="float: left">
      ...
        <target layer>
      ...
    </div>
  ...
</span>

Because of the floating ancestor, the containing layer of target layer
is not the span, but the layer found in the containing block chain.

BUG=703666, 703481
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2761413002
Cr-Commit-Position: refs/heads/master@{#458857}
Committed: https://chromium.googlesource.com/chromium/src/+/be8909e4d0dfe024e1b39f5abf195e63d97652ae

[Xianzhu, 2017-03-21 21:10:01]

Description was changed from

==========
Fix position of floating layer whose containing block is also floating

BUG=703666
==========

to

==========
Fix position of floating layer whose containing block is also floating

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

[Xianzhu, 2017-03-21 21:10:11]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[Xianzhu, 2017-03-21 21:10:26]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org

[commit-bot: I haz the power, 2017-03-21 21:10:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chrishtr, 2017-03-21 21:48:46]

https://codereview.chromium.org/2761413002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (left):

https://codereview.chromium.org/2761413002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:874:
layoutObject.isFloatingWithNonContainingBlockParent()) {
Do the other callsites of isFloatingWithNonContainingBlockParent() need some
adjustment?
Do we invalidate correctly for nested floats?

[commit-bot: I haz the power, 2017-03-21 22:20:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 22:20:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xianzhu, 2017-03-22 04:41:57]

Description was changed from

==========
Fix position of floating layer whose containing block is also floating

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

to

==========
Fix position of layer with floating ancestor within inline parent layer

<span style="position: relative">
  ...
    <div style="float: left">
      ...
        <target layer>
      ...
    </div>
  ...
</span>

Because of the floating ancestor, the containing layer of target layer
is not the span, but the layer found in the containing block chain.

BUG=703666,703481

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

[Xianzhu, 2017-03-22 15:02:00]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 15:02:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2017-03-22 15:03:15]

https://codereview.chromium.org/2761413002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (left):

https://codereview.chromium.org/2761413002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:874:
layoutObject.isFloatingWithNonContainingBlockParent()) {
On 2017/03/21 21:48:46, chrishtr wrote:
> Do the other callsites of isFloatingWithNonContainingBlockParent() need some
> adjustment?

The function is still good for checking if a floating object's enclosing layer
is in the containing block chain.

Patch Set 2 is a more general approach fixing the more general bug
(crbug.com/703481). Actually whether the layer itself is floating is not the
key. The problem is that there might be non-layer floating object ancestor
changing the containing layer order of a layer.

When the parent layer is an inline, we should traverse the containing block
chain to find the containing layer to ensure correctness if there is any
floating object. We don't need to know if there is any floating object because
the traversal is actually a universal slow path suitable for any layer to find
the containing layer.

> Do we invalidate correctly for nested floats?

Invalidation is good. containingLayer affects painting only.

[chrishtr, 2017-03-22 17:20:15]

It looks to me like the core issue is that
LayoutObject::isFloatingWithNonContainingBlockParent
is answering the question regarding float-under-inline for the *LayoutObject*
tree, but PaintLayer::containingLayer wants to ask it for the *PaintLayer* tree.
For example, in the testcase you added at
PaintLayerTest.FloatLayerUnderFloatUnderInlineLayer,
floating->layoutObject().isFloatingWithNonContainingBlockParent() returns false.

If my analysis is correct, a better fix would be to add a method
PaintLayer::isFloatingWithNonContainingBlockParent and call it from the cases
where the answer
required is a PaintLayer. e.g. these call sites I think need adjustment:

PaintLayer::containingLayer
PaintInvalidator::updatePaintingLayer (updates paint invalidation container,
which is always
a PaintLayer)
PaintInvalidationState::PaintInvalidationState (ditto)
GraphicsLayerUpdater::compositingContainer (composited PaintLayer)


It appears that canMapBetweenLayoutObjects in LayoutGeometryMapper can keep the
current
call.

[Xianzhu, 2017-03-22 17:29:51]

On 2017/03/22 17:20:15, chrishtr wrote:
> It looks to me like the core issue is that
> LayoutObject::isFloatingWithNonContainingBlockParent
> is answering the question regarding float-under-inline for the *LayoutObject*
> tree, but PaintLayer::containingLayer wants to ask it for the *PaintLayer*
tree.
> For example, in the testcase you added at
> PaintLayerTest.FloatLayerUnderFloatUnderInlineLayer,
> floating->layoutObject().isFloatingWithNonContainingBlockParent() returns
false.
> 
> If my analysis is correct, a better fix would be to add a method
> PaintLayer::isFloatingWithNonContainingBlockParent and call it from the cases
> where the answer
> required is a PaintLayer. e.g. these call sites I think need adjustment:
> 
> PaintLayer::containingLayer
> PaintInvalidator::updatePaintingLayer (updates paint invalidation container,
> which is always
> a PaintLayer)
> PaintInvalidationState::PaintInvalidationState (ditto)
> GraphicsLayerUpdater::compositingContainer (composited PaintLayer)
> 

The problem is not a floating layer having inline parent, but a non-layer
floating object affects containing layer of a layer whose parent layer is
inline:

<span style="position: relative"> <!-- inline layer -->
  ...
    <div style="float: left">  <!-- not a layer -->
      ...
        <target layer>   <!-- a child layer whose parent layer is the inline
layer, but containing layer is not the inline layer -->
      ...
    </div>
  ...
</span>

The places calling isFloatingWithNonContainingBlockParent are still valid
because they are checking if an object (whether or not having layer) should use
special path. This also applies to the cases in PaintInvalidationState and
PaintInvalidator.

> 
> It appears that canMapBetweenLayoutObjects in LayoutGeometryMapper can keep
the
> current
> call.

[Xianzhu, 2017-03-22 17:34:46]

On 2017/03/22 17:29:51, Xianzhu wrote:
> On 2017/03/22 17:20:15, chrishtr wrote:
> > It looks to me like the core issue is that
> > LayoutObject::isFloatingWithNonContainingBlockParent
> > is answering the question regarding float-under-inline for the
*LayoutObject*
> > tree, but PaintLayer::containingLayer wants to ask it for the *PaintLayer*
> tree.
> > For example, in the testcase you added at
> > PaintLayerTest.FloatLayerUnderFloatUnderInlineLayer,
> > floating->layoutObject().isFloatingWithNonContainingBlockParent() returns
> false.
> > 
> > If my analysis is correct, a better fix would be to add a method
> > PaintLayer::isFloatingWithNonContainingBlockParent and call it from the
cases
> > where the answer
> > required is a PaintLayer. e.g. these call sites I think need adjustment:
> > 
> > PaintLayer::containingLayer
> > PaintInvalidator::updatePaintingLayer (updates paint invalidation container,
> > which is always
> > a PaintLayer)
> > PaintInvalidationState::PaintInvalidationState (ditto)
> > GraphicsLayerUpdater::compositingContainer (composited PaintLayer)
> > 
> 
> The problem is not a floating layer having inline parent, but a non-layer
> floating object affects containing layer of a layer whose parent layer is
> inline:

Clarification: floating layer having inline parent was a problem that the
original CL fixed. The original CL missed the case of non-layer floating objects
having inline enclosing layers that this CL addresses. This new code in this CL
also handles the case of floating layer.

> 
> <span style="position: relative"> <!-- inline layer -->
>   ...
>     <div style="float: left">  <!-- not a layer -->
>       ...
>         <target layer>   <!-- a child layer whose parent layer is the inline
> layer, but containing layer is not the inline layer -->
>       ...
>     </div>
>   ...
> </span>
> 
> The places calling isFloatingWithNonContainingBlockParent are still valid
> because they are checking if an object (whether or not having layer) should
use
> special path. This also applies to the cases in PaintInvalidationState and
> PaintInvalidator.
> 
> > 
> > It appears that canMapBetweenLayoutObjects in LayoutGeometryMapper can keep
> the
> > current
> > call.

[chrishtr, 2017-03-22 18:13:04]

This CL looks good.

I'm not sure why patch set 4 is better than patch set 3 for merging though. 3 is
clearly
preferrable for ToT.

Also, per offline discussion, could you add a TODO to GraphicsLayerUpdater?

[chrishtr, 2017-03-22 18:26:54]

lgtm

LGTM with TODO added. In offline discussion, we agreed the risk of patch set 3
and 4 is
about the same, but 4 may be easier to merge, so I am agnostic about which is
better, and defer
to Xianzhu's judgement.

[Xianzhu, 2017-03-22 18:30:04]

On 2017/03/22 18:13:04, chrishtr wrote:
> 
> Also, per offline discussion, could you add a TODO to GraphicsLayerUpdater?

Done.

[Xianzhu, 2017-03-22 18:30:14]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-03-22 18:30:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2761413002/#ps80001
(title: "TODO in GraphicsLayerUpdater")

[commit-bot: I haz the power, 2017-03-22 18:30:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 20:17:52]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490207414191420,
"parent_rev": "5cbb188b9d0ad7851eb50ca807c6bb37e02128e6", "commit_rev":
"be8909e4d0dfe024e1b39f5abf195e63d97652ae"}

[commit-bot: I haz the power, 2017-03-22 20:18:40]

Description was changed from

==========
Fix position of layer with floating ancestor within inline parent layer

<span style="position: relative">
  ...
    <div style="float: left">
      ...
        <target layer>
      ...
    </div>
  ...
</span>

Because of the floating ancestor, the containing layer of target layer
is not the span, but the layer found in the containing block chain.

BUG=703666,703481

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

to

==========
Fix position of layer with floating ancestor within inline parent layer

<span style="position: relative">
  ...
    <div style="float: left">
      ...
        <target layer>
      ...
    </div>
  ...
</span>

Because of the floating ancestor, the containing layer of target layer
is not the span, but the layer found in the containing block chain.

BUG=703666,703481

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2761413002
Cr-Commit-Position: refs/heads/master@{#458857}
Committed:
https://chromium.googlesource.com/chromium/src/+/be8909e4d0dfe024e1b39f5abf19...
==========

[commit-bot: I haz the power, 2017-03-22 20:18:41]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/be8909e4d0dfe024e1b39f5abf19...