Fix paint and rect mapping issues for stacked float under stacked inline

Fix paint and rect mapping issues for stacked float under stacked inline

Previously we skipped all inline ancestors until the containing block
for floating objects. This was wrong for stacked float with stacked
inline containers.

TEST=paint/invalidation/compositing/stacked-float-under-composited-inline.html
TEST=*.*FloatUnder*Inline
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

[Xianzhu, 2017-01-20 05:47:21]

Description was changed from

==========
Fix paint and rect mapping issues for stacked float under stacked inline

Previously we skipped all inline ancestors until the containing block
for floating objects. This was wrong for stacked float with stacked
inline containers.

TEST=paint/invalidation/compositing/stacked-float-under-composited-inline.html
TEST=*.*FloatUnder*Inline
==========

to

==========
Fix paint and rect mapping issues for stacked float under stacked inline

Previously we skipped all inline ancestors until the containing block
for floating objects. This was wrong for stacked float with stacked
inline containers.

TEST=paint/invalidation/compositing/stacked-float-under-composited-inline.html
TEST=*.*FloatUnder*Inline
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2017-01-20 05:47:28]

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

[commit-bot: I haz the power, 2017-01-20 05:47:40]

Dry run: 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-01-20 07:39:10]

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

[commit-bot: I haz the power, 2017-01-20 07:39:10]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2017-01-20 17:25:21]

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

[chrishtr, 2017-01-21 02:15:00]

https://codereview.chromium.org/2642763009/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2642763009/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2493: }
ASSERT_NOT_REACHED()

https://codereview.chromium.org/2642763009/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp (right):

https://codereview.chromium.org/2642763009/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:241:
EXPECT_EQ(container, floating->containingBlock());
Can you explain to me why containingBlock disagrees with
container here?

[Xianzhu, 2017-01-21 20:18:27]

On 2017/01/21 02:15:00, chrishtr wrote:
>
https://codereview.chromium.org/2642763009/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):
> 
>
https://codereview.chromium.org/2642763009/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutObject.cpp:2493: }
> ASSERT_NOT_REACHED()
> 
>
https://codereview.chromium.org/2642763009/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp (right):
> 
>
https://codereview.chromium.org/2642763009/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:241:
> EXPECT_EQ(container, floating->containingBlock());
> Can you explain to me why containingBlock disagrees with
> container here?

At first I thought the answer was obvious, but after I read the spec and tested
on FireFox, I found that this is a great question, and the basic idea of this CL
seems totally wrong.

This is the issue that this CL was to 'fix':

  <span style="position: relative; top: 50px; left: 50px">
    <div style="float: left; position: relative">Float</div>
  </span>

Now we paint the float with 50,50 offset. Before
https://codereview.chromium.org/2575423003/, we also apply 50,50 offset to the
float during geometry mapping. After
https://codereview.chromium.org/2575423003/, we invalidate paint and paint in
different ways. According to how we are painting, I thought the span establishes
"containing block" (our "container") for the float.

However, according to the spec, the relative-positioned span should not affect
positioning of the float. FireFox conforms to the spec.

Now I think the current LayoutObject::container() is correct to return
containingBlock for floats. We should fix painting code instead. We should undo
the offset of the painting layer containing the float if the painting layer's
layout object is not the containing block of the float.

[chrishtr, 2017-01-23 22:37:45]

On 2017/01/21 at 20:18:27, wangxianzhu wrote:
> On 2017/01/21 02:15:00, chrishtr wrote:
> >
https://codereview.chromium.org/2642763009/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):
> > 
> >
https://codereview.chromium.org/2642763009/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/layout/LayoutObject.cpp:2493: }
> > ASSERT_NOT_REACHED()
> > 
> >
https://codereview.chromium.org/2642763009/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp (right):
> > 
> >
https://codereview.chromium.org/2642763009/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:241:
> > EXPECT_EQ(container, floating->containingBlock());
> > Can you explain to me why containingBlock disagrees with
> > container here?
> 
> At first I thought the answer was obvious, but after I read the spec and
tested on FireFox, I found that this is a great question, and the basic idea of
this CL seems totally wrong.
> 
> This is the issue that this CL was to 'fix':
> 
>   <span style="position: relative; top: 50px; left: 50px">
>     <div style="float: left; position: relative">Float</div>
>   </span>
> 
> Now we paint the float with 50,50 offset. Before
https://codereview.chromium.org/2575423003/, we also apply 50,50 offset to the
float during geometry mapping. After
https://codereview.chromium.org/2575423003/, we invalidate paint and paint in
different ways. According to how we are painting, I thought the span establishes
"containing block" (our "container") for the float.
> 
> However, according to the spec, the relative-positioned span should not affect
positioning of the float. FireFox conforms to the spec.
> 
> Now I think the current LayoutObject::container() is correct to return
containingBlock for floats. We should fix painting code instead. We should undo
the offset of the painting layer containing the float if the painting layer's
layout object is not the containing block of the float.

Ok. Discarding this CL then right?

[Xianzhu, 2017-01-23 22:46:33]

On 2017/01/23 22:37:45, chrishtr wrote:
> On 2017/01/21 at 20:18:27, wangxianzhu wrote:
> > On 2017/01/21 02:15:00, chrishtr wrote:
> > >
>
https://codereview.chromium.org/2642763009/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/2642763009/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/layout/LayoutObject.cpp:2493: }
> > > ASSERT_NOT_REACHED()
> > > 
> > >
>
https://codereview.chromium.org/2642763009/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/2642763009/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:241:
> > > EXPECT_EQ(container, floating->containingBlock());
> > > Can you explain to me why containingBlock disagrees with
> > > container here?
> > 
> > At first I thought the answer was obvious, but after I read the spec and
> tested on FireFox, I found that this is a great question, and the basic idea
of
> this CL seems totally wrong.
> > 
> > This is the issue that this CL was to 'fix':
> > 
> >   <span style="position: relative; top: 50px; left: 50px">
> >     <div style="float: left; position: relative">Float</div>
> >   </span>
> > 
> > Now we paint the float with 50,50 offset. Before
> https://codereview.chromium.org/2575423003/, we also apply 50,50 offset to the
> float during geometry mapping. After
> https://codereview.chromium.org/2575423003/, we invalidate paint and paint in
> different ways. According to how we are painting, I thought the span
establishes
> "containing block" (our "container") for the float.
> > 
> > However, according to the spec, the relative-positioned span should not
affect
> positioning of the float. FireFox conforms to the spec.
> > 
> > Now I think the current LayoutObject::container() is correct to return
> containingBlock for floats. We should fix painting code instead. We should
undo
> the offset of the painting layer containing the float if the painting layer's
> layout object is not the containing block of the float.
> 
> Ok. Discarding this CL then right?

Yes. Replaced by https://codereview.chromium.org/2647383004/ and
https://codereview.chromium.org/2650873002/.