- Revision
- 143284
- Author
- [email protected]
- Date
- 2013-02-18 18:01:53 -0800 (Mon, 18 Feb 2013)
Log Message
Padding and border changes don't trigger the relayout of children in some cases.
https://bugs.webkit.org/show_bug.cgi?id=109639.
Reviewed by Ryosuke Niwa.
Source/WebCore:
The fix for this bug was way too general and involved putting code into RenderBox. Since
RenderBox makes no assumptions about what kind of layout system might derive from it, it
was incorrect to just mark all children as needing layout whenever borders and padding
changed widths.
This patch takes the two cases handled by the original code and makes them more
specialized down in subclasses, i.e., RenderBlock and RenderTableRow. RenderBlock has
been refined to only check if children aren't inline and to also not invalidate
floats or irrelevant positioned objects that might not even have this block as their
containing block.
The RenderTableRow code is specialized to only care about collapsing borders and
to only check borders rather than padding. It also requires that a child be a cell
in order to do the invalidation.
Covered by existing tests, since this is just specializing the code to more precisely
cover the test cases that have already been written.
Longer term, it should be layout code that figures this stuff out rather than style
change code, but that involves more dramatic changes that can wait.
Test: fast/block/positioning/border-change-relayout-test.html
* rendering/RenderBlock.cpp:
(WebCore::borderOrPaddingLogicalWidthChanged):
(WebCore):
(WebCore::RenderBlock::styleDidChange):
* rendering/RenderBox.cpp:
(WebCore):
(WebCore::RenderBox::styleDidChange):
* rendering/RenderTableRow.cpp:
(WebCore::borderLogicalWidthChanged):
(WebCore):
(WebCore::RenderTableRow::styleDidChange):
LayoutTests:
* fast/block/positioning/border-change-relayout-test-expected.html: Added.
* fast/block/positioning/border-change-relayout-test.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (143283 => 143284)
--- trunk/LayoutTests/ChangeLog 2013-02-19 01:56:38 UTC (rev 143283)
+++ trunk/LayoutTests/ChangeLog 2013-02-19 02:01:53 UTC (rev 143284)
@@ -1,3 +1,13 @@
+2013-02-18 David Hyatt <[email protected]>
+
+ Padding and border changes don't trigger the relayout of children in some cases.
+ https://bugs.webkit.org/show_bug.cgi?id=109639.
+
+ Reviewed by Ryosuke Niwa.
+
+ * fast/block/positioning/border-change-relayout-test-expected.html: Added.
+ * fast/block/positioning/border-change-relayout-test.html: Added.
+
2013-02-18 Geoffrey Garen <[email protected]>
Shrank the SourceProvider cache
Added: trunk/LayoutTests/fast/block/positioning/border-change-relayout-test-expected.html (0 => 143284)
--- trunk/LayoutTests/fast/block/positioning/border-change-relayout-test-expected.html (rev 0)
+++ trunk/LayoutTests/fast/block/positioning/border-change-relayout-test-expected.html 2013-02-19 02:01:53 UTC (rev 143284)
@@ -0,0 +1,16 @@
+<!doctype html>
+<head>
+<style>
+* { -webkit-box-sizing: border-box; }
+</style>
+</head>
+<body>
+<div id="test" style="position:relative; width:100px;height:100px; border:5px solid black; background-color:red">
+ <div style="display:inline-block">
+ <div>
+ <div style="position:absolute;left:0;top:0;width:100%; height:100%;background-color:lime"></div>
+ </div>
+ </div>
+</div>
+</body>
+
Added: trunk/LayoutTests/fast/block/positioning/border-change-relayout-test.html (0 => 143284)
--- trunk/LayoutTests/fast/block/positioning/border-change-relayout-test.html (rev 0)
+++ trunk/LayoutTests/fast/block/positioning/border-change-relayout-test.html 2013-02-19 02:01:53 UTC (rev 143284)
@@ -0,0 +1,16 @@
+<!doctype html>
+<head>
+<style>
+* { -webkit-box-sizing: border-box; }
+</style>
+</head>
+<body _onload_="document.body.offsetWidth; document.getElementById('test').style.border = '5px solid black'">
+<div id="test" style="position:relative; width:100px;height:100px; border:10px solid blue; background-color:red">
+ <div style="display:inline-block">
+ <div>
+ <div style="position:absolute;left:0;top:0;width:100%; height:100%;background-color:lime"></div>
+ </div>
+ </div>
+</div>
+</body>
+
Modified: trunk/Source/WebCore/ChangeLog (143283 => 143284)
--- trunk/Source/WebCore/ChangeLog 2013-02-19 01:56:38 UTC (rev 143283)
+++ trunk/Source/WebCore/ChangeLog 2013-02-19 02:01:53 UTC (rev 143284)
@@ -1,3 +1,45 @@
+2013-02-18 David Hyatt <[email protected]>
+
+ Padding and border changes don't trigger the relayout of children in some cases.
+ https://bugs.webkit.org/show_bug.cgi?id=109639.
+
+ Reviewed by Ryosuke Niwa.
+
+ The fix for this bug was way too general and involved putting code into RenderBox. Since
+ RenderBox makes no assumptions about what kind of layout system might derive from it, it
+ was incorrect to just mark all children as needing layout whenever borders and padding
+ changed widths.
+
+ This patch takes the two cases handled by the original code and makes them more
+ specialized down in subclasses, i.e., RenderBlock and RenderTableRow. RenderBlock has
+ been refined to only check if children aren't inline and to also not invalidate
+ floats or irrelevant positioned objects that might not even have this block as their
+ containing block.
+
+ The RenderTableRow code is specialized to only care about collapsing borders and
+ to only check borders rather than padding. It also requires that a child be a cell
+ in order to do the invalidation.
+
+ Covered by existing tests, since this is just specializing the code to more precisely
+ cover the test cases that have already been written.
+
+ Longer term, it should be layout code that figures this stuff out rather than style
+ change code, but that involves more dramatic changes that can wait.
+
+ Test: fast/block/positioning/border-change-relayout-test.html
+
+ * rendering/RenderBlock.cpp:
+ (WebCore::borderOrPaddingLogicalWidthChanged):
+ (WebCore):
+ (WebCore::RenderBlock::styleDidChange):
+ * rendering/RenderBox.cpp:
+ (WebCore):
+ (WebCore::RenderBox::styleDidChange):
+ * rendering/RenderTableRow.cpp:
+ (WebCore::borderLogicalWidthChanged):
+ (WebCore):
+ (WebCore::RenderTableRow::styleDidChange):
+
2013-02-18 Mark Lam <[email protected]>
Small follow up to r143271: Fix SQLTransaction leak.
Modified: trunk/Source/WebCore/rendering/RenderBlock.cpp (143283 => 143284)
--- trunk/Source/WebCore/rendering/RenderBlock.cpp 2013-02-19 01:56:38 UTC (rev 143283)
+++ trunk/Source/WebCore/rendering/RenderBlock.cpp 2013-02-19 02:01:53 UTC (rev 143284)
@@ -337,14 +337,30 @@
RenderBox::styleWillChange(diff, newStyle);
}
+static bool borderOrPaddingLogicalWidthChanged(const RenderStyle* oldStyle, const RenderStyle* newStyle)
+{
+ if (newStyle->isHorizontalWritingMode())
+ return oldStyle->borderLeftWidth() != newStyle->borderLeftWidth()
+ || oldStyle->borderRightWidth() != newStyle->borderRightWidth()
+ || oldStyle->paddingLeft() != newStyle->paddingLeft()
+ || oldStyle->paddingRight() != newStyle->paddingRight();
+
+ return oldStyle->borderTopWidth() != newStyle->borderTopWidth()
+ || oldStyle->borderBottomWidth() != newStyle->borderBottomWidth()
+ || oldStyle->paddingTop() != newStyle->paddingTop()
+ || oldStyle->paddingBottom() != newStyle->paddingBottom();
+}
+
void RenderBlock::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
{
RenderBox::styleDidChange(diff, oldStyle);
-
+
+ RenderStyle* newStyle = style();
+
#if ENABLE(CSS_EXCLUSIONS)
// FIXME: Bug 89993: Style changes should affect the ExclusionShapeInsideInfos for other render blocks that
// share the same ExclusionShapeInsideInfo
- updateExclusionShapeInsideInfoAfterStyleChange(style()->resolvedShapeInside(), oldStyle ? oldStyle->resolvedShapeInside() : 0);
+ updateExclusionShapeInsideInfoAfterStyleChange(newStyle->resolvedShapeInside(), oldStyle ? oldStyle->resolvedShapeInside() : 0);
#endif
if (!isAnonymousBlock()) {
@@ -352,7 +368,7 @@
for (RenderBlock* currCont = blockElementContinuation(); currCont; currCont = currCont->blockElementContinuation()) {
RenderBoxModelObject* nextCont = currCont->continuation();
currCont->setContinuation(0);
- currCont->setStyle(style());
+ currCont->setStyle(newStyle);
currCont->setContinuation(nextCont);
}
}
@@ -389,6 +405,38 @@
parentBlock->markAllDescendantsWithFloatsForLayout();
parentBlock->markSiblingsWithFloatsForLayout();
}
+
+ // It's possible for our border/padding to change, but for the overall logical width of the block to
+ // end up being the same. When this happens, we won't understand to set |relayoutChildren| to true
+ // in layoutBlock. Longer-term, we should be figuring this stuff out over in layoutBlock, since
+ // we need to be able to handle the same situation in RenderRegions when we have distinct borders in
+ // those regions. For now, though, we just mark the children for relayout right here.
+ if (oldStyle && diff == StyleDifferenceLayout && needsLayout() && borderOrPaddingLogicalWidthChanged(oldStyle, newStyle)) {
+ // First walk our normal flow objects and see if there is any change.
+ if (!childrenInline()) {
+ for (RenderBox* childBox = firstChildBox(); childBox; childBox = childBox->nextSiblingBox()) {
+ // Positioned objects are checked below, since we only care about those objects for which we
+ // are the containing block. We skip floats completely, since this matches the behavior of
+ // the |relayoutChildren| boolean in layoutBlock. It is entirely possible there are bugs
+ // related to floats here, but for now we'll be consistent with layoutBlock, since those
+ // bugs would exist even without border/padding changes.
+ if (childBox->isFloatingOrOutOfFlowPositioned())
+ continue;
+ childBox->setChildNeedsLayout(true, MarkOnlyThis);
+ }
+ }
+
+ // Now walk the positioned objects for which we are the containing block.
+ TrackedRendererListHashSet* positionedDescendants = positionedObjects();
+ if (positionedDescendants) {
+ RenderBox* positionedBox;
+ TrackedRendererListHashSet::iterator end = positionedDescendants->end();
+ for (TrackedRendererListHashSet::iterator it = positionedDescendants->begin(); it != end; ++it) {
+ positionedBox = *it;
+ positionedBox->setChildNeedsLayout(true, MarkContainingBlockChain);
+ }
+ }
+ }
}
RenderBlock* RenderBlock::continuationBefore(RenderObject* beforeChild)
Modified: trunk/Source/WebCore/rendering/RenderBox.cpp (143283 => 143284)
--- trunk/Source/WebCore/rendering/RenderBox.cpp 2013-02-19 01:56:38 UTC (rev 143283)
+++ trunk/Source/WebCore/rendering/RenderBox.cpp 2013-02-19 02:01:53 UTC (rev 143284)
@@ -232,20 +232,6 @@
RenderBoxModelObject::styleWillChange(diff, newStyle);
}
-static bool borderOrPaddingLogicalWidthChanged(const RenderStyle* oldStyle, const RenderStyle* newStyle)
-{
- if (newStyle->isHorizontalWritingMode())
- return oldStyle->borderLeftWidth() != newStyle->borderLeftWidth()
- || oldStyle->borderRightWidth() != newStyle->borderRightWidth()
- || oldStyle->paddingLeft() != newStyle->paddingLeft()
- || oldStyle->paddingRight() != newStyle->paddingRight();
-
- return oldStyle->borderTopWidth() != newStyle->borderTopWidth()
- || oldStyle->borderBottomWidth() != newStyle->borderBottomWidth()
- || oldStyle->paddingTop() != newStyle->paddingTop()
- || oldStyle->paddingBottom() != newStyle->paddingBottom();
-}
-
void RenderBox::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
{
// Horizontal writing mode definition is updated in RenderBoxModelObject::updateFromStyle,
@@ -319,11 +305,6 @@
#if ENABLE(CSS_EXCLUSIONS)
updateExclusionShapeOutsideInfoAfterStyleChange(style()->shapeOutside(), oldStyle ? oldStyle->shapeOutside() : 0);
#endif
-
- if (oldStyle && diff == StyleDifferenceLayout && needsLayout() && borderOrPaddingLogicalWidthChanged(oldStyle, newStyle)) {
- for (RenderObject* child = firstChild(); child; child = child->nextSibling())
- child->setChildNeedsLayout(true, MarkOnlyThis);
- }
}
#if ENABLE(CSS_EXCLUSIONS)
Modified: trunk/Source/WebCore/rendering/RenderTableRow.cpp (143283 => 143284)
--- trunk/Source/WebCore/rendering/RenderTableRow.cpp 2013-02-19 01:56:38 UTC (rev 143283)
+++ trunk/Source/WebCore/rendering/RenderTableRow.cpp 2013-02-19 02:01:53 UTC (rev 143284)
@@ -54,6 +54,16 @@
section()->setNeedsCellRecalc();
}
+static bool borderLogicalWidthChanged(const RenderStyle* oldStyle, const RenderStyle* newStyle)
+{
+ if (newStyle->isHorizontalWritingMode())
+ return oldStyle->borderLeftWidth() != newStyle->borderLeftWidth()
+ || oldStyle->borderRightWidth() != newStyle->borderRightWidth();
+
+ return oldStyle->borderTopWidth() != newStyle->borderTopWidth()
+ || oldStyle->borderBottomWidth() != newStyle->borderBottomWidth();
+}
+
void RenderTableRow::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
{
ASSERT(style()->display() == TABLE_ROW);
@@ -69,6 +79,17 @@
RenderTable* table = this->table();
if (table && !table->selfNeedsLayout() && !table->normalChildNeedsLayout() && oldStyle && oldStyle->border() != style()->border())
table->invalidateCollapsedBorders();
+
+ if (table && oldStyle && diff == StyleDifferenceLayout && needsLayout() && table->collapseBorders() && borderLogicalWidthChanged(oldStyle, style())) {
+ // If the border width changes on a row, we need to make sure the cells in the row know to lay out again.
+ // This only happens when borders are collapsed, since they end up affecting the border sides of the cell
+ // itself.
+ for (RenderBox* childBox = firstChildBox(); childBox; childBox = childBox->nextSiblingBox()) {
+ if (!childBox->isTableCell())
+ continue;
+ childBox->setChildNeedsLayout(true, MarkOnlyThis);
+ }
+ }
}
}
