Bug 115687 – Need to Remove Anonymous Wrappers When All Children Become Inline

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Created attachment 200861 [details]
Testcase

The attached testcase lays out differently when the CSS float property is toggled of and on again for the two <div> elements.

When the page is first loaded, the two green boxes are staggered, and the red box is visible. After toggling 'float: left' off and on again for the divs, the two green boxes are aligned horizontally, and the red box is invisible.

[Simon Fraser (smfr)]

<rdar://problem/13822704>

[Simon Fraser (smfr)]

The behavior of this testcase also changed in http://trac.webkit.org/changeset/142152

[Robert Hogan]

Created attachment 200937 [details]
Reduction toggling floats off and then on again

[Robert Hogan]

Created attachment 200938 [details]
Reduction toggling floats off and then on again

[Robert Hogan]

On WebKi(In reply to comment #4)
> Created an attachment (id=200938) [details]
> Reduction toggling floats off and then on again

On WebKit r147273 and above I get a green bar below the red box. Opera does the same. In IE10 the behaviour is as Simon describes - the two green boxes are aligned horizontally and the red box is invisible. FireFox is the only one to restore the layout as it was originally.

[Robert Hogan]

The bug is another case where we ought to destroy anonymous wrappers but don't. Bug 52123 has quite a few other similarly affected cases from the CSS test suite.

[Robert Hogan]

Created attachment 201260 [details]
Patch

[Build Bot]

Comment on attachment 201260 [details]
Patch

Attachment 201260 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/299064

New failing tests:
fast/dynamic/002.html
fast/block/float/float-not-removed-from-next-sibling2.html

[Build Bot]

Created attachment 201301 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Build Bot]

Comment on attachment 201260 [details]
Patch

Attachment 201260 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/297349

New failing tests:
fast/dynamic/002.html
fast/block/float/float-not-removed-from-next-sibling2.html

[Build Bot]

Created attachment 201479 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Robert Hogan]

Created attachment 201631 [details]
Patch

[Build Bot]

Comment on attachment 201631 [details]
Patch

Attachment 201631 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/470004

New failing tests:
fast/dynamic/002.html
fast/block/float/float-not-removed-from-next-sibling2.html

[Build Bot]

Created attachment 201653 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Build Bot]

Comment on attachment 201631 [details]
Patch

Attachment 201631 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/375963

New failing tests:
fast/dynamic/002.html
fast/block/float/float-not-removed-from-next-sibling2.html

[Build Bot]

Created attachment 201668 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Robert Hogan]

Created attachment 201748 [details]
Patch

[Robert Hogan]

Created attachment 201751 [details]
Patch

[Dave Hyatt]

Comment on attachment 201751 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=201751&action=review

r-

> Source/WebCore/rendering/RenderObject.cpp:1680
> +void RenderObject::removeAnonymousWrappersFromLineIfNecessary()
> +{

It sure looks like you only intend to run this code for inlines, but I see no check for this.

I'd also rename it to removeAnonymousWrappersForInlinesIfNecessary.

> Source/WebCore/rendering/RenderObject.cpp:1694
> +    Vector<RenderObject*> anonymousBlocks;
> +    RenderObject* curr = parent()->firstChild();
> +    while (curr) {
> +        if (curr->isAnonymousBlock() && !toRenderBlock(curr)->isAnonymousBlockContinuation())
> +            anonymousBlocks.append(curr);
> +        else if (!curr->style()->isFloating() && !curr->style()->hasOutOfFlowPosition())
> +            return;
> +        curr = curr->nextSibling();
> +    }

Sorry, I do not understand this code at all. You'll have to explain it. It seems like your parent() could just be some random RenderInline, so why would you want to look at an inline's kids here? I am not following this.

> Source/WebCore/rendering/RenderObject.cpp:1698
> +    RenderBlock* parentBlock = toRenderBlock(parent());
> +    for (unsigned i = 0; i < anonymousBlocks.size(); i++)
> +        parentBlock->collapseAnonymousBoxChild(parentBlock, toRenderBlock(anonymousBlocks[i]));

How do you know your parent is a block? Your s_noLongerAffectsParentBlock boolean is set to true when your parent is a RenderInline().

[Robert Hogan]

(In reply to comment #19)

> > Source/WebCore/rendering/RenderObject.cpp:1698
> > +    RenderBlock* parentBlock = toRenderBlock(parent());
> > +    for (unsigned i = 0; i < anonymousBlocks.size(); i++)
> > +        parentBlock->collapseAnonymousBoxChild(parentBlock, toRenderBlock(anonymousBlocks[i]));
> 
> How do you know your parent is a block? Your s_noLongerAffectsParentBlock boolean is set to true when your parent is a RenderInline().

Wood, meet trees - I only want to do this when the parent is a RenderBlock as far as I can tell.

[Robert Hogan]

Created attachment 201975 [details]
Patch

[Dave Hyatt]

Comment on attachment 201975 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=201975&action=review

r-

> Source/WebCore/rendering/RenderObject.cpp:125
> +bool RenderObject::s_noLongerAffectsParentBlock = false;

You should tighten this since you only want to run it for inlines. s_inlineBecameOutOfFlow would be a better name.

> Source/WebCore/rendering/RenderObject.cpp:1690
> +    Vector<RenderObject*> anonymousBlocks;

Not really following why you have to collect stuff into a Vector.

> Source/WebCore/rendering/RenderObject.cpp:1698
> +    while (curr) {
> +        if (curr->isAnonymousBlock() && !toRenderBlock(curr)->isAnonymousBlockContinuation())
> +            anonymousBlocks.append(curr);
> +        else if (!curr->style()->isFloating() && !curr->style()->hasOutOfFlowPosition())
> +            return;
> +        curr = curr->nextSibling();
> +    }

I think this code has the potential to kill a pre or post anonymous block created by continuations. You can't get rid of those, even if they become empty.

It seems to me that you're better off writing the loop like this:

RenderObject* curr = parent()->firstChild();
while (curr && (curr->isAnonymousBlock() && !toRenderBlock(curr)->isAnonymousBlockContinuation()) || (curr->style()->isFloating() || curr->style()->hasOutOfFlowPosition()))
    cur = curr->nextSibling();
if (curr) return;

Then you just walk the render tree children again and do the collapse.

> Source/WebCore/rendering/RenderObject.cpp:1910
> +        s_noLongerAffectsParentBlock = ((!isFloating() && newStyle->isFloating()) || (!isOutOfFlowPositioned() && newStyle->hasOutOfFlowPosition()))
> +            && parent() && parent()->isRenderBlock();

Add isInline() && at the start of this, since you don't want to run this for blocks.

[Robert Hogan]

Created attachment 202158 [details]
Patch

[Robert Hogan]

(In reply to comment #22)
> > Source/WebCore/rendering/RenderObject.cpp:1910
> > +        s_noLongerAffectsParentBlock = ((!isFloating() && newStyle->isFloating()) || (!isOutOfFlowPositioned() && newStyle->hasOutOfFlowPosition()))
> > +            && parent() && parent()->isRenderBlock();
> 
> Add isInline() && at the start of this, since you don't want to run this for blocks.

I do - if a block is the only block on a line and it becomes floating then any inlines on the line can drop their anonymous wrappers.

[Dave Hyatt]

Comment on attachment 202158 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 202158 [details]
Patch

Clearing flags on attachment: 202158

Committed r150527: <http://trac.webkit.org/changeset/150527>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.