WebKit Bugzilla
Attachment 360116 Details for
Bug 193824
: [LFC][BFC][MarginCollapsing] Add "clear" to static position computation.
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
Patch.txt (text/plain), 17.36 KB, created by
zalan
on 2019-01-25 08:07:33 PST
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
zalan
Created:
2019-01-25 08:07:33 PST
Size:
17.36 KB
patch
obsolete
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog >index d158b70c90a..b8a9a0deb70 100644 >--- a/Source/WebCore/ChangeLog >+++ b/Source/WebCore/ChangeLog >@@ -1,3 +1,36 @@ >+2019-01-25 Zalan Bujtas <zalan@apple.com> >+ >+ [LFC][BFC][MarginCollapsing] Add "clear" to static position computation. >+ https://bugs.webkit.org/show_bug.cgi?id=193824 >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ When clear property is set and floats are present, we have to estimate and set the box's vertical position during >+ static positioning to be able to properly layout its subtree. >+ >+ <div style="float: left; width: 100px; height: 100px;"></div> >+ <div style="clear: left;"> >+ <div style="float: left; width: 100px; height: 100px;"></div> >+ </div> >+ >+ In the above example since the second float's parent clears the first float, the second float is positioned below >+ the first float. If we didn't push down (clear) the box, the float child would get placed next to the first float. >+ >+ * layout/blockformatting/BlockFormattingContext.cpp: >+ (WebCore::Layout::BlockFormattingContext::layout const): >+ (WebCore::Layout::BlockFormattingContext::layoutFormattingContextRoot const): >+ (WebCore::Layout::BlockFormattingContext::computeStaticPosition const): >+ (WebCore::Layout::BlockFormattingContext::computeEstimatedVerticalPosition const): >+ (WebCore::Layout::BlockFormattingContext::computeEstimatedVerticalPositionForFloatClear const): >+ (WebCore::Layout::BlockFormattingContext::computeHeightAndMargin const): >+ (WebCore::Layout::BlockFormattingContext::verticalPositionWithMargin const): >+ (WebCore::Layout::BlockFormattingContext::computeVerticalPositionForFloatClear const): Deleted. >+ (WebCore::Layout::BlockFormattingContext::adjustedVerticalPositionAfterMarginCollapsing const): Deleted. >+ * layout/blockformatting/BlockFormattingContext.h: >+ * layout/blockformatting/BlockMarginCollapse.cpp: >+ (WebCore::Layout::BlockFormattingContext::MarginCollapse::estimatedMarginBefore): >+ * layout/displaytree/DisplayBox.h: >+ > 2019-01-24 Zalan Bujtas <zalan@apple.com> > > [LFC][BFC][MarginCollapsing] Move positive/negative margin value updating to a dedicated function >diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp b/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp >index 7fc4ebc358d..0f9c1b746ad 100644 >--- a/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp >+++ b/Source/WebCore/layout/blockformatting/BlockFormattingContext.cpp >@@ -90,7 +90,7 @@ void BlockFormattingContext::layout() const > LOG_WITH_STREAM(FormattingContextLayout, stream << "[Compute] -> [Position][Border][Padding][Width][Margin] -> for layoutBox(" << &layoutBox << ")"); > computeBorderAndPadding(layoutBox); > computeWidthAndMargin(layoutBox); >- computeStaticPosition(layoutBox); >+ computeStaticPosition(floatingContext, layoutBox); > if (!is<Container>(layoutBox) || !downcast<Container>(layoutBox).hasInFlowOrFloatingChild()) > break; > layoutQueue.append(downcast<Container>(layoutBox).firstInFlowOrFloatingChild()); >@@ -105,9 +105,6 @@ void BlockFormattingContext::layout() const > // Formatting root boxes are special-cased and they don't come here. > ASSERT(!layoutBox.establishesFormattingContext()); > computeHeightAndMargin(layoutBox); >- // Finalize position with clearance. >- if (layoutBox.hasFloatClear()) >- computeVerticalPositionForFloatClear(floatingContext, layoutBox); > if (!is<Container>(layoutBox)) > continue; > auto& container = downcast<Container>(layoutBox); >@@ -130,7 +127,7 @@ void BlockFormattingContext::layoutFormattingContextRoot(FloatingContext& floati > LOG_WITH_STREAM(FormattingContextLayout, stream << "[Compute] -> [Position][Border][Padding][Width][Margin] -> for layoutBox(" << &layoutBox << ")"); > computeBorderAndPadding(layoutBox); > computeWidthAndMargin(layoutBox); >- computeStaticPosition(layoutBox); >+ computeStaticPosition(floatingContext, layoutBox); > // Swich over to the new formatting context (the one that the root creates). > auto formattingContext = layoutState().createFormattingContext(layoutBox); > formattingContext->layout(); >@@ -143,9 +140,7 @@ void BlockFormattingContext::layoutFormattingContextRoot(FloatingContext& floati > if (layoutBox.isFloatingPositioned()) { > computeFloatingPosition(floatingContext, layoutBox); > floatingContext.floatingState().append(layoutBox); >- } else if (layoutBox.hasFloatClear()) >- computeVerticalPositionForFloatClear(floatingContext, layoutBox); >- else if (layoutBox.establishesBlockFormattingContext()) >+ } else if (layoutBox.establishesBlockFormattingContext()) > computePositionToAvoidFloats(floatingContext, layoutBox); > > // Now that we computed the root's height, we can go back and layout the out-of-flow descedants (if any). >@@ -176,12 +171,12 @@ void BlockFormattingContext::placeInFlowPositionedChildren(const Container& cont > LOG_WITH_STREAM(FormattingContextLayout, stream << "End: move in-flow positioned children -> parent: " << &container); > } > >-void BlockFormattingContext::computeStaticPosition(const Box& layoutBox) const >+void BlockFormattingContext::computeStaticPosition(const FloatingContext& floatingContext, const Box& layoutBox) const > { > auto& layoutState = this->layoutState(); > layoutState.displayBoxForLayoutBox(layoutBox).setTopLeft(Geometry::staticPosition(layoutState, layoutBox)); > if (layoutBox.hasFloatClear()) >- computeEstimatedVerticalPositionForFloatClear(layoutBox); >+ computeEstimatedVerticalPositionForFloatClear(floatingContext, layoutBox); > else if (layoutBox.establishesFormattingContext()) > computeEstimatedVerticalPositionForFormattingRoot(layoutBox); > } >@@ -197,7 +192,7 @@ void BlockFormattingContext::computeEstimatedVerticalPosition(const Box& layoutB > auto collapsedValues = UsedVerticalMargin::CollapsedValues { estimatedMarginBefore.collapsedValue, { }, estimatedMarginBefore.isCollapsedThrough }; > auto verticalMargin = UsedVerticalMargin { nonCollapsedValues, collapsedValues }; > displayBox.setVerticalMargin(verticalMargin); >- displayBox.setTop(adjustedVerticalPositionAfterMarginCollapsing(layoutBox, verticalMargin)); >+ displayBox.setTop(verticalPositionWithMargin(layoutBox, verticalMargin)); > #if !ASSERT_DISABLED > displayBox.setHasEstimatedMarginBefore(); > #endif >@@ -243,8 +238,25 @@ void BlockFormattingContext::computeEstimatedVerticalPositionForFormattingRoot(c > } > } > >-void BlockFormattingContext::computeEstimatedVerticalPositionForFloatClear(const Box&) const >+void BlockFormattingContext::computeEstimatedVerticalPositionForFloatClear(const FloatingContext& floatingContext, const Box& layoutBox) const > { >+ ASSERT(layoutBox.hasFloatClear()); >+ if (floatingContext.floatingState().isEmpty()) >+ return; >+ // The static position with clear requires margin esitmation to see if clearance is needed. >+ computeEstimatedVerticalPosition(layoutBox); >+ computeEstimatedVerticalPositionForAncestors(layoutBox); >+ auto verticalPositionAndClearance = floatingContext.verticalPositionWithClearance(layoutBox); >+ if (!verticalPositionAndClearance.position) { >+ ASSERT(!verticalPositionAndClearance.clearance); >+ return; >+ } >+ >+ auto& displayBox = layoutState().displayBoxForLayoutBox(layoutBox); >+ ASSERT(*verticalPositionAndClearance.position >= displayBox.top()); >+ displayBox.setTop(*verticalPositionAndClearance.position); >+ if (verticalPositionAndClearance.clearance) >+ displayBox.setHasClearance(); > } > > #ifndef NDEBUG >@@ -292,54 +304,6 @@ void BlockFormattingContext::computePositionToAvoidFloats(const FloatingContext& > layoutState.displayBoxForLayoutBox(layoutBox).setTopLeft(*adjustedPosition); > } > >-void BlockFormattingContext::computeVerticalPositionForFloatClear(const FloatingContext& floatingContext, const Box& layoutBox) const >-{ >- ASSERT(layoutBox.hasFloatClear()); >- if (floatingContext.floatingState().isEmpty()) >- return; >- >- // For formatting roots, we already precomputed final position. >- if (!layoutBox.establishesFormattingContext()) >- computeEstimatedVerticalPositionForAncestors(layoutBox); >- ASSERT(hasPrecomputedMarginBefore(layoutBox)); >- >- // 1. Compute and adjust the vertical position with clearance. >- // 2. Reset margin collapsing with previous in-flow sibling if clerance is present. >- auto verticalPositionAndClearance = floatingContext.verticalPositionWithClearance(layoutBox); >- if (!verticalPositionAndClearance.position) >- return; >- >- // Adjust vertical position first. >- auto& layoutState = this->layoutState(); >- auto& displayBox = layoutState.displayBoxForLayoutBox(layoutBox); >- displayBox.setTop(*verticalPositionAndClearance.position); >- if (!verticalPositionAndClearance.clearance) >- return; >- >- displayBox.setHasClearance(); >- // Adjust margin collapsing with clearance. >- auto* previousInFlowSibling = layoutBox.previousInFlowSibling(); >- if (!previousInFlowSibling) >- return; >- >- // Clearance inhibits margin collapsing. Let's reset the relevant adjoining margins. >- // Does this box with clearance actually collapse its margin before with the previous inflow box's margin after? >- auto verticalMargin = displayBox.verticalMargin(); >- if (!verticalMargin.hasCollapsedValues() || !verticalMargin.collapsedValues().before) >- return; >- >- // Reset previous after and current before margin values to non-collapsing. >- auto& previousInFlowDisplayBox = layoutState.displayBoxForLayoutBox(*previousInFlowSibling); >- auto previousVerticalMargin = previousInFlowDisplayBox.verticalMargin(); >- ASSERT(previousVerticalMargin.hasCollapsedValues() && previousVerticalMargin.collapsedValues().after); >- >- previousVerticalMargin.setCollapsedValues({ previousVerticalMargin.collapsedValues().before, { } }); >- verticalMargin.setCollapsedValues({ { }, verticalMargin.collapsedValues().after }); >- >- previousInFlowDisplayBox.setVerticalMargin(previousVerticalMargin); >- displayBox.setVerticalMargin(verticalMargin); >-} >- > void BlockFormattingContext::computeWidthAndMargin(const Box& layoutBox) const > { > auto& layoutState = this->layoutState(); >@@ -428,7 +392,7 @@ void BlockFormattingContext::computeHeightAndMargin(const Box& layoutBox) const > > ASSERT(!hasEstimatedMarginBefore(layoutBox) || estimatedMarginBefore(layoutBox).usedValue() == verticalMargin.before()); > removeEstimatedMarginBefore(layoutBox); >- displayBox.setTop(adjustedVerticalPositionAfterMarginCollapsing(layoutBox, verticalMargin)); >+ displayBox.setTop(verticalPositionWithMargin(layoutBox, verticalMargin)); > displayBox.setContentBoxHeight(heightAndMargin.height); > displayBox.setVerticalMargin(verticalMargin); > >@@ -501,14 +465,19 @@ FormattingContext::InstrinsicWidthConstraints BlockFormattingContext::instrinsic > return instrinsicWidthConstraints; > } > >-LayoutUnit BlockFormattingContext::adjustedVerticalPositionAfterMarginCollapsing(const Box& layoutBox, const UsedVerticalMargin& verticalMargin) const >+LayoutUnit BlockFormattingContext::verticalPositionWithMargin(const Box& layoutBox, const UsedVerticalMargin& verticalMargin) const > { > ASSERT(!layoutBox.isOutOfFlowPositioned()); >- // Now that we've computed the final margin before, let's shift the box's vertical position. >- // 1. Check if the margin before collapses with the previous box's margin after. if not -> return previous box's bottom including margin after + marginBefore >- // 2. Check if the previous box's margins collapse through. If not -> return previous box' bottom excluding margin after + marginBefore (they are supposed to be equal) >- // 3. Go to previous box and start from step #1 until we hit the parent box. >+ // Now that we've computed the final margin before, let's shift the box's vertical position if needed. >+ // 1. Check if the box has clearance. If so, we've already precomputed/finalized the top value and vertical margin does not impact it anymore. >+ // 2. Check if the margin before collapses with the previous box's margin after. if not -> return previous box's bottom including margin after + marginBefore >+ // 3. Check if the previous box's margins collapse through. If not -> return previous box' bottom excluding margin after + marginBefore (they are supposed to be equal) >+ // 4. Go to previous box and start from step #1 until we hit the parent box. > auto& layoutState = this->layoutState(); >+ auto& displayBox = layoutState.displayBoxForLayoutBox(layoutBox); >+ if (displayBox.hasClearance()) >+ return displayBox.top(); >+ > auto* currentLayoutBox = &layoutBox; > while (currentLayoutBox) { > if (!currentLayoutBox->previousInFlowSibling()) >diff --git a/Source/WebCore/layout/blockformatting/BlockFormattingContext.h b/Source/WebCore/layout/blockformatting/BlockFormattingContext.h >index 382b2a2e2ea..572ca265d9e 100644 >--- a/Source/WebCore/layout/blockformatting/BlockFormattingContext.h >+++ b/Source/WebCore/layout/blockformatting/BlockFormattingContext.h >@@ -58,18 +58,17 @@ private: > void computeWidthAndMargin(const Box&) const; > void computeHeightAndMargin(const Box&) const; > >- void computeStaticPosition(const Box&) const; >+ void computeStaticPosition(const FloatingContext&, const Box&) const; > void computeFloatingPosition(const FloatingContext&, const Box&) const; > void computePositionToAvoidFloats(const FloatingContext&, const Box&) const; >- void computeVerticalPositionForFloatClear(const FloatingContext&, const Box&) const; > > void computeEstimatedVerticalPosition(const Box&) const; > void computeEstimatedVerticalPositionForAncestors(const Box&) const; > void computeEstimatedVerticalPositionForFormattingRoot(const Box&) const; >- void computeEstimatedVerticalPositionForFloatClear(const Box&) const; >+ void computeEstimatedVerticalPositionForFloatClear(const FloatingContext&, const Box&) const; > > InstrinsicWidthConstraints instrinsicWidthConstraints() const override; >- LayoutUnit adjustedVerticalPositionAfterMarginCollapsing(const Box&, const UsedVerticalMargin&) const; >+ LayoutUnit verticalPositionWithMargin(const Box&, const UsedVerticalMargin&) const; > > // This class implements positioning and sizing for boxes participating in a block formatting context. > class Geometry : public FormattingContext::Geometry { >diff --git a/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp b/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp >index d41405c34dc..02f7f00114a 100644 >--- a/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp >+++ b/Source/WebCore/layout/blockformatting/BlockMarginCollapse.cpp >@@ -582,11 +582,9 @@ EstimatedMarginBefore BlockFormattingContext::MarginCollapse::estimatedMarginBef > return { }; > > ASSERT(layoutBox.isBlockLevelBox()); >- // Can't estimate vertical margins for out of flow boxes (and we shouldn't need to do it for float boxes). >- ASSERT(layoutBox.isInFlow()); >+ // Don't estimate vertical margins for out of flow boxes. >+ ASSERT(layoutBox.isInFlow() || layoutBox.isFloatingPositioned()); > ASSERT(!layoutBox.replaced()); >- // Can't cross block formatting context boundary. >- ASSERT(!layoutBox.establishesBlockFormattingContext()); > > auto computedVerticalMargin = Geometry::computedVerticalMargin(layoutState, layoutBox); > auto nonCollapsedMargin = UsedVerticalMargin::NonCollapsedValues { computedVerticalMargin.before.valueOr(0), computedVerticalMargin.after.valueOr(0) }; >diff --git a/Source/WebCore/layout/displaytree/DisplayBox.h b/Source/WebCore/layout/displaytree/DisplayBox.h >index b7549f1fc40..fd31dfe7d0e 100644 >--- a/Source/WebCore/layout/displaytree/DisplayBox.h >+++ b/Source/WebCore/layout/displaytree/DisplayBox.h >@@ -242,7 +242,7 @@ private: > Layout::UsedHorizontalMargin m_horizontalMargin; > Layout::UsedVerticalMargin m_verticalMargin; > Layout::ComputedHorizontalMargin m_horizontalComputedMargin; >- bool m_hasClearance; >+ bool m_hasClearance { false }; > > Layout::Edges m_border; > Optional<Layout::Edges> m_padding; >diff --git a/Tools/ChangeLog b/Tools/ChangeLog >index 5282ee4b060..0a493f84fa5 100644 >--- a/Tools/ChangeLog >+++ b/Tools/ChangeLog >@@ -1,3 +1,12 @@ >+2019-01-25 Zalan Bujtas <zalan@apple.com> >+ >+ [LFC][BFC][MarginCollapsing] Add "clear" to static position computation. >+ https://bugs.webkit.org/show_bug.cgi?id=193824 >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ * LayoutReloaded/misc/LFC-passing-tests.txt: >+ > 2019-01-24 Fujii Hironori <Hironori.Fujii@sony.com> > > [Win][WebKitTestRunner] Implement EventSenderProxy >diff --git a/Tools/LayoutReloaded/misc/LFC-passing-tests.txt b/Tools/LayoutReloaded/misc/LFC-passing-tests.txt >index e8592bd0eb4..393c713e16e 100644 >--- a/Tools/LayoutReloaded/misc/LFC-passing-tests.txt >+++ b/Tools/LayoutReloaded/misc/LFC-passing-tests.txt >@@ -133,6 +133,7 @@ fast/block/margin-collapse/029.html > fast/block/margin-collapse/035.html > fast/block/margin-collapse/039.html > fast/block/margin-collapse/040.html >+fast/block/margin-collapse/044.html > fast/block/positioning/003.html > fast/block/positioning/004.html > fast/block/positioning/005.html
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Flags:
koivisto
:
review+
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 193824
: 360116