Fix geometry mapping issues for float under inline

Fix geometry mapping issues for float under inline

Floating object belongs to its containing block, no matter if it's
under an inline and no matter if the inline is layered, stacked, or
paint invalidation container. This requires us to specially handle
floating objects for LayoutObject::paintingLayer(), LayoutObject::
container(), paint invalidator, etc.

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

Review-Url: https://codereview.chromium.org/2575423003
Cr-Commit-Position: refs/heads/master@{#442046}
Committed: https://chromium.googlesource.com/chromium/src/+/78b26d84270257a830facd6477ef6e3ff59aca66

[Xianzhu, 2016-12-16 01:57:40]

Description was changed from

==========
Fix paint invalidation issues for float under inline

Floating object belongs to its containing block, no matter if it's
under an inline and no matter if the inline is layered, stacked, or
paint invalidation container. This requires us to specially handle
floating objects for LayoutObject::paintingLayer(), LayoutObject::
container(), paint invalidator, etc.

BUG=666553
==========

to

==========
Fix paint invalidation issues for float under inline

Floating object belongs to its containing block, no matter if it's
under an inline and no matter if the inline is layered, stacked, or
paint invalidation container. This requires us to specially handle
floating objects for LayoutObject::paintingLayer(), LayoutObject::
container(), paint invalidator, etc.

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

[Xianzhu, 2016-12-16 01:57:41]

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

[commit-bot: I haz the power, 2016-12-16 01:58:35]

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, 2016-12-16 03:46:06]

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

[commit-bot: I haz the power, 2016-12-16 03:46:07]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Xianzhu, 2016-12-16 04:18:22]

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

[commit-bot: I haz the power, 2016-12-16 04:19: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, 2016-12-16 04:49:01]

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

[commit-bot: I haz the power, 2016-12-16 04:49:02]

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

[Xianzhu, 2016-12-16 05:53:17]

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

[commit-bot: I haz the power, 2016-12-16 05:54:07]

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, 2016-12-16 07:45:45]

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

[commit-bot: I haz the power, 2016-12-16 07:45:46]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Xianzhu, 2016-12-16 17:25:29]

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

[chrishtr, 2016-12-16 18:50:20]

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

https://codereview.chromium.org/2575423003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2544: if (isFloating())
I'm concerned about side-effects of modifying container() like this.
Are floats just broken in this code in general? How are there not more
consequences?

[Xianzhu, 2016-12-16 19:08:49]

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

https://codereview.chromium.org/2575423003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2544: if (isFloating())
On 2016/12/16 18:50:20, chrishtr wrote:
> I'm concerned about side-effects of modifying container() like this.
> Are floats just broken in this code in general? How are there not more
> consequences?

Paint invalidation of float-under-inline was broken in general because of wrong
container() and paintingLayer(). We computed wrong visual rect for the float
because the containing inline was treated as the container so added wrong paint
offsets; and invalidated wrong layer if the inline has a layer. We didn't have
test coverage for the case.

[wkorman, 2016-12-16 19:40:31]

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

https://codereview.chromium.org/2575423003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2544: if (isFloating())
On 2016/12/16 at 19:08:49, Xianzhu wrote:
> On 2016/12/16 18:50:20, chrishtr wrote:
> > I'm concerned about side-effects of modifying container() like this.
> > Are floats just broken in this code in general? How are there not more
> > consequences?
> 
> Paint invalidation of float-under-inline was broken in general because of
wrong container() and paintingLayer(). We computed wrong visual rect for the
float because the containing inline was treated as the container so added wrong
paint offsets; and invalidated wrong layer if the inline has a layer. We didn't
have test coverage for the case.

Are there any ramifications to using containingBlock() from within container()
given that it is explicitly defined as differing in three ways, from
LayoutObject.h docs:

  // (1) It can be used on orphaned subtrees, i.e., it can be called safely
  //     even when the object is not part of the primary document subtree yet.
  // (2) For normal flow elements, it just returns the parent.
  // (3) For absolute positioned elements, it will return a relative
  //     positioned inline. containingBlock() simply skips relpositioned inlines
  //     and lets an enclosing block handle the layout of the positioned object.
  //     This does mean that computePositionedLogicalWidth and
  //     computePositionedLogicalHeight have to use container().

(1) seems not impactful for this issue.
(2) seems like it could be an issue in certain cases, but maybe it's ok because
it would just lead to skipping things we want skipped for the float case anyway?
Are there test cases we can add to validate that this float case you are looking
to fix works for both out of flow and normal flow?
(3) seems not impactful for this issue.

[Xianzhu, 2016-12-16 20:06:42]

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

https://codereview.chromium.org/2575423003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2544: if (isFloating())
On 2016/12/16 19:40:31, wkorman wrote:
> On 2016/12/16 at 19:08:49, Xianzhu wrote:
> > On 2016/12/16 18:50:20, chrishtr wrote:
> > > I'm concerned about side-effects of modifying container() like this.
> > > Are floats just broken in this code in general? How are there not more
> > > consequences?
> > 
> > Paint invalidation of float-under-inline was broken in general because of
> wrong container() and paintingLayer(). We computed wrong visual rect for the
> float because the containing inline was treated as the container so added
wrong
> paint offsets; and invalidated wrong layer if the inline has a layer. We
didn't
> have test coverage for the case.
> 
> Are there any ramifications to using containingBlock() from within container()
> given that it is explicitly defined as differing in three ways, from
> LayoutObject.h docs:
> 
>   // (1) It can be used on orphaned subtrees, i.e., it can be called safely
>   //     even when the object is not part of the primary document subtree yet.
>   // (2) For normal flow elements, it just returns the parent.
>   // (3) For absolute positioned elements, it will return a relative
>   //     positioned inline. containingBlock() simply skips relpositioned
inlines
>   //     and lets an enclosing block handle the layout of the positioned
object.
>   //     This does mean that computePositionedLogicalWidth and
>   //     computePositionedLogicalHeight have to use container().
> 
> (1) seems not impactful for this issue.
> (2) seems like it could be an issue in certain cases, but maybe it's ok
because
> it would just lead to skipping things we want skipped for the float case
anyway?
> Are there test cases we can add to validate that this float case you are
looking
> to fix works for both out of flow and normal flow?

According to https://www.w3.org/TR/CSS2/visuren.html#positioning-scheme,
floating objects are out of flow, so none of the three points cover floating
objects, and making container() to be the same as containingBlock() for floating
objects doesn't conflict with these points.

What do you mean "works for both out of flow and normal flow"? Can you give some
simple examples?

> (3) seems not impactful for this issue.

https://codereview.chromium.org/2575423003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2575423003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:899: // (2) For normal flow
elements except for floating objects, it just returns
Based on https://www.w3.org/TR/CSS2/visuren.html#positioning-scheme, floating
objects should not be an exception of normal flow elements.

Perhaps it worth a note about that floating objects don't belong to exception 2.

[wkorman, 2016-12-16 21:50:40]

lgtm

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

https://codereview.chromium.org/2575423003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2544: if (isFloating())
On 2016/12/16 at 20:06:42, Xianzhu wrote:
> According to https://www.w3.org/TR/CSS2/visuren.html#positioning-scheme,
floating objects are out of flow, so none of the three points cover floating
objects, and making container() to be the same as containingBlock() for floating
objects doesn't conflict with these points.
> 
> What do you mean "works for both out of flow and normal flow"? Can you give
some simple examples?

I was thinking the same as the tests you've already added in this change, but
with add'l permutations of the ancestor state (abspos, relpos, composited vs.
not, inline vs box). But this change should only affect the float on which
container() is called, so ancestor state is not relevant unless we are uncertain
re: our coverage of some of those other states.

Am ok proceeding as is.

[chrishtr, 2016-12-16 22:10:30]

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

https://codereview.chromium.org/2575423003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:127:
TEST_F(LayoutObjectTest, FloatUnderInline) {
Please add more float examples to test your changes to containingBlock &
container. Also verify that your change does not change the answer for any of
them.

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

https://codereview.chromium.org/2575423003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp:713:
EXPECT_EQ(LayoutRect(8, 8, 33, 44), rect);
add * { margin: 0 } to get rid of the 8px

https://codereview.chromium.org/2575423003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right):

https://codereview.chromium.org/2575423003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:68: // This is
the same as current.paintOffset except when a floating object has
Nit: add a comma after paintOffset

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

https://codereview.chromium.org/2575423003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:3240:
EXPECT_EQ(LayoutPoint(8, 8), localBorderBoxProperties->paintOffset);
* { margin: 0 }

[Xianzhu, 2016-12-17 04:58:37]

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

[commit-bot: I haz the power, 2016-12-17 04:58:51]

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

[Xianzhu, 2016-12-17 05:43:18]

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

https://codereview.chromium.org/2575423003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:127:
TEST_F(LayoutObjectTest, FloatUnderInline) {
On 2016/12/16 22:10:30, chrishtr wrote:
> Please add more float examples to test your changes to containingBlock &
> container. Also verify that your change does not change the answer for any of
> them.

Added a FloatUnderBlock case, and added test for containingBlock().

We also have a lot of existing float-under-block layout tests, so there should
be enough coverage.

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

https://codereview.chromium.org/2575423003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp:713:
EXPECT_EQ(LayoutRect(8, 8, 33, 44), rect);
On 2016/12/16 22:10:30, chrishtr wrote:
> add * { margin: 0 } to get rid of the 8px

Added an position:absolute container to test we accumulate block ancestor paint
offset for floating objects.

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

https://codereview.chromium.org/2575423003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:3240:
EXPECT_EQ(LayoutPoint(8, 8), localBorderBoxProperties->paintOffset);
On 2016/12/16 22:10:30, chrishtr wrote:
> * { margin: 0 }

Added an absolute position div container to ensure we will just skip paint
offsets of the inline parent, but not paint offset of the containing block.

[commit-bot: I haz the power, 2016-12-17 06:58:45]

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

[commit-bot: I haz the power, 2016-12-17 06:58:45]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Xianzhu, 2017-01-03 21:33:54]

Chris, do you have more comments?

[chrishtr, 2017-01-03 21:39:14]

lgtm

[Xianzhu, 2017-01-03 21:41:47]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-01-03 21:41:47]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2575423003/#ps60001
(title: "-")

[commit-bot: I haz the power, 2017-01-03 21:42:11]

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-03 21:44:17]

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

[commit-bot: I haz the power, 2017-01-03 21:44:18]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Xianzhu, 2017-01-03 22:20:27]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-01-03 22:20:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2575423003/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-03 22:20:53]

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-04 00:53:40]

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

[commit-bot: I haz the power, 2017-01-04 00:53:41]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Xianzhu, 2017-01-04 01:07:10]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2017-01-04 01:07:38]

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-04 01:15:19]

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

[commit-bot: I haz the power, 2017-01-04 01:15:21]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Xianzhu, 2017-01-04 01:35:28]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-01-04 01:35:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2575423003/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-04 01:36:04]

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-04 03:32:56]

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

[commit-bot: I haz the power, 2017-01-04 03:32:57]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Xianzhu, 2017-01-04 19:33:05]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-01-04 19:33:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2575423003/#ps120001
(title: "Fix LayoutGeometryMapper::pushMappingsToAncestor(PaintLayer*,
PaintLayer*)")

[commit-bot: I haz the power, 2017-01-04 19:33:27]

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

[Xianzhu, 2017-01-04 19:41:22]

Description was changed from

==========
Fix paint invalidation issues for float under inline

Floating object belongs to its containing block, no matter if it's
under an inline and no matter if the inline is layered, stacked, or
paint invalidation container. This requires us to specially handle
floating objects for LayoutObject::paintingLayer(), LayoutObject::
container(), paint invalidator, etc.

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

to

==========
Fix geometry mapping issues for float under inline

Floating object belongs to its containing block, no matter if it's
under an inline and no matter if the inline is layered, stacked, or
paint invalidation container. This requires us to specially handle
floating objects for LayoutObject::paintingLayer(), LayoutObject::
container(), paint invalidator, etc.

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

[commit-bot: I haz the power, 2017-01-04 20:57:08]

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

[commit-bot: I haz the power, 2017-01-04 20:57:10]

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_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Xianzhu, 2017-01-04 21:39:16]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-01-04 21:39:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2575423003/#ps140001
(title: "Fix svg/text/clear-floats-crash.svg")

[commit-bot: I haz the power, 2017-01-04 21:39:38]

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

[Xianzhu, 2017-01-04 21:43:36]

The CQ bit was unchecked by wangxianzhu@chromium.org

[Xianzhu, 2017-01-04 22:11:33]

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

[commit-bot: I haz the power, 2017-01-04 22:11:55]

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

[Xianzhu, 2017-01-04 22:12:05]

Patchset #8 (id:140001) has been deleted

[Xianzhu, 2017-01-04 22:16:31]

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

[Xianzhu, 2017-01-04 22:16:32]

There are two small changes since the last lgtm:

- Fix LayoutGeometryMapper::pushMappingsToAncestor(PaintLayer* layer,
PaintLayer* ancestorLayer) where layer is under a float and ancestorLayer is an
inline:
https://codereview.chromium.org/2575423003/diff2/100001:120001/third_party/We...
as well as a unit test.

- Fix PaintPropertyTreeBuilder floating paint offset handling for SVG text with
float CSS property:
https://codereview.chromium.org/2575423003/diff2/120001:160001/third_party/We....

PTAL.

[wkorman, 2017-01-04 22:27:09]

https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp (right):

https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp:208: if
(current->isFloating() && current->parent() &&
Some concern re: unexpected ramifications of this, but indirect callsites look
limited to CompositingInputsUpdater and ScrollingCoordinator. It doesn't look
like there are any existing unit tests for this class. It seems worth adding
some if this is expected to be long-lived code, at least to cover this newly
added logic initially.

Perhaps add eae@ or szager@ to get layout team input.

https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.cpp (right):

https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.cpp:75:
setFloating(false);
What was causing floating to be set to true in some cases? Is this just
enforcing something already in spec that we were not enforcing to date, and dev
could just put a float style on an svg element and it would have taken?

[Xianzhu, 2017-01-04 22:49:58]

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

[Xianzhu, 2017-01-04 22:49:59]

+szager for feedback about the change in LayoutGeometryMap
(https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...)
and its test
(https://codereview.chromium.org/2575423003/diff2/100001:160001/third_party/We...).

https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp (right):

https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp:208: if
(current->isFloating() && current->parent() &&
On 2017/01/04 22:27:08, wkorman wrote:
> Some concern re: unexpected ramifications of this, but indirect callsites look
> limited to CompositingInputsUpdater and ScrollingCoordinator. It doesn't look
> like there are any existing unit tests for this class. It seems worth adding
> some if this is expected to be long-lived code, at least to cover this newly
> added logic initially.
> 
> Perhaps add eae@ or szager@ to get layout team input.

The unit test is here:
https://codereview.chromium.org/2575423003/diff2/100001:160001/third_party/We....

This function should return false if the parent-child relationship of
PaintLayers is different from the container-containee relationship of the
layers' objects.

https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.cpp (right):

https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGBlock.cpp:75:
setFloating(false);
On 2017/01/04 22:27:08, wkorman wrote:
> What was causing floating to be set to true in some cases? Is this just
> enforcing something already in spec that we were not enforcing to date, and
dev
> could just put a float style on an svg element and it would have taken?

isFloating() is set in LayoutBox::updateFromStyle():
  setFloating(!isOutOfFlowPositioned() && styleToUse.isFloating());

Most SVG objects ignores 'float' because they are not LayoutBox. LayoutSVGBlock
inherits from LayoutBlockFlow so it get isFloating set if there is 'float' style
which should be ignored.

Most of our code already ignore isFloating() of an LayoutSVGBlock, except for
PaintPropertyTreeBuilder in this CL
(https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...).

[wkorman, 2017-01-04 22:59:49]

lgtm

[commit-bot: I haz the power, 2017-01-05 00:37:32]

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

[commit-bot: I haz the power, 2017-01-05 00:37:33]

Dry run: This issue passed the CQ dry run.

[mstensho (USE GERRIT), 2017-01-05 10:31:29]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2017-01-05 10:31:30]

https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1120: LayoutBlock*
containingBlock(const LayoutBoxModelObject* ancestor = nullptr,
How about a struct that holds these three variables instead?

struct AncestorSkipInfo { // There could be a better name
    const LayoutBoxModelObject* const ancestor { nullptr };
    bool ancestorSkipped { false };
    bool filterSkipped { false };
};

LayoutBlock* containingBlock(AncestorSkipInfo* = nullptr)

And then we should have the same in containingBlockForAbsolutePosition(), and
even container().

[Xianzhu, 2017-01-06 00:42:36]

https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1120: LayoutBlock*
containingBlock(const LayoutBoxModelObject* ancestor = nullptr,
On 2017/01/05 10:31:30, mstensho wrote:
> How about a struct that holds these three variables instead?
> 
> struct AncestorSkipInfo { // There could be a better name
>     const LayoutBoxModelObject* const ancestor { nullptr };
>     bool ancestorSkipped { false };
>     bool filterSkipped { false };
> };
> 
> LayoutBlock* containingBlock(AncestorSkipInfo* = nullptr)
> 
> And then we should have the same in containingBlockForAbsolutePosition(), and
> even container().

Sounds good. Would like to address this in a follow-up.

[mstensho (USE GERRIT), 2017-01-06 09:34:48]

On 2017/01/06 00:42:36, Xianzhu wrote:
>
https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutObject.h (right):
> 
>
https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutObject.h:1120: LayoutBlock*
> containingBlock(const LayoutBoxModelObject* ancestor = nullptr,
> On 2017/01/05 10:31:30, mstensho wrote:
> > How about a struct that holds these three variables instead?
> > 
> > struct AncestorSkipInfo { // There could be a better name
> >     const LayoutBoxModelObject* const ancestor { nullptr };
> >     bool ancestorSkipped { false };
> >     bool filterSkipped { false };
> > };
> > 
> > LayoutBlock* containingBlock(AncestorSkipInfo* = nullptr)
> > 
> > And then we should have the same in containingBlockForAbsolutePosition(),
and
> > even container().
> 
> Sounds good. Would like to address this in a follow-up.

Sure! Might as well do it myself, if you have better things to do. :)

[Xianzhu, 2017-01-06 16:59:44]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-01-06 16:59:44]

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/2575423003/#ps160001
(title: "SVG floating")

[commit-bot: I haz the power, 2017-01-06 17:00:02]

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

[Xianzhu, 2017-01-06 17:00:04]

On 2017/01/06 09:34:48, mstensho wrote:
> On 2017/01/06 00:42:36, Xianzhu wrote:
> >
>
https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/layout/LayoutObject.h (right):
> > 
> >
>
https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/layout/LayoutObject.h:1120: LayoutBlock*
> > containingBlock(const LayoutBoxModelObject* ancestor = nullptr,
> > On 2017/01/05 10:31:30, mstensho wrote:
> > > How about a struct that holds these three variables instead?
> > > 
> > > struct AncestorSkipInfo { // There could be a better name
> > >     const LayoutBoxModelObject* const ancestor { nullptr };
> > >     bool ancestorSkipped { false };
> > >     bool filterSkipped { false };
> > > };
> > > 
> > > LayoutBlock* containingBlock(AncestorSkipInfo* = nullptr)
> > > 
> > > And then we should have the same in containingBlockForAbsolutePosition(),
> and
> > > even container().
> > 
> > Sounds good. Would like to address this in a follow-up.
> 
> Sure! Might as well do it myself, if you have better things to do. :)

Please go ahead for it. Thanks!

[commit-bot: I haz the power, 2017-01-06 21:03:18]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1483721984339500,
"parent_rev": "913296ee74616a4589b5fea9c5873ead36e0e321", "commit_rev":
"78b26d84270257a830facd6477ef6e3ff59aca66"}

[commit-bot: I haz the power, 2017-01-06 21:03:48]

Description was changed from

==========
Fix geometry mapping issues for float under inline

Floating object belongs to its containing block, no matter if it's
under an inline and no matter if the inline is layered, stacked, or
paint invalidation container. This requires us to specially handle
floating objects for LayoutObject::paintingLayer(), LayoutObject::
container(), paint invalidator, etc.

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

to

==========
Fix geometry mapping issues for float under inline

Floating object belongs to its containing block, no matter if it's
under an inline and no matter if the inline is layered, stacked, or
paint invalidation container. This requires us to specially handle
floating objects for LayoutObject::paintingLayer(), LayoutObject::
container(), paint invalidator, etc.

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

Review-Url: https://codereview.chromium.org/2575423003
Cr-Commit-Position: refs/heads/master@{#442046}
Committed:
https://chromium.googlesource.com/chromium/src/+/78b26d84270257a830facd6477ef...
==========

[commit-bot: I haz the power, 2017-01-06 21:03:49]

Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/78b26d84270257a830facd6477ef...

[mstensho (USE GERRIT), 2017-01-15 08:03:51]

On 2017/01/06 17:00:04, Xianzhu wrote:
> On 2017/01/06 09:34:48, mstensho wrote:
> > On 2017/01/06 00:42:36, Xianzhu wrote:
> > >
> >
>
https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/layout/LayoutObject.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2575423003/diff/160001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/layout/LayoutObject.h:1120: LayoutBlock*
> > > containingBlock(const LayoutBoxModelObject* ancestor = nullptr,
> > > On 2017/01/05 10:31:30, mstensho wrote:
> > > > How about a struct that holds these three variables instead?
> > > > 
> > > > struct AncestorSkipInfo { // There could be a better name
> > > >     const LayoutBoxModelObject* const ancestor { nullptr };
> > > >     bool ancestorSkipped { false };
> > > >     bool filterSkipped { false };
> > > > };
> > > > 
> > > > LayoutBlock* containingBlock(AncestorSkipInfo* = nullptr)
> > > > 
> > > > And then we should have the same in
containingBlockForAbsolutePosition(),
> > and
> > > > even container().
> > > 
> > > Sounds good. Would like to address this in a follow-up.
> > 
> > Sure! Might as well do it myself, if you have better things to do. :)
> 
> Please go ahead for it. Thanks!

Here: https://codereview.chromium.org/2634493007/