Make leading OOF objects to be handled by block layout in LayoutNG (issue 2847823002 by glebl@chromium.org)

[gl...@chromium.org]

Reviewers: ikilpatrick, kojii
CL: https://codereview.chromium.org/2847823002/

Description:
Make leading OOF objects to be handled by block layout in LayoutNG

This patch changes LayoutNG's LayoutObject tree for DOM examples with
OOF object

We want to use the block layout for out of flow positioned
objects when they go in front of inline blocks or if they are just
standalone objects.
Example 1:
<div id="zero"><div id="oof"></div></div>
Legacy Layout: #oof is in inline context.
LayoutNG: #oof is in block context.

Example 2:
<div id=container><oof></oof>Hello!</div>
Legacy Layout: oof is in inline context.
LayoutNG: oof is in block context.

Example 3:
<div id=container>Hello!<oof></oof></div>
Legacy Layout: oof is in inline context.
LayoutNG: oof is in inline context.

This also deprecates ShouldHandleByInlineContext in favor of
LayoutObject::ChildrenInline()

2 new failing tests:
floats-clear/floats-114.xht
floats-clear/floats-040.xht
Those are reftest, this patch fixes border/padding issue for OOF so they
started failing because we need to fix reftest as well.

BUG=712665,635619

Affected files (+32, -27 lines):
M third_party/WebKit/LayoutTests/TestExpectations
M third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
M third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm_test.cc
M third_party/WebKit/Source/core/layout/ng/ng_block_node.cc
M third_party/WebKit/Source/core/layout/ng/ng_block_node_test.cc


Index: third_party/WebKit/LayoutTests/TestExpectations
diff --git a/third_party/WebKit/LayoutTests/TestExpectations b/third_party/WebKit/LayoutTests/TestExpectations
index 6b5f754a601d4ae5eb99a1fcde55796603d06109..5afa83ebe958b1a9202c134be51a866331391c7a 100644
--- a/third_party/WebKit/LayoutTests/TestExpectations
+++ b/third_party/WebKit/LayoutTests/TestExpectations
@@ -371,12 +371,14 @@ crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/floats-clear/floats-031
crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/floats-clear/floats-036.xht [ Skip ]
crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/floats-clear/floats-038.xht [ Skip ]
crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/floats-clear/floats-039.xht [ Skip ]
+crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/floats-clear/floats-040.xht [ Skip ]
crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/floats-clear/floats-043.xht [ Skip ]
crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/floats-clear/floats-101.xht [ Skip ]
crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/floats-clear/floats-108.xht [ Skip ]
crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/floats-clear/floats-109.xht [ Skip ]
crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/floats-clear/floats-110.xht [ Skip ]
crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/floats-clear/floats-113.xht [ Skip ]
+crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/floats-clear/floats-114.xht [ Skip ]
crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/floats-clear/floats-117.xht [ Skip ]
crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/floats-clear/floats-120.xht [ Skip ]
crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/floats-clear/floats-126.xht [ Skip ]
Index: third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp b/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
index 9fd14255efb568cc83fcfc7da64129b8aba15919..3e9abd5d671ba8aaeedaf0b269388036334bdd4c 100644
--- a/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
@@ -2984,8 +2984,30 @@ void LayoutBlockFlow::AddChild(LayoutObject* new_child,
// children as blocks.
// So, if our children are currently inline and a block child has to be
// inserted, we move all our inline children into anonymous block boxes.
- bool child_is_block_level =
- !new_child->IsInline() && !new_child->IsFloatingOrOutOfFlowPositioned();
+ bool child_is_block_level = !new_child->IsInline();
+
+ // ** LayoutNG **
+ // We want to use the block layout for out of flow positioned
+ // objects when they go in front of inline blocks or if they are just
+ // standalone objects.
+ // Example 1:
+ // <div id="zero"><div id="oof"></div></div>
+ // Legacy Layout: #oof is in inline context.
+ // LayoutNG: #oof is in block context.
+ //
+ // Example 2:
+ // <div id=container><oof></oof>Hello!</div>
+ // Legacy Layout: oof is in inline context.
+ // LayoutNG: oof is in block context.
+ //
+ // Example 3:
+ // <div id=container>Hello!<oof></oof></div>
+ // Legacy Layout: oof is in inline context.
+ // LayoutNG: oof is in inline context.
+ bool layout_ng_enabled = RuntimeEnabledFeatures::layoutNGEnabled();
+ if (new_child->IsFloatingOrOutOfFlowPositioned())
+ child_is_block_level = layout_ng_enabled && !FirstChild();
+
if (ChildrenInline()) {
if (child_is_block_level) {
// Wrap the inline content in anonymous blocks, to allow for the new block
Index: third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm_test.cc
diff --git a/third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm_test.cc b/third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm_test.cc
index 946359d25203cced24ece0f797abba11a7724cd9..41ed97fca9fdf0a77f9e5ad17b48643015a47a83 100644
--- a/third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm_test.cc
+++ b/third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm_test.cc
@@ -119,8 +119,10 @@ TEST_F(NGInlineLayoutAlgorithmTest, TextFloatsAroundFloatsBefore) {
ToNGPhysicalBoxFragment(html_fragment->Children()[0].Get());
auto* container_fragment =
ToNGPhysicalBoxFragment(body_fragment->Children()[0].Get());
- auto* line_box_fragments_wrapper =
+ auto* span_box_fragments_wrapper =
ToNGPhysicalBoxFragment(container_fragment->Children()[0].Get());
+ auto* line_box_fragments_wrapper =
+ ToNGPhysicalBoxFragment(span_box_fragments_wrapper->Children()[0].Get());
Vector<NGPhysicalTextFragment*> text_fragments;
for (const auto& child : line_box_fragments_wrapper->Children()) {
auto* line_box = ToNGPhysicalLineBoxFragment(child.Get());
Index: third_party/WebKit/Source/core/layout/ng/ng_block_node.cc
diff --git a/third_party/WebKit/Source/core/layout/ng/ng_block_node.cc b/third_party/WebKit/Source/core/layout/ng/ng_block_node.cc
index 96402b5f64b35d33df7bb8377a1409ecae153d5f..36a3fa547ce57cd2b817ddbf357d9fd624f6fc6d 100644
--- a/third_party/WebKit/Source/core/layout/ng/ng_block_node.cc
+++ b/third_party/WebKit/Source/core/layout/ng/ng_block_node.cc
@@ -222,32 +222,11 @@ LayoutObject* NGBlockNode::GetLayoutObject() {
return layout_box_;
}

-static bool ShouldHandleByInlineContext(LayoutObject* child) {
- DCHECK(child);
- // The spec isn't clear about whether floats/OOF should be in inline
- // formatting context or in block formatting context.
- // Prefer inline formatting context because 1) floats/OOF at the beginning
- // and in the middle of inline should be handled in the same code, and 2)
- // it matches to the LayoutObject tree.
- for (; child; child = child->NextSibling()) {
- if (child->IsInline())
- return true;
- if (child->IsFloating() || child->IsOutOfFlowPositioned())
- continue;
- return false;
- }
- // All children are either float or OOF.
- // TODO(kojii): Should this be handled in block context or inline context?
- // If we handle in inline, we can remove all code for floats/OOF from block
- // layout, but it may change semantics and causes incorrectness?
- return false;
-}
-
NGLayoutInputNode* NGBlockNode::FirstChild() {
if (!first_child_) {
LayoutObject* child = layout_box_->SlowFirstChild();
if (child) {
- if (ShouldHandleByInlineContext(child)) {
+ if (layout_box_->ChildrenInline()) {
first_child_ = new NGInlineNode(child, ToLayoutBlockFlow(layout_box_));
} else {
first_child_ = new NGBlockNode(child);
Index: third_party/WebKit/Source/core/layout/ng/ng_block_node_test.cc
diff --git a/third_party/WebKit/Source/core/layout/ng/ng_block_node_test.cc b/third_party/WebKit/Source/core/layout/ng/ng_block_node_test.cc
index d3becbc086b9845c4ec174e69290d1588d4a7e2e..0950ab67bc028a9a15b8a92deb4f9fe66360b124 100644
--- a/third_party/WebKit/Source/core/layout/ng/ng_block_node_test.cc
+++ b/third_party/WebKit/Source/core/layout/ng/ng_block_node_test.cc
@@ -77,7 +77,7 @@ TEST_F(NGBlockNodeForTest, ChildFloatBeforeInline) {
NGBlockNode* container =
new NGBlockNode(GetLayoutObjectByElementId("container"));
NGLayoutInputNode* child1 = container->FirstChild();
- EXPECT_TRUE(child1 && child1->IsInline());
+ EXPECT_TRUE(child1 && child1->IsBlock());
NGLayoutInputNode* child2 = child1->NextSibling();
EXPECT_EQ(child2, nullptr);
}
@@ -143,7 +143,7 @@ TEST_F(NGBlockNodeForTest, ChildOofBeforeInline) {
NGBlockNode* container =
new NGBlockNode(GetLayoutObjectByElementId("container"));
NGLayoutInputNode* child1 = container->FirstChild();
- EXPECT_TRUE(child1 && child1->IsInline());
+ EXPECT_TRUE(child1 && child1->IsBlock());
NGLayoutInputNode* child2 = child1->NextSibling();
EXPECT_EQ(child2, nullptr);
}

[ikilp...@chromium.org]

lgtm if others are fine. +eae


https://codereview.chromium.org/2847823002/diff/1/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2847823002/diff/1/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp#newcode3006
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:3006: //

LayoutNG: oof is in inline context.

did we want the end children in the inline context? (also this does
that?)

https://codereview.chromium.org/2847823002/

[ko...@chromium.org]

Standalone case is fine with me.

Could you mind to explain why we prefer to handle leading OOF in block context?
That doesn't look intuitive to me, and it is slightly less efficient while it is
very common case, no?

https://codereview.chromium.org/2847823002/

[ko...@chromium.org]

On 2017/04/28 at 02:18:17, kojii wrote:
> Could you mind to explain why we prefer to handle leading OOF in block
context? That doesn't look intuitive to me, and it is slightly less efficient
while it is very common case, no?

Also it creates a situation where inline/block children are mixed, which I'd
like not to happen.
https://docs.google.com/a/chromium.org/document/d/1kAJLEYHl5eE6ean3NSTnJOuGbJFccr0bIqsIpou19k4/edit?usp=sharing

What is the problem you're trying to solve by making leading OOF as block?

https://codereview.chromium.org/2847823002/

[gl...@chromium.org]

I thought you agreed with this solution. May be I misunderstood you.
from https://codereview.chromium.org/2829593002/#msg25
"...If this is the only difference, and Ian is good, I'm good,..."
I discussed it with Ian and he agreed with this solution by LGTMing this patch.


re: What is the problem you're trying to solve by making leading OOF as block?
this patch is based on your patch http://crrev.com/2829593002 but fixes broken
unittests/layout tests.


https://codereview.chromium.org/2847823002/

[ko...@chromium.org]

I'm good with 1 and 3. I meant by "I'm good" comment that I'm good with 1, but
didn't mean 2...

Did you check how the LayoutObject tree looks like for 2 with this change? I
guess we create additional LayoutBlockFlow and additional NGBlockNode, no?
Floats at the beginning of inline is probably the most common case I think, I
wish us not to create anonymous blocks.

If we intentionally accept the inefficiency, you might be seeing other benefits,
and I'm wondering what the benefits are.

https://codereview.chromium.org/2847823002/

[gl...@chromium.org]

We don't create any additional LayoutBlockFlow/NGBlockNode for the use case N2
with this change.
Use case N2:
<div>
<span></span>
<float></float>
</div>

NGInputNodeTree:

NGBlockNode: 'LayoutNGBlockFlow HTML'
NGBlockNode: 'LayoutNGBlockFlow BODY'
NGBlockNode: 'LayoutNGBlockFlow DIV'
NGInlineNode
NGInlineItem. Type: 'OpenTag'. LayoutObject: 'LayoutInline SPAN'
NGInlineItem. Type: 'CloseTag'. LayoutObject: 'LayoutInline SPAN'
NGInlineItem. Type: 'Floating'. LayoutObject: 'LayoutNGBlockFlow
(floating) DIV class='float-left''

As can be seen the float is an inline node in this case and this is expected.

Another example:
Use case N5
<div>
<float></float>
<span></span>
<div></div>
</div>

NGInputNodeTree:

NGBlockNode: 'LayoutNGBlockFlow HTML'
NGBlockNode: 'LayoutNGBlockFlow BODY'
NGBlockNode: 'LayoutNGBlockFlow DIV'
NGBlockNode: 'LayoutNGBlockFlow (floating) DIV class='float-left''
NGBlockNode: 'LayoutNGBlockFlow (anonymous)'
NGInlineNode
NGInlineItem. Type: 'OpenTag'. LayoutObject: 'LayoutInline SPAN'
NGInlineItem. Type: 'CloseTag'. LayoutObject: 'LayoutInline SPAN'
NGBlockNode: 'LayoutNGBlockFlow DIV'

same here, only one anonymous wrapper for <span> element.
Please find the complete report by this link
https://docs.google.com/document/d/1p9z05t7yDLe2ZvKhISLRnArlyeHisC8ejsngV62AlAM/edit#


https://codereview.chromium.org/2847823002/

[ko...@chromium.org]

On 2017/05/02 at 01:43:23, glebl wrote:
> We don't create any additional LayoutBlockFlow/NGBlockNode for the use case N2
with this change.

The example 2 in the comment is the use case 1 in your report doc. There's
"NGBlockNode: 'LayoutNGBlockFlow (anonymous)'", right?

https://codereview.chromium.org/2847823002/

[gl...@chromium.org]

On 2017/05/02 02:48:54, kojii wrote:
> On 2017/05/02 at 01:43:23, glebl wrote:
> > We don't create any additional LayoutBlockFlow/NGBlockNode for the use case
N2
> with this change.
>
> The example 2 in the comment is the use case 1 in your report doc. There's
> "NGBlockNode: 'LayoutNGBlockFlow (anonymous)'", right?

yes, for the example below we need an anonymous block to wrap the inline node.

<div>
<float></float>
<span></span>

</div>

the main benefit of this patch is to keep the code path predictable. In the
legacy layout a first-child float can be a block or inline node(default). That
leads to the situation that in the example below the float is inline.

<div1 style="margin-bottom: 200px;">
<float></float>
</div1>
<div2 style="height: 10px;">
</div2>

In the new layout system we resolve the block's position in space before any
inline node. This patch tries to make a clear separation between block/inline
nodes. The current version of the block layout algorithm handles the example
above correctly and positions the float at div2's offset.
I don't know why we introduced ShouldHandleByInlineContext instead of using
ChildrenInline at the first place. The current code works correctly with the
ShouldHandleByInlineContext's version of NGNode tree. There is another way to
fix inline/block algorithms without this change by teaching the inline algorithm
about MarginStrut, empty divs, unpositioned floats etc. It's just a double/extra
work.


https://codereview.chromium.org/2847823002/

[gl...@chromium.org]

Koji, we discussed this issue at the Layout meeting today. Per the last update
from Ian/Email we want to try to introduce our own painter for inlines that will
paint directly from the fragments tree. Therefore we're fine with a temporarily
introduced anonymous block to wrap inlines in the example below.

<div>
<float></float>
<span></span>
</div>

I think it makes the code path be more predictable as we now handle these 2
examples in the same way, i.e. floats before inline/block remain in the block
context.
Ex 1.

<div>
<float></float>
<span></span>
</div>

The float is a block node here.

Ex 2.
<div>
<float></float>
<div></div>
</div>
The float is a block node here.


https://codereview.chromium.org/2847823002/

[e...@chromium.org]

LGTM as per discussion in layout meeting today. I'm not thrilled about the extra
anonymous block but it seems like the best option for the time being as it
simplifies both the painting and the float code.

https://codereview.chromium.org/2847823002/

[ko...@chromium.org]

lgtm, Japan is in a holiday week and that I was slow to respond, sorry about
this. I wasn't intended to slow this discussion this long, sorry about that.

https://codereview.chromium.org/2847823002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2847823002/20001

https://codereview.chromium.org/2847823002/

[commit-bot@chromium.org via codereview.chromium.org]

Try jobs failed on following builders:
chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/428274)

https://codereview.chromium.org/2847823002/

[ko...@chromium.org]

> I don't know why we introduced ShouldHandleByInlineContext instead of using
ChildrenInline at the first place.

It was to avoid mixed children only in NG, but your fix avoids it in a different
way, so that is fine now.

https://codereview.chromium.org/2847823002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch.

Follow status at:

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2847823002/60001

https://codereview.chromium.org/2847823002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/415a2e47b6e9f046a503388298ad2e94b24e3c7b

https://codereview.chromium.org/2847823002/