Bug 109639 – Padding and border changes doesn't trigger relayout of children

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

Padding and border changes doesn't trigger relayout of children

[Tony Chang]

Created attachment 187976 [details]
Patch

[Tony Chang]

fast/table/border-collapsing/cached-change-row-border-width.html failed on Chromium Linux, looking to see if it fails on Mac too.

[WebKit Review Bot]

Comment on attachment 187976 [details]
Patch

Attachment 187976 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16517457

New failing tests:
fast/table/border-collapsing/cached-change-row-border-width.html

[Build Bot]

Comment on attachment 187976 [details]
Patch

Attachment 187976 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16454761

New failing tests:
fast/table/border-collapsing/cached-change-row-border-width.html

[Build Bot]

Comment on attachment 187976 [details]
Patch

Attachment 187976 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16546168

New failing tests:
fast/table/border-collapsing/cached-change-row-border-width.html

[Tony Chang]

Created attachment 188189 [details]
Patch

[Tony Chang]

This patch also seems to fix bug 104864.  This makes me think that http://trac.webkit.org/changeset/129644 revealed this layout bug by doing fewer style recalcs.

[Kent Tamura]

Comment on attachment 188189 [details]
Patch

Looks good

[WebKit Review Bot]

Comment on attachment 188189 [details]
Patch

Clearing flags on attachment: 188189

Committed r142889: <http://trac.webkit.org/changeset/142889>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

*** Bug 104864 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

This patch appears to have caused an assertion failure on scrollbars/overflow-scrollbar-combinations.html:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=scrollbars%2Foverflow-scrollbar-combinations.html

Blame list:
http://trac.webkit.org/log/?verbose=on&rev=142890&stop_rev=142888

ASSERTION FAILED: needsLayout()
/Volumes/Data/slave/mountainlion-debug/build/Source/WebCore/rendering/RenderBox.cpp(324) : virtual void WebCore::RenderBox::styleDidChange(WebCore::StyleDifference, const WebCore::RenderStyle *)
1   0x10d654c4a WebCore::RenderBox::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*)
2   0x10d5ca6d3 WebCore::RenderBlock::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*)
3   0x10d7b94fd WebCore::RenderScrollbarPart::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*)
4   0x10d78cf39 WebCore::RenderObject::setStyle(WTF::PassRefPtr<WebCore::RenderStyle>)
5   0x10d7b3d5e WebCore::RenderScrollbar::updateScrollbarPart(WebCore::ScrollbarPart, bool)
6   0x10d7b3ef1 WebCore::RenderScrollbar::updateScrollbarParts(bool)
7   0x10d7b43aa WebCore::RenderScrollbar::styleChanged()
8   0x10d70786c WebCore::RenderLayer::setHasVerticalScrollbar(bool)
9   0x10d714484 WebCore::RenderLayer::updateScrollbarsAfterStyleChange(WebCore::RenderStyle const*)
10  0x10d7149b1 WebCore::RenderLayer::styleChanged(WebCore::StyleDifference, WebCore::RenderStyle const*)
11  0x10d74e33e WebCore::RenderLayerModelObject::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*)
12  0x10d654526 WebCore::RenderBox::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*)
13  0x10d5ca6d3 WebCore::RenderBlock::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*)
14  0x10d78cf39 WebCore::RenderObject::setStyle(WTF::PassRefPtr<WebCore::RenderStyle>)
15  0x10d78c47a WebCore::RenderObject::setAnimatableStyle(WTF::PassRefPtr<WebCore::RenderStyle>)
16  0x10d4fa0d6 WebCore::NodeRenderingContext::createRendererForElementIfNeeded()
17  0x10c89298d WebCore::Element::createRendererIfNeeded()
18  0x10c8929e5 WebCore::Element::attach()
19  0x10cb198bc WebCore::executeTask(WebCore::HTMLConstructionSiteTask&)
20  0x10cb19746 WebCore::HTMLConstructionSite::executeQueuedTasks()
21  0x10cc01075 WebCore::HTMLTreeBuilder::constructTree(WebCore::AtomicHTMLToken*)
22  0x10cb3a191 WebCore::HTMLDocumentParser::constructTreeFromHTMLToken(WebCore::HTMLToken&)
23  0x10cb39949 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode)
24  0x10cb39110 WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode)
25  0x10cb3a650 WebCore::HTMLDocumentParser::append(WebCore::SegmentedString const&)
26  0x10c644da7 WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter*, char const*, unsigned long)
27  0x10c716963 WebCore::DocumentWriter::addData(char const*, unsigned long)
28  0x10c6dcc87 WebCore::DocumentLoader::commitData(char const*, unsigned long)
29  0x10a012371 WebKit::WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int)
30  0x10c6dcd60 WebCore::DocumentLoader::commitLoad(char const*, int)
31  0x10c6dd30b WebCore::DocumentLoader::receivedData(char const*, int)

[WebKit Review Bot]

Re-opened since this is blocked by bug 109891

[Tony Chang]

Created attachment 188602 [details]
Patch

[Tony Chang]

Comment on attachment 188602 [details]
Patch

Fixed the ASSERT by moving the check into the if statement. See ChangeLog for more details.

[WebKit Review Bot]

Comment on attachment 188602 [details]
Patch

Clearing flags on attachment: 188602

Committed r143092: <http://trac.webkit.org/changeset/143092>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dave Hyatt]

I'm not happy with this patch, or with the original code that was added here. Can we talk in #webkit on Monday about a better approach? I'm guessing you ended up here because you weren't quite sure how to patch layoutBlock, but RenderBox is way too general a spot for this kind of code (and this code is going to trigger relayouts of children in situations where it wasn't necessary to do so).

[Dave Hyatt]

Reopening this to code a more specialized fix that doesn't do relayouts it shouldn't.

[Dave Hyatt]

Created attachment 188940 [details]
Patch

[Dave Hyatt]

Created attachment 188941 [details]
Patch

Add a couple of null checks.

[WebKit Review Bot]

Comment on attachment 188941 [details]
Patch

Attachment 188941 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16610442

New failing tests:
fast/regions/seamless-iframe-flowed-into-regions.html

[Ryosuke Niwa]

Comment on attachment 188941 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:420
> +        // First walk our normal flow objects and see if there is any change.
> +        for (RenderBox* childBox = firstChildBox(); childBox; childBox = childBox->nextSiblingBox()) {
> +            if (childBox->isFloatingOrOutOfFlowPositioned())
> +                continue;
> +            childBox->setChildNeedsLayout(true, MarkOnlyThis);
> +        }

I would be good to mention why we don't have to set floating objects dirty per IRC discussion.

> Source/WebCore/rendering/RenderBlock.cpp:423
> +        // Now walk the positioned objects for which we are the containing block.
> +        TrackedRendererListHashSet* positionedDescendants = positionedObjects();

It would be great if you could come up with a test case for floating or position objects.

> Source/WebCore/rendering/RenderTableRow.cpp:89
> +                if (!childBox->isTableCell())
> +                    continue;

On a completely unrelated note, it's weird that we can ever have a non-RenderTableCell RenderObject in RenderTableRow.

[Dave Hyatt]

Created attachment 188954 [details]
Patch with a positioning test case

I added a positioning test case that illustrates what was wrong with the RenderBox code and why you need RenderBlock-specific code to fix this bug the right way.

[Ryosuke Niwa]

Comment on attachment 188954 [details]
Patch with a positioning test case

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

> Source/WebCore/ChangeLog:27
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the long description but after the bug url.

> Source/WebCore/ChangeLog:30
> +        Added fast/block/positioning/border-change-relayout-test.html to illustrate why the
> +        old patch didn't work.

Please use the de-factor standard format:
Test: fast/block/positioning/border-change-relayout-test.html

[Dave Hyatt]

Fixed in r143284.

[Tony Chang]

(In reply to comment #18)
> I'm not happy with this patch, or with the original code that was added here. Can we talk in #webkit on Monday about a better approach? I'm guessing you ended up here because you weren't quite sure how to patch layoutBlock, but RenderBox is way too general a spot for this kind of code (and this code is going to trigger relayouts of children in situations where it wasn't necessary to do so).

Thanks for fixing!  Sorry, I was OOO yesterday due to President's Day.