Simplify Writing-Mode Related Calculation of Margins

Simplify Writing-Mode Related Calculation of Margins

Remove the very complicated logic from computeLogicalHeight() and
computeLogicalWidth() whose sole purpose was to use the writing mode of
the child vs the containing block when appropriate. The complexity was
unnecessary and the same result can be achieved by ensuring we use the
child's writing mode when computing the size of the box and the parent's
writing mode when computing the margins for position (e.g. for margin
collapsing).

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176154

[leviw_travelin_and_unemployed, 2014-06-04 19:23:05]

LGTM!

[rhogan, 2014-06-08 06:27:41]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2014-06-08 06:28:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/robhogan@gmail.com/298563002/20001

[commit-bot: I haz the power, 2014-06-08 06:28:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-08 06:28:11]

Failed to apply patch for Source/core/rendering/RenderBox.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/RenderBox.cpp
  Hunk #1 succeeded at 2237 (offset 24 lines).
  Hunk #2 succeeded at 2406 (offset 24 lines).
  Hunk #3 succeeded at 2454 (offset 24 lines).
  Hunk #4 succeeded at 2484 (offset 24 lines).
  Hunk #5 succeeded at 2511 (offset 24 lines).
  Hunk #6 succeeded at 2571 (offset 24 lines).
  Hunk #7 FAILED at 2901.
  1 out of 7 hunks FAILED -- saving rejects to file
Source/core/rendering/RenderBox.cpp.rej

Patch:       Source/core/rendering/RenderBox.cpp
Index: Source/core/rendering/RenderBox.cpp
diff --git a/Source/core/rendering/RenderBox.cpp
b/Source/core/rendering/RenderBox.cpp
index
243bf9a00fb97c80ed3eb8336363dd33484edfeb..d1d6c4e3ed1179dd652681682e7b3569a915cf04
100644
--- a/Source/core/rendering/RenderBox.cpp
+++ b/Source/core/rendering/RenderBox.cpp
@@ -2213,15 +2213,8 @@ void
RenderBox::computeLogicalWidth(LogicalExtentComputedValues& computedValues)
     }
 
     // Margin calculations.
-    if (hasPerpendicularContainingBlock || isFloating() || isInline()) {
-        computedValues.m_margins.m_start =
minimumValueForLength(styleToUse->marginStart(), containerLogicalWidth);
-        computedValues.m_margins.m_end =
minimumValueForLength(styleToUse->marginEnd(), containerLogicalWidth);
-    } else {
-        bool hasInvertedDirection = cb->style()->isLeftToRightDirection() !=
style()->isLeftToRightDirection();
-        computeInlineDirectionMargins(cb, containerLogicalWidth,
computedValues.m_extent,
-            hasInvertedDirection ? computedValues.m_margins.m_end :
computedValues.m_margins.m_start,
-            hasInvertedDirection ? computedValues.m_margins.m_start :
computedValues.m_margins.m_end);
-    }
+    computeMarginsForDirection(InlineDirection, cb, containerLogicalWidth,
computedValues.m_extent, computedValues.m_margins.m_start,
+        computedValues.m_margins.m_end, style()->marginStart(),
style()->marginEnd());
 
     if (!hasPerpendicularContainingBlock && containerLogicalWidth &&
containerLogicalWidth != (computedValues.m_extent +
computedValues.m_margins.m_start + computedValues.m_margins.m_end)
         && !isFloating() && !isInline() &&
!cb->isFlexibleBoxIncludingDeprecated() && !cb->isRenderGrid()) {
@@ -2389,13 +2382,19 @@ bool RenderBox::autoWidthShouldFitContent() const
         || isHTMLTextAreaElement(*node()) || (isHTMLLegendElement(*node()) &&
!style()->hasOutOfFlowPosition()));
 }
 
-void RenderBox::computeInlineDirectionMargins(RenderBlock* containingBlock,
LayoutUnit containerWidth, LayoutUnit childWidth, LayoutUnit& marginStart,
LayoutUnit& marginEnd) const
+void RenderBox::computeMarginsForDirection(MarginDirection flowDirection, const
RenderBlock* containingBlock, LayoutUnit containerWidth, LayoutUnit childWidth,
LayoutUnit& marginStart, LayoutUnit& marginEnd, Length marginStartLength, Length
marginEndLength) const
 {
-    const RenderStyle* containingBlockStyle = containingBlock->style();
-    Length marginStartLength = style()->marginStartUsing(containingBlockStyle);
-    Length marginEndLength = style()->marginEndUsing(containingBlockStyle);
+    if (flowDirection == BlockDirection || isFloating() || isInline()) {
+        if (isTableCell() && flowDirection == BlockDirection) {
+            // FIXME: Not right if we allow cells to have different
directionality than the table. If we do allow this, though,
+            // we may just do it with an extra anonymous block inside the cell.
+            marginStart = 0;
+            marginEnd = 0;
+            return;
+        }
 
-    if (isFloating() || isInline()) {
+        // Margins are calculated with respect to the logical width of
+        // the containing block (8.3)
         // Inline blocks/tables and floats don't have their margins increased.
         marginStart = minimumValueForLength(marginStartLength, containerWidth);
         marginEnd = minimumValueForLength(marginEndLength, containerWidth);
@@ -2431,8 +2430,9 @@ void RenderBox::computeInlineDirectionMargins(RenderBlock*
containingBlock, Layo
 
     // CSS 2.1: "If both 'margin-left' and 'margin-right' are 'auto', their
used values are equal. This horizontally centers the element
     // with respect to the edges of the containing block."
+    const RenderStyle* containingBlockStyle = containingBlock->style();
     if ((marginStartLength.isAuto() && marginEndLength.isAuto() &&
marginBoxWidth < availableWidth)
-        || (!marginStartLength.isAuto() && !marginEndLength.isAuto() &&
containingBlock->style()->textAlign() == WEBKIT_CENTER)) {
+        || (!marginStartLength.isAuto() && !marginEndLength.isAuto() &&
containingBlockStyle->textAlign() == WEBKIT_CENTER)) {
         // Other browsers center the margin box for align=center elements so we
match them here.
         LayoutUnit centeredMarginBoxStart = max<LayoutUnit>(0, (availableWidth
- childWidth - marginStartWidth - marginEndWidth) / 2);
         marginStart = centeredMarginBoxStart + marginStartWidth;
@@ -2460,32 +2460,6 @@ void
RenderBox::computeInlineDirectionMargins(RenderBlock* containingBlock, Layo
     marginEnd = marginEndWidth;
 }
 
-static bool shouldFlipBeforeAfterMargins(const RenderStyle*
containingBlockStyle, const RenderStyle* childStyle)
-{
-    ASSERT(containingBlockStyle->isHorizontalWritingMode() !=
childStyle->isHorizontalWritingMode());
-    WritingMode childWritingMode = childStyle->writingMode();
-    bool shouldFlip = false;
-    switch (containingBlockStyle->writingMode()) {
-    case TopToBottomWritingMode:
-        shouldFlip = (childWritingMode == RightToLeftWritingMode);
-        break;
-    case BottomToTopWritingMode:
-        shouldFlip = (childWritingMode == RightToLeftWritingMode);
-        break;
-    case RightToLeftWritingMode:
-        shouldFlip = (childWritingMode == BottomToTopWritingMode);
-        break;
-    case LeftToRightWritingMode:
-        shouldFlip = (childWritingMode == BottomToTopWritingMode);
-        break;
-    }
-
-    if (!containingBlockStyle->isLeftToRightDirection())
-        shouldFlip = !shouldFlip;
-
-    return shouldFlip;
-}
-
 void RenderBox::updateLogicalHeight()
 {
     m_intrinsicContentLogicalHeight = contentLogicalHeight();
@@ -2513,23 +2487,20 @@ void RenderBox::computeLogicalHeight(LayoutUnit
logicalHeight, LayoutUnit logica
         computePositionedLogicalHeight(computedValues);
     else {
         RenderBlock* cb = containingBlock();
-        bool hasPerpendicularContainingBlock = cb->isHorizontalWritingMode() !=
isHorizontalWritingMode();
 
-        if (!hasPerpendicularContainingBlock) {
-            bool shouldFlipBeforeAfter = cb->style()->writingMode() !=
style()->writingMode();
-            computeBlockDirectionMargins(cb,
-                shouldFlipBeforeAfter ? computedValues.m_margins.m_after :
computedValues.m_margins.m_before,
-                shouldFlipBeforeAfter ? computedValues.m_margins.m_before :
computedValues.m_margins.m_after);
-        }
+        // If we are perpendicular to our containing block then we need to
resolve our block-start and block-end margins so that if they
+        // are 'auto' we are centred or aligned within the inline flow
containing block: this is done by computing the margins as though they are
inline.
+        // Note that as this is the 'sizing phase' we are using our own writing
mode rather than the containing block's. We use the containing block's
+        // writing mode when figuring out the block-direction margins for
positioning in |computeAndSetBlockDirectionMargins| (i.e. margin collapsing
etc.).
+        // See
http://www.w3.org/TR/2014/CR-css-writing-modes-3-20140320/#orthogonal-flows
+        MarginDirection flowDirection = isHorizontalWritingMode() !=
cb->isHorizontalWritingMode() ? InlineDirection : BlockDirection;
 
         // For tables, calculate margins only.
         if (isTable()) {
-            if (hasPerpendicularContainingBlock) {
-                bool shouldFlipBeforeAfter =
shouldFlipBeforeAfterMargins(cb->style(), style());
-                computeInlineDirectionMargins(cb,
containingBlockLogicalWidthForContent(), computedValues.m_extent,
-                    shouldFlipBeforeAfter ? computedValues.m_margins.m_after :
computedValues.m_margins.m_before,
-                    shouldFlipBeforeAfter ? computedValues.m_margins.m_before :
computedValues.m_margins.m_after);
-            }
+            // FIXME: RenderTable::layout() calls updateLogicalHeight() when an
empty table has no height yet, so auto margins can come out wrong here when
+            // we are perpendicular to our containing block.
+            computeMarginsForDirection(flowDirection, cb,
containingBlockLogicalWidthForContent(), computedValues.m_extent,
computedValues.m_margins.m_before,
+                computedValues.m_margins.m_after, style()->marginBefore(),
style()->marginAfter());
             return;
         }
 
@@ -2576,13 +2547,8 @@ void RenderBox::computeLogicalHeight(LayoutUnit
logicalHeight, LayoutUnit logica
         }
 
         computedValues.m_extent = heightResult;
-
-        if (hasPerpendicularContainingBlock) {
-            bool shouldFlipBeforeAfter =
shouldFlipBeforeAfterMargins(cb->style(), style());
-            computeInlineDirectionMargins(cb,
containingBlockLogicalWidthForContent(), heightResult,
-                shouldFlipBeforeAfter ? computedValues.m_margins.m_after :
computedValues.m_margins.m_before,
-                shouldFlipBeforeAfter ? computedValues.m_margins.m_before :
computedValues.m_margins.m_after);
-        }
+        computeMarginsForDirection(flowDirection, cb,
containingBlockLogicalWidthForContent(), computedValues.m_extent,
computedValues.m_margins.m_before,
+            computedValues.m_margins.m_after, style()->marginBefore(),
style()->marginAfter());
     }
 
     // WinIE quirk: The <html> block always fills the entire canvas in quirks …
(message too large)

[rhogan, 2014-06-14 09:14:48]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2014-06-14 09:15:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/robhogan@gmail.com/298563002/40001

[commit-bot: I haz the power, 2014-06-14 10:42:16]

Change committed as 176154