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.
<rdar://problem/13822704>
The behavior of this testcase also changed in http://trac.webkit.org/changeset/142152
Created attachment 200937 [details] Reduction toggling floats off and then on again
Created attachment 200938 [details] Reduction toggling floats off and then on again
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.
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.
Created attachment 201260 [details] Patch
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
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 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
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
Created attachment 201631 [details] Patch
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
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 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
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
Created attachment 201748 [details] Patch
Created attachment 201751 [details] Patch
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().
(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.
Created attachment 201975 [details] Patch
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.
Created attachment 202158 [details] Patch
(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 on attachment 202158 [details] Patch r=me
Comment on attachment 202158 [details] Patch Clearing flags on attachment: 202158 Committed r150527: <http://trac.webkit.org/changeset/150527>
All reviewed patches have been landed. Closing bug.