Strip anonymous wrappers when a block flow no longer requires them

Strip anonymous wrappers when a block flow no longer requires them

When a floating/out-of-flow or inline child is added to a an element with block
children ensure that we don't create unnecessary anonymous wrappers for it but
use any adjacent wrappers if they are available. For good measure, also ensure any
sequences of out-of-flow/floating/inline elements the addition of the inline child
creates are folded into the same anonymous block.

If we remove a child and it leaves us without any block children then reset the
parent to have inline children so that it doesn't get an unnecessary anonymous
wrapper. 

BUG=411256
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=202391

[rhogan, 2015-08-25 19:40:15]

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

[rhogan, 2015-08-25 19:41:05]

mstensho@ For now my test coverage is just the test cases from the bug. I'll try
to think of a few more but if you have any comments in the meantime that would
be great.

[szager1, 2015-08-26 17:04:39]

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

[szager1, 2015-08-26 17:04:40]

https://codereview.chromium.org/1310253005/diff/1/Source/core/layout/LayoutBl...
File Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1310253005/diff/1/Source/core/layout/LayoutBl...
Source/core/layout/LayoutBlock.cpp:454: while (nextChild &&
(nextChild->isFloatingOrOutOfFlowPositioned() || nextChild->isInline() ||
nextChild->isAnonymousBlock())) {
nextChild->isInline() should never be true here

https://codereview.chromium.org/1310253005/diff/1/Source/core/layout/LayoutBl...
Source/core/layout/LayoutBlock.cpp:460: nextBlock->deleteLineBoxTree();
Redundant?  nextBlock->moveAllChildrenIncludingFloatsTo() should delete the line
box tree.

[mstensho (USE GERRIT), 2015-08-26 19:44:15]

https://codereview.chromium.org/1310253005/diff/1/LayoutTests/fast/block/floa...
File
LayoutTests/fast/block/float/add-abspos-before-float-in-block-children-block-expected.html
(right):

https://codereview.chromium.org/1310253005/diff/1/LayoutTests/fast/block/floa...
LayoutTests/fast/block/float/add-abspos-before-float-in-block-children-block-expected.html:10:
<div id="a">
please terminate me.

https://codereview.chromium.org/1310253005/diff/1/LayoutTests/fast/block/floa...
File
LayoutTests/fast/block/float/add-inline-before-float-in-block-children-block-expected.html
(right):

https://codereview.chromium.org/1310253005/diff/1/LayoutTests/fast/block/floa...
LayoutTests/fast/block/float/add-inline-before-float-in-block-children-block-expected.html:10:
<div id="a">
Help! Where's my end tag?

https://codereview.chromium.org/1310253005/diff/1/LayoutTests/fast/block/floa...
File
LayoutTests/fast/block/float/add-inlines-in-block-children-block-expected.html
(right):

https://codereview.chromium.org/1310253005/diff/1/LayoutTests/fast/block/floa...
LayoutTests/fast/block/float/add-inlines-in-block-children-block-expected.html:10:
<div id="a">
Needs moar terminator.

https://codereview.chromium.org/1310253005/diff/1/Source/core/layout/LayoutBl...
File Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1310253005/diff/1/Source/core/layout/LayoutBl...
Source/core/layout/LayoutBlock.cpp:453: // Reparent adjacent inline siblings to
the new box.
As szager has already pointed out, how could there be inline siblings here?
We're in a block with block children.

Why don't we look at preceding siblings as well?

Making sure that we don't have extraneous anonymous block seems important, but
why don't we just try a bit harder to locate an anonymous block earlier instead
of creating a new one and deleting the old one? I'm talking about the "if
(afterChild && afterChild->isAnonymousBlock())" thing a bit further up. If your
new code finds an anonymous block, it means that we could have just used it in
the first place, instead of creating a new one and deleting the old one.

So, I'm thinking that I'd like to see:

1. Look harder for an existing anonymous block
2. If none can be found, create a new one, put the inline there, and reparent
adjacent (after AND before) floats and out-of-flow objects

One more thing: your changes here may lead to the situation where we have a
block-children-block whose only child is an anonymous block to contain inlines.
I dare you get rid of that redundant anonymous block and turn this parent block
into an inline-children-block! :) LayoutBlock::removeChild() already does it
when it happens ("The removal has knocked us down to containing only a single
anonymous box").

[rhogan, 2015-09-02 19:16:55]

Not quite there yet - but ready for another look

>
https://codereview.chromium.org/1310253005/diff/1/Source/core/layout/LayoutBl...
> File Source/core/layout/LayoutBlock.cpp (right):
> 
>
https://codereview.chromium.org/1310253005/diff/1/Source/core/layout/LayoutBl...
> Source/core/layout/LayoutBlock.cpp:453: // Reparent adjacent inline siblings
to the new box.
> As szager has already pointed out, how could there be inline siblings here?
We're in a block with block children.
> 
> Why don't we look at preceding siblings as well?
Done. In fact I found an early return where we don't need to add an anonymous
block at all: when we add an inline to block flow that only has out of
flow/floating elements.

> 
> Making sure that we don't have extraneous anonymous block seems important, but
why don't we just try a bit harder to locate an anonymous block earlier instead
of creating a new one and deleting the old one? I'm talking about the "if
(afterChild && afterChild->isAnonymousBlock())" thing a bit further up. If your
new code finds an anonymous block, it means that we could have just used it in
the first place, instead of creating a new one and deleting the old one.

I don't think we can look harder - but maybe I'm not seeing it. If we're
inserting before an element either it's previous sibling is an anonymous block
or it's not. If it's not we need to create one.

> 
> So, I'm thinking that I'd like to see:
> 
> 1. Look harder for an existing anonymous block
> 2. If none can be found, create a new one, put the inline there, and reparent
adjacent (after AND before) floats and out-of-flow objects

Done. 

> 
> One more thing: your changes here may lead to the situation where we have a
block-children-block whose only child is an anonymous block to contain inlines.
I dare you get rid of that redundant anonymous block and turn this parent block
into an inline-children-block! :) LayoutBlock::removeChild() already does it
when it happens ("The removal has knocked us down to containing only a single
anonymous box").

Haven't addressed this yet.

[mstensho (USE GERRIT), 2015-09-03 08:44:30]

> I don't think we can look harder - but maybe I'm not seeing it. If we're
> inserting before an element either it's previous sibling is an anonymous block
> or it's not. If it's not we need to create one.

Right... its previous sibling could be a float or out-of-flow thing. But if
that's the case, it would mean that there cannot be any interesting preceding
anonymous block sibling, because then such floats and out-of-flow things would
already have been reparented into it. So, alright, no point in looking in that
direction. But what if the *next* sibling - i.e. |beforeChild| - is an anonymous
block? Couldn't we parent into that one, as first child?

https://codereview.chromium.org/1310253005/diff/60001/Source/core/layout/Layo...
File Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1310253005/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutBlock.cpp:387: LayoutBlockFlow* block =
toLayoutBlockFlow(child);
I still think that if we find another anonymous block on our way, we have
failed. We should have parented into it in the first place.

https://codereview.chromium.org/1310253005/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutBlock.cpp:467: bool needsAnonymousBox =
isFlexibleBoxIncludingDeprecated() || isLayoutGrid();
I cannot find this condition anywhere else in the child insertion code. How did
you come up with it?

https://codereview.chromium.org/1310253005/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutBlock.cpp:468: while (nextChild && !needsAnonymousBox)
{
We're walking through all children to figure out if we can be turned into an
inline-children block? Since this is a block-children block, there already has
to be a block child there, since blocks (when created) start being marked as
having inline children until someone attempts to insert a block child. No?

[rhogan, 2015-09-06 15:09:27]

https://codereview.chromium.org/1310253005/diff/60001/Source/core/layout/Layo...
File Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1310253005/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutBlock.cpp:387: LayoutBlockFlow* block =
toLayoutBlockFlow(child);
On 2015/09/03 at 08:44:30, mstensho wrote:
> I still think that if we find another anonymous block on our way, we have
failed. We should have parented into it in the first place.

Yup - addressed this by parenting floating/out-of-flow objects into adjacent
anonymous blocks at style change time. This means we never have them in the way
when looking for a previous sibling that is an anonymous block.

https://codereview.chromium.org/1310253005/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutBlock.cpp:467: bool needsAnonymousBox =
isFlexibleBoxIncludingDeprecated() || isLayoutGrid();
On 2015/09/03 at 08:44:30, mstensho wrote:
> I cannot find this condition anywhere else in the child insertion code. How
did you come up with it?
These seem to be the only two layout blocks that do not allow inline children.

https://codereview.chromium.org/1310253005/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutBlock.cpp:468: while (nextChild && !needsAnonymousBox)
{
On 2015/09/03 at 08:44:30, mstensho wrote:
> We're walking through all children to figure out if we can be turned into an
inline-children block? Since this is a block-children block, there already has
to be a block child there, since blocks (when created) start being marked as
having inline children until someone attempts to insert a block child. No?
Clarified what's happening in the comments - we can end up here when we have
block children but they're all out of flow or floating.

[mstensho (USE GERRIT), 2015-09-07 14:01:48]

https://codereview.chromium.org/1310253005/diff/120001/Source/core/layout/Lay...
File Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1310253005/diff/120001/Source/core/layout/Lay...
Source/core/layout/LayoutBlock.cpp:464: // If we are a block that allows inline
children and the child we're adding means that
But we are not a block that allows inline children.

We're inside "if (!childrenInline() && ...)".

How do we end up with !childrenInline() and only floating or out-of-flow
children? I think that's a bug.

[rhogan, 2015-09-07 17:33:46]

On 2015/09/07 at 14:01:48, mstensho wrote:
>
https://codereview.chromium.org/1310253005/diff/120001/Source/core/layout/Lay...
> File Source/core/layout/LayoutBlock.cpp (right):
> 
>
https://codereview.chromium.org/1310253005/diff/120001/Source/core/layout/Lay...
> Source/core/layout/LayoutBlock.cpp:464: // If we are a block that allows
inline children and the child we're adding means that
> But we are not a block that allows inline children.
> 
> We're inside "if (!childrenInline() && ...)".
> 
> How do we end up with !childrenInline() and only floating or out-of-flow
children? I think that's a bug.

You could add a block-flow element and then remove it or switch it to being
inline-block, for example. We don't switch it back to being inline in the first
case but this will do it for the second. So yeah, I should just add it to
removeChild().

[mstensho (USE GERRIT), 2015-09-07 17:38:31]

On 2015/09/07 17:33:46, rhogan wrote:
> On 2015/09/07 at 14:01:48, mstensho wrote:
> >
>
https://codereview.chromium.org/1310253005/diff/120001/Source/core/layout/Lay...
> > File Source/core/layout/LayoutBlock.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1310253005/diff/120001/Source/core/layout/Lay...
> > Source/core/layout/LayoutBlock.cpp:464: // If we are a block that allows
> inline children and the child we're adding means that
> > But we are not a block that allows inline children.
> > 
> > We're inside "if (!childrenInline() && ...)".
> > 
> > How do we end up with !childrenInline() and only floating or out-of-flow
> children? I think that's a bug.
> 
> You could add a block-flow element and then remove it or switch it to being
> inline-block, for example. We don't switch it back to being inline in the
first
> case but this will do it for the second. So yeah, I should just add it to
> removeChild().

That's what I'm thinking too.

[rhogan, 2015-09-08 18:37:29]

On 2015/09/07 at 17:38:31, mstensho wrote:

> That's what I'm thinking too.

Ready for another look I think.

[mstensho (USE GERRIT), 2015-09-09 20:14:50]

A bit concerned about performance implications here, but not about the
correctness. :)

https://codereview.chromium.org/1310253005/diff/140001/Source/core/layout/Lay...
File Source/core/layout/LayoutBlock.cpp (left):

https://codereview.chromium.org/1310253005/diff/140001/Source/core/layout/Lay...
Source/core/layout/LayoutBlock.cpp:470: 
Since you're not changing anything else here, maybe back out this change as
well?

https://codereview.chromium.org/1310253005/diff/140001/Source/core/layout/Lay...
File Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1310253005/diff/140001/Source/core/layout/Lay...
Source/core/layout/LayoutBlock.cpp:306:
toLayoutBlock(parent())->moveChildTo(toLayoutBlock(previousSibling()), this, 0,
false);
0 -> nullptr

https://codereview.chromium.org/1310253005/diff/140001/Source/core/layout/Lay...
Source/core/layout/LayoutBlock.cpp:413: void
LayoutBlock::addAdjacentSiblingsToAnonymousBox(AdjacentSiblings siblings,
LayoutBlock* anonymousBox)
Not sure if this operation deserves its own method anymore. Almost every line
has to worry about whether we're walking forwards or backwards. :)

https://codereview.chromium.org/1310253005/diff/140001/Source/core/layout/Lay...
Source/core/layout/LayoutBlock.cpp:419: moveChildTo(anonymousBox, child,
previousSiblings ? anonymousBox->firstChild() : 0, false);
0 -> nullptr

https://codereview.chromium.org/1310253005/diff/140001/Source/core/layout/Lay...
Source/core/layout/LayoutBlock.cpp:795: } else {
Should condition this on !beingDestroyed().

And !child->isFloatingOrOutOfFlowPositioned()? Otherwise, the situation isn't
going to change.

https://codereview.chromium.org/1310253005/diff/140001/Source/core/layout/Lay...
Source/core/layout/LayoutBlock.cpp:799: bool childrenAllInline =
!isFlexibleBoxIncludingDeprecated() && !isLayoutGrid();
I wonder if we should say isLayoutBlockFlow() instead. And maybe not involve
childrenAllInline; just skip the rest of the code if it's an object type that
doesn't allow inline children.

https://codereview.chromium.org/1310253005/diff/140001/Source/core/layout/Lay...
Source/core/layout/LayoutBlock.cpp:800: while (nextChild && childrenAllInline) {
So, there's really no need for |childrenAllInline|. Just check if you have
non-null |nextChild| afterwards. And maybe name it |child| instead.

https://codereview.chromium.org/1310253005/diff/140001/Source/core/layout/Lay...
Source/core/layout/LayoutBlock.cpp:801: if
(!nextChild->isFloatingOrOutOfFlowPositioned()) {
What if we meet an anonymous block? Too much work to tidy up that? Oh wait,
that's not supposed to happen, is it? Maybe assert, then.

https://codereview.chromium.org/1310253005/diff/140001/Source/core/layout/Lay...
Source/core/layout/LayoutBlock.cpp:810: 
Extraneous blank line.

[rhogan, 2015-09-14 12:41:04]

Thanks for the review - incorporated your comments except the following:

> 
>
https://codereview.chromium.org/1310253005/diff/140001/Source/core/layout/Lay...
> Source/core/layout/LayoutBlock.cpp:801: if
> (!nextChild->isFloatingOrOutOfFlowPositioned()) {
> What if we meet an anonymous block? Too much work to tidy up that? Oh wait,
> that's not supposed to happen, is it? Maybe assert, then.
> 
If we meet an anonymous block (which we can), we just bail. We're only
interested in establishing if the children are all floating or out-of-flow - if
they are, we can make the block have inline children.

[mstensho (USE GERRIT), 2015-09-15 11:47:30]

Almost there now. :)

https://codereview.chromium.org/1310253005/diff/160001/LayoutTests/fast/block...
File
LayoutTests/fast/block/float/add-abspos-before-float-in-block-children-block-expected.html
(right):

https://codereview.chromium.org/1310253005/diff/160001/LayoutTests/fast/block...
LayoutTests/fast/block/float/add-abspos-before-float-in-block-children-block-expected.html:10:
<div id="a">
Please close the element.

https://codereview.chromium.org/1310253005/diff/160001/LayoutTests/fast/block...
File
LayoutTests/fast/block/float/add-inline-before-float-in-block-children-block-expected.html
(right):

https://codereview.chromium.org/1310253005/diff/160001/LayoutTests/fast/block...
LayoutTests/fast/block/float/add-inline-before-float-in-block-children-block-expected.html:10:
<div id="a">
Please close the element.

https://codereview.chromium.org/1310253005/diff/160001/LayoutTests/fast/block...
File
LayoutTests/fast/block/float/add-inline-between-floats-with-preceding-anonymous-box.html
(right):

https://codereview.chromium.org/1310253005/diff/160001/LayoutTests/fast/block...
LayoutTests/fast/block/float/add-inline-between-floats-with-preceding-anonymous-box.html:1:
<!DOCTYPE html>
The test looks fine. But could you add an foo-expected.html counterpart, or
change this to a check-layout.js test?

Let's avoid PNG / dump-render-tree tests when we can.

(This comment applies to multiple files)

https://codereview.chromium.org/1310253005/diff/160001/LayoutTests/fast/block...
File
LayoutTests/fast/block/float/add-inlines-in-block-children-block-expected.html
(right):

https://codereview.chromium.org/1310253005/diff/160001/LayoutTests/fast/block...
LayoutTests/fast/block/float/add-inlines-in-block-children-block-expected.html:10:
<div id="a">
Please close the DIV.

https://codereview.chromium.org/1310253005/diff/160001/Source/core/layout/Lay...
File Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1310253005/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutBlock.cpp:308: if (nextSibling() &&
nextSibling()->isAnonymousBlock())
else if. Not that there should be two adjacent anonymous blocks ever, but no
need to perform this check if we found a suitable previousSibling().

https://codereview.chromium.org/1310253005/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutBlock.cpp:487: // Reparent adjacent inline siblings to
the new box.
Not inline. Float or out-of-flow positioned.

https://codereview.chromium.org/1310253005/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutBlock.cpp:694: static bool
shouldMakeChildrenInline(LayoutBlock* block)
Could be static inline?

https://codereview.chromium.org/1310253005/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutBlock.cpp:694: static bool
shouldMakeChildrenInline(LayoutBlock* block)
I hope the parameter can be const.

https://codereview.chromium.org/1310253005/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutBlock.cpp:700: if
(!child->isFloatingOrOutOfFlowPositioned())
I still believe that if a LayoutBlockFlow only contains floats, out-of-flow
objects and/or anonymous blocks, we should get rid of all anonymous blocks and
promote their children. I think it's wrong to keep anonymous blocks for no
reason. But, by all means, not in this CL. Could you add a TODO, though?

https://codereview.chromium.org/1310253005/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutBlock.cpp:810: // If we are a block that allows inline
children and the child we're removing means that
The "that allows inline children" confuses me. I understand that it means "not
flexbox, not grid, not table", but this is currently a block that has BLOCK
children. Ok, so there's nothing incorrect with this sentence, but could you
just cut the beginning so that it becomes "If the child we're removing means
that ..."?

[rhogan, 2015-09-15 21:03:18]

On 2015/09/15 at 11:47:30, mstensho wrote:
> Almost there now. :)
> 

Addressed all your comments - except for the use of rendertext results which
I've kept on per our discussion in irc. There's no way of just having a
rendertree but no pixel results unfortunately.

[mstensho (USE GERRIT), 2015-09-16 08:48:10]

lgtm, but maybe you should run the performance tests. It's easy to construct a
test where shouldMakeChildrenInline() causes significant slowdown, although I
don't think it matters on the world wild web. If we already have a test that
suffers from this, we might have to investigate, or, at least notify whomever is
monitoring the performance test results that a slowdown is expected.

[rhogan, 2015-09-16 19:52:46]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2015-09-16 19:53:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1310253005/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1310253005/180001

[commit-bot: I haz the power, 2015-09-16 22:07:58]

Committed patchset #10 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202391