- Revision
- 257718
- Author
- [email protected]
- Date
- 2020-03-02 10:00:01 -0800 (Mon, 02 Mar 2020)
Log Message
[BFC][MarginCollapsing] Sometimes precomputed margin values are just plain wrong.
https://bugs.webkit.org/show_bug.cgi?id=208448
<rdar://problem/59950390>
Reviewed by Antti Koivisto.
Consider the following content:
<div style="float: left"></div>
<div style="height: 100px">
<div style="float: left"></div>
<div>
<span style="white-space: pre"> </span>
</div>
<div style="margin-bottom: 320px;"></div>
</div>
</div>
In order to compute the position of the second float we need be able to intersect it with all
the other floats in the same floating context. The float positioning starts with the box's static vertical position
which is the position the box would be taking if it wasn't floating positioned.
This vertical position needs to be in the same coordinate system as all the other floats to be able to intersect properly.
The coordinate system is the formatting root's content box.
When the second float box is being positioned, we don't yet have final margins for its ancestors, so the box's vertical
position at this point is only computed relative to its containing block.
In most cases we could just walk the ancestor chain all the way to the formatting root and pre-compute the margin before values.
However in some not-so-rare cases, the ancestor margin before value depends on some content after the float box and to be able
to figure out the exact margin values, we actually need to lay out the rest of the content.
In the example above, the second float's final position depends on whether the <span> has white-space: pre or not (whether the parent
div produces empty content or not).
This patch computes margin before values only for the ancestors ignoring margin collapsing (and margin after values).
* layout/MarginTypes.h:
(WebCore::Layout::PrecomputedMarginBefore::usedValue const):
* layout/blockformatting/BlockFormattingContext.cpp:
(WebCore::Layout::BlockFormattingContext::precomputeVerticalPositionForBoxAndAncestors):
(WebCore::Layout::BlockFormattingContext::computeHeightAndMargin):
(WebCore::Layout::BlockFormattingContext::setPrecomputedMarginBefore): Deleted.
(WebCore::Layout::BlockFormattingContext::hasPrecomputedMarginBefore const): Deleted.
* layout/blockformatting/BlockFormattingContext.h:
* layout/blockformatting/PrecomputedBlockMarginCollapse.cpp:
(WebCore::Layout::BlockFormattingContext::MarginCollapse::precomputedPositiveNegativeValues const):
(WebCore::Layout::BlockFormattingContext::MarginCollapse::precomputedPositiveNegativeMarginBefore const):
(WebCore::Layout::BlockFormattingContext::MarginCollapse::precomputedMarginBefore):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (257717 => 257718)
--- trunk/Source/WebCore/ChangeLog 2020-03-02 17:44:17 UTC (rev 257717)
+++ trunk/Source/WebCore/ChangeLog 2020-03-02 18:00:01 UTC (rev 257718)
@@ -1,3 +1,50 @@
+2020-03-02 Zalan Bujtas <[email protected]>
+
+ [BFC][MarginCollapsing] Sometimes precomputed margin values are just plain wrong.
+ https://bugs.webkit.org/show_bug.cgi?id=208448
+ <rdar://problem/59950390>
+
+ Reviewed by Antti Koivisto.
+
+ Consider the following content:
+ <div style="float: left"></div>
+ <div style="height: 100px">
+ <div style="float: left"></div>
+ <div>
+ <span style="white-space: pre"> </span>
+ </div>
+ <div style="margin-bottom: 320px;"></div>
+ </div>
+ </div>
+
+ In order to compute the position of the second float we need be able to intersect it with all
+ the other floats in the same floating context. The float positioning starts with the box's static vertical position
+ which is the position the box would be taking if it wasn't floating positioned.
+ This vertical position needs to be in the same coordinate system as all the other floats to be able to intersect properly.
+ The coordinate system is the formatting root's content box.
+ When the second float box is being positioned, we don't yet have final margins for its ancestors, so the box's vertical
+ position at this point is only computed relative to its containing block.
+ In most cases we could just walk the ancestor chain all the way to the formatting root and pre-compute the margin before values.
+ However in some not-so-rare cases, the ancestor margin before value depends on some content after the float box and to be able
+ to figure out the exact margin values, we actually need to lay out the rest of the content.
+ In the example above, the second float's final position depends on whether the <span> has white-space: pre or not (whether the parent
+ div produces empty content or not).
+
+ This patch computes margin before values only for the ancestors ignoring margin collapsing (and margin after values).
+
+ * layout/MarginTypes.h:
+ (WebCore::Layout::PrecomputedMarginBefore::usedValue const):
+ * layout/blockformatting/BlockFormattingContext.cpp:
+ (WebCore::Layout::BlockFormattingContext::precomputeVerticalPositionForBoxAndAncestors):
+ (WebCore::Layout::BlockFormattingContext::computeHeightAndMargin):
+ (WebCore::Layout::BlockFormattingContext::setPrecomputedMarginBefore): Deleted.
+ (WebCore::Layout::BlockFormattingContext::hasPrecomputedMarginBefore const): Deleted.
+ * layout/blockformatting/BlockFormattingContext.h:
+ * layout/blockformatting/PrecomputedBlockMarginCollapse.cpp:
+ (WebCore::Layout::BlockFormattingContext::MarginCollapse::precomputedPositiveNegativeValues const):
+ (WebCore::Layout::BlockFormattingContext::MarginCollapse::precomputedPositiveNegativeMarginBefore const):
+ (WebCore::Layout::BlockFormattingContext::MarginCollapse::precomputedMarginBefore):
+
2020-03-02 Dean Jackson <[email protected]>
maps.google.com is not loading properly, screen flickers when zooming
Modified: trunk/Source/WebCore/layout/MarginTypes.h (257717 => 257718)
--- trunk/Source/WebCore/layout/MarginTypes.h 2020-03-02 17:44:17 UTC (rev 257717)
+++ trunk/Source/WebCore/layout/MarginTypes.h 2020-03-02 18:00:01 UTC (rev 257718)
@@ -75,13 +75,6 @@
LayoutUnit end;
};
-struct PrecomputedMarginBefore {
- LayoutUnit usedValue() const { return collapsedValue.valueOr(nonCollapsedValue); }
- LayoutUnit nonCollapsedValue;
- Optional<LayoutUnit> collapsedValue;
- bool isCollapsedThrough { false };
-};
-
// FIXME: This structure might need to change to indicate that the cached value is not necessarily the same as the box's computed margin value.
// This only matters in case of collapse through margins when they collapse into another sibling box.
// <div style="margin: 1px"></div><div style="margin: 10px"></div> <- the second div's before/after marings collapse through and the same time they collapse into
@@ -100,6 +93,13 @@
Values after;
};
+struct PrecomputedMarginBefore {
+ LayoutUnit usedValue() const { return collapsedValue.valueOr(nonCollapsedValue); }
+ LayoutUnit nonCollapsedValue;
+ Optional<LayoutUnit> collapsedValue;
+ PositiveAndNegativeVerticalMargin::Values positiveAndNegativeMarginBefore;
+};
+
inline UsedVerticalMargin::UsedVerticalMargin(UsedVerticalMargin::NonCollapsedValues nonCollapsedValues, UsedVerticalMargin::CollapsedValues collapsedValues)
: m_nonCollapsedValues(nonCollapsedValues)
, m_collapsedValues(collapsedValues)
Modified: trunk/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp (257717 => 257718)
--- trunk/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp 2020-03-02 17:44:17 UTC (rev 257717)
+++ trunk/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp 2020-03-02 18:00:01 UTC (rev 257718)
@@ -274,10 +274,7 @@
//
// The idea here is that as long as we don't cross the block formatting context boundary, we should be able to pre-compute the final top position.
// FIXME: we currently don't account for the "clear" property when computing the final position for an ancestor.
- for (auto* ancestor = &layoutBox; ancestor && !ancestor->establishesBlockFormattingContext(); ancestor = ancestor->containingBlock()) {
- // FIXME: with incremental layout, we might actually have a valid (non-precomputed) margin top as well.
- if (hasPrecomputedMarginBefore(*ancestor))
- return;
+ for (auto* ancestor = &layoutBox; ancestor && ancestor != &root(); ancestor = ancestor->containingBlock()) {
auto horizontalConstraintsForAncestor = [&] {
auto* containingBlock = ancestor->containingBlock();
return containingBlock == &root() ? horizontalConstraints.root : Geometry::horizontalConstraintsForInFlow(geometryForBox(*containingBlock));
@@ -290,15 +287,16 @@
auto computedVerticalMargin = geometry().computedVerticalMargin(*ancestor, horizontalConstraintsForAncestor());
auto usedNonCollapsedMargin = UsedVerticalMargin::NonCollapsedValues { computedVerticalMargin.before.valueOr(0), computedVerticalMargin.after.valueOr(0) };
auto precomputedMarginBefore = marginCollapse().precomputedMarginBefore(*ancestor, usedNonCollapsedMargin);
- setPrecomputedMarginBefore(*ancestor, precomputedMarginBefore);
+ formattingState().setPositiveAndNegativeVerticalMargin(*ancestor, { precomputedMarginBefore.positiveAndNegativeMarginBefore, { } });
auto& displayBox = formattingState().displayBox(*ancestor);
auto nonCollapsedValues = UsedVerticalMargin::NonCollapsedValues { precomputedMarginBefore.nonCollapsedValue, { } };
- auto collapsedValues = UsedVerticalMargin::CollapsedValues { precomputedMarginBefore.collapsedValue, { }, precomputedMarginBefore.isCollapsedThrough };
+ auto collapsedValues = UsedVerticalMargin::CollapsedValues { precomputedMarginBefore.collapsedValue, { }, false };
auto verticalMargin = UsedVerticalMargin { nonCollapsedValues, collapsedValues };
displayBox.setVerticalMargin(verticalMargin);
displayBox.setTop(verticalPositionWithMargin(*ancestor, verticalMargin, verticalConstraintsForAncestor()));
#if ASSERT_ENABLED
+ setPrecomputedMarginBefore(*ancestor, precomputedMarginBefore);
displayBox.setHasPrecomputedMarginBefore();
#endif
}
@@ -438,8 +436,19 @@
return;
}
- ASSERT(!hasPrecomputedMarginBefore(layoutBox) || precomputedMarginBefore(layoutBox).usedValue() == verticalMargin.before());
- removePrecomputedMarginBefore(layoutBox);
+#if ASSERT_ENABLED
+ if (hasPrecomputedMarginBefore(layoutBox) && precomputedMarginBefore(layoutBox).usedValue() != verticalMargin.before()) {
+ // When the pre-computed margin turns out to be incorrect, we need to re-layout this subtree with the correct margin values.
+ // <div style="float: left"></div>
+ // <div>
+ // <div style="margin-bottom: 200px"></div>
+ // </div>
+ // The float box triggers margin before computation on the ancestor chain to be able to intersect with other floats in the same floating context.
+ // However in some cases the parent margin-top collapses with some next siblings (nephews) and there's no way to be able to properly
+ // account for that without laying out every node in the FC (in the example, the margin-bottom pushes down the float).
+ ASSERT_NOT_IMPLEMENTED_YET();
+ }
+#endif
auto& displayBox = formattingState().displayBox(layoutBox);
displayBox.setTop(verticalPositionWithMargin(layoutBox, verticalMargin, verticalConstraints));
displayBox.setContentBoxHeight(contentHeightAndMargin.contentHeight);
@@ -553,21 +562,7 @@
return containingBlockContentBoxTop + verticalMargin.before();
}
-void BlockFormattingContext::setPrecomputedMarginBefore(const Box& layoutBox, const PrecomputedMarginBefore& precomputedMarginBefore)
-{
- // Can't cross formatting context boundary.
- ASSERT(&layoutState().formattingStateForBox(layoutBox) == &formattingState());
- m_precomputedMarginBeforeList.set(&layoutBox, precomputedMarginBefore);
}
-
-bool BlockFormattingContext::hasPrecomputedMarginBefore(const Box& layoutBox) const
-{
- // Can't cross formatting context boundary.
- ASSERT(&layoutState().formattingStateForBox(layoutBox) == &formattingState());
- return m_precomputedMarginBeforeList.contains(&layoutBox);
}
-}
-}
-
#endif
Modified: trunk/Source/WebCore/layout/blockformatting/BlockFormattingContext.h (257717 => 257718)
--- trunk/Source/WebCore/layout/blockformatting/BlockFormattingContext.h 2020-03-02 17:44:17 UTC (rev 257717)
+++ trunk/Source/WebCore/layout/blockformatting/BlockFormattingContext.h 2020-03-02 18:00:01 UTC (rev 257718)
@@ -134,7 +134,7 @@
PositiveAndNegativeVerticalMargin::Values positiveNegativeMarginAfter(const Box&, UsedVerticalMargin::NonCollapsedValues) const;
PositiveAndNegativeVerticalMargin::Values precomputedPositiveNegativeMarginBefore(const Box&, UsedVerticalMargin::NonCollapsedValues) const;
- PositiveAndNegativeVerticalMargin::Values precomputedPositiveNegativeValues(const Box&, MarginType) const;
+ PositiveAndNegativeVerticalMargin::Values precomputedPositiveNegativeValues(const Box&) const;
PositiveAndNegativeVerticalMargin::Values computedPositiveAndNegativeMargin(PositiveAndNegativeVerticalMargin::Values, PositiveAndNegativeVerticalMargin::Values) const;
Optional<LayoutUnit> marginValue(PositiveAndNegativeVerticalMargin::Values) const;
@@ -167,17 +167,16 @@
};
BlockFormattingContext::Quirks quirks() const { return Quirks(*this); }
- void setPrecomputedMarginBefore(const Box&, const PrecomputedMarginBefore&);
- void removePrecomputedMarginBefore(const Box& layoutBox) { m_precomputedMarginBeforeList.remove(&layoutBox); }
- bool hasPrecomputedMarginBefore(const Box&) const;
Optional<LayoutUnit> usedAvailableWidthForFloatAvoider(const FloatingContext&, const Box&, const ConstraintsPair<HorizontalConstraints>&, const ConstraintsPair<VerticalConstraints>&);
-#if ASSERT_ENABLED
- PrecomputedMarginBefore precomputedMarginBefore(const Box& layoutBox) const { return m_precomputedMarginBeforeList.get(&layoutBox); }
-#endif
const BlockFormattingState& formattingState() const { return downcast<BlockFormattingState>(FormattingContext::formattingState()); }
BlockFormattingState& formattingState() { return downcast<BlockFormattingState>(FormattingContext::formattingState()); }
+#if ASSERT_ENABLED
+ void setPrecomputedMarginBefore(const Box& layoutBox, const PrecomputedMarginBefore& precomputedMarginBefore) { m_precomputedMarginBeforeList.set(&layoutBox, precomputedMarginBefore); }
+ PrecomputedMarginBefore precomputedMarginBefore(const Box& layoutBox) const { return m_precomputedMarginBeforeList.get(&layoutBox); }
+ bool hasPrecomputedMarginBefore(const Box& layoutBox) const { return m_precomputedMarginBeforeList.contains(&layoutBox); }
+#endif
private:
HashMap<const Box*, PrecomputedMarginBefore> m_precomputedMarginBeforeList;
};
Modified: trunk/Source/WebCore/layout/blockformatting/PrecomputedBlockMarginCollapse.cpp (257717 => 257718)
--- trunk/Source/WebCore/layout/blockformatting/PrecomputedBlockMarginCollapse.cpp 2020-03-02 17:44:17 UTC (rev 257717)
+++ trunk/Source/WebCore/layout/blockformatting/PrecomputedBlockMarginCollapse.cpp 2020-03-02 18:00:01 UTC (rev 257718)
@@ -36,14 +36,16 @@
namespace WebCore {
namespace Layout {
-PositiveAndNegativeVerticalMargin::Values BlockFormattingContext::MarginCollapse::precomputedPositiveNegativeValues(const Box& layoutBox, MarginType marginType) const
+PositiveAndNegativeVerticalMargin::Values BlockFormattingContext::MarginCollapse::precomputedPositiveNegativeValues(const Box& layoutBox) const
{
- auto computedVerticalMargin = formattingContext().geometry().computedVerticalMargin(layoutBox, Geometry::horizontalConstraintsForInFlow(formattingContext().geometryForBox(*layoutBox.containingBlock())));
- auto nonCollapsedMargin = UsedVerticalMargin::NonCollapsedValues { computedVerticalMargin.before.valueOr(0), computedVerticalMargin.after.valueOr(0) };
+ auto& blockFormattingState = downcast<BlockFormattingState>(layoutState().formattingStateForBox(layoutBox));
+ if (blockFormattingState.hasPositiveAndNegativeVerticalMargin(layoutBox))
+ return blockFormattingState.positiveAndNegativeVerticalMargin(layoutBox).before;
- if (marginType == MarginType::Before)
- return precomputedPositiveNegativeMarginBefore(layoutBox, nonCollapsedMargin);
- return positiveNegativeMarginAfter(layoutBox, nonCollapsedMargin);
+ auto horizontalConstraints = Geometry::horizontalConstraintsForInFlow(formattingContext().geometryForBox(*layoutBox.containingBlock()));
+ auto computedVerticalMargin = formattingContext().geometry().computedVerticalMargin(layoutBox, horizontalConstraints);
+ auto nonCollapsedMargin = UsedVerticalMargin::NonCollapsedValues { computedVerticalMargin.before.valueOr(0), computedVerticalMargin.after.valueOr(0) };
+ return precomputedPositiveNegativeMarginBefore(layoutBox, nonCollapsedMargin);
}
PositiveAndNegativeVerticalMargin::Values BlockFormattingContext::MarginCollapse::precomputedPositiveNegativeMarginBefore(const Box& layoutBox, UsedVerticalMargin::NonCollapsedValues nonCollapsedValues) const
@@ -51,13 +53,15 @@
auto firstChildCollapsedMarginBefore = [&]() -> PositiveAndNegativeVerticalMargin::Values {
if (!marginBeforeCollapsesWithFirstInFlowChildMarginBefore(layoutBox))
return { };
- return precomputedPositiveNegativeValues(*downcast<ContainerBox>(layoutBox).firstInFlowChild(), MarginType::Before);
+ return precomputedPositiveNegativeValues(*downcast<ContainerBox>(layoutBox).firstInFlowChild());
};
auto previouSiblingCollapsedMarginAfter = [&]() -> PositiveAndNegativeVerticalMargin::Values {
if (!marginBeforeCollapsesWithPreviousSiblingMarginAfter(layoutBox))
return { };
- return precomputedPositiveNegativeValues(*layoutBox.previousInFlowSibling(), MarginType::After);
+ auto& previousInFlowSibling = *layoutBox.previousInFlowSibling();
+ auto& blockFormattingState = downcast<BlockFormattingState>(layoutState().formattingStateForBox(previousInFlowSibling));
+ return blockFormattingState.positiveAndNegativeVerticalMargin(previousInFlowSibling).after;
};
// 1. Gather positive and negative margin values from first child if margins are adjoining.
@@ -83,13 +87,9 @@
ASSERT(layoutBox.isInFlow() || layoutBox.isFloatingPositioned());
ASSERT(!layoutBox.isReplacedBox());
- auto marginsCollapseThrough = this->marginsCollapseThrough(layoutBox);
auto positiveNegativeMarginBefore = precomputedPositiveNegativeMarginBefore(layoutBox, usedNonCollapsedMargin);
-
- auto collapsedMarginBefore = marginValue(!marginsCollapseThrough ? positiveNegativeMarginBefore
- : computedPositiveAndNegativeMargin(positiveNegativeMarginBefore, positiveNegativeMarginAfter(layoutBox, usedNonCollapsedMargin)));
-
- return { usedNonCollapsedMargin.before, collapsedMarginBefore, marginsCollapseThrough };
+ auto collapsedMarginBefore = marginValue(positiveNegativeMarginBefore);
+ return { usedNonCollapsedMargin.before, collapsedMarginBefore, positiveNegativeMarginBefore };
}
}
