Bug 115687

Summary: Need to Remove Anonymous Wrappers When All Children Become Inline
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Layout and RenderingAssignee: Robert Hogan <robert>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, esprehn+autocc, glenn, hyatt, rniwa, robert, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 115820, 115818    
Attachments:
Description Flags
Testcase
none
Reduction toggling floats off and then on again
none
Reduction toggling floats off and then on again
none
Patch
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
none
Patch
none
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
none
Patch
none
Patch
none
Patch
none
Patch none

Description Simon Fraser (smfr) 2013-05-06 18:13:41 PDT
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.
Comment 1 Simon Fraser (smfr) 2013-05-06 18:14:04 PDT
<rdar://problem/13822704>
Comment 2 Simon Fraser (smfr) 2013-05-06 18:15:27 PDT
The behavior of this testcase also changed in http://trac.webkit.org/changeset/142152
Comment 3 Robert Hogan 2013-05-07 11:15:41 PDT
Created attachment 200937 [details]
Reduction toggling floats off and then on again
Comment 4 Robert Hogan 2013-05-07 11:16:27 PDT
Created attachment 200938 [details]
Reduction toggling floats off and then on again
Comment 5 Robert Hogan 2013-05-07 11:18:50 PDT
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.
Comment 6 Robert Hogan 2013-05-07 11:51:14 PDT
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.
Comment 7 Robert Hogan 2013-05-09 11:39:35 PDT
Created attachment 201260 [details]
Patch
Comment 8 Build Bot 2013-05-09 16:25:30 PDT
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
Comment 9 Build Bot 2013-05-09 16:25:32 PDT
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
Comment 10 Build Bot 2013-05-11 20:54:51 PDT
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
Comment 11 Build Bot 2013-05-11 20:54:54 PDT
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
Comment 12 Robert Hogan 2013-05-13 14:02:18 PDT
Created attachment 201631 [details]
Patch
Comment 13 Build Bot 2013-05-13 17:52:41 PDT
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
Comment 14 Build Bot 2013-05-13 17:52:43 PDT
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
Comment 15 Build Bot 2013-05-13 19:31:20 PDT
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
Comment 16 Build Bot 2013-05-13 19:31:23 PDT
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
Comment 17 Robert Hogan 2013-05-14 13:17:17 PDT
Created attachment 201748 [details]
Patch
Comment 18 Robert Hogan 2013-05-14 13:26:17 PDT
Created attachment 201751 [details]
Patch
Comment 19 Dave Hyatt 2013-05-16 10:37:28 PDT
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().
Comment 20 Robert Hogan 2013-05-16 11:14:59 PDT
(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.
Comment 21 Robert Hogan 2013-05-16 11:33:50 PDT
Created attachment 201975 [details]
Patch
Comment 22 Dave Hyatt 2013-05-17 11:36:36 PDT
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.
Comment 23 Robert Hogan 2013-05-17 14:34:25 PDT
Created attachment 202158 [details]
Patch
Comment 24 Robert Hogan 2013-05-20 10:36:44 PDT
(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.
Comment 25 Dave Hyatt 2013-05-21 14:04:15 PDT
Comment on attachment 202158 [details]
Patch

r=me
Comment 26 WebKit Commit Bot 2013-05-22 11:59:14 PDT
Comment on attachment 202158 [details]
Patch

Clearing flags on attachment: 202158

Committed r150527: <http://trac.webkit.org/changeset/150527>
Comment 27 WebKit Commit Bot 2013-05-22 11:59:18 PDT
All reviewed patches have been landed.  Closing bug.