[CSS Grid Layout] Fix positioned grid children position and size

Source/core/rendering/RenderBox.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2005 Allan Sandfeld Jensen (kde@carewolf.com)
  *           (C) 2005, 2006 Samuel Weinig (sam.weinig@gmail.com)
  * Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2013 Adobe Systems Incorporated. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 2651 matching lines @@
 
     // Use viewport as container for top-level fixed-position elements.
     if (style()->position() == FixedPosition && containingBlock->isRenderView()) {
         const RenderView* view = toRenderView(containingBlock);
         if (FrameView* frameView = view->frameView()) {
             LayoutRect viewportRect = frameView->viewportConstrainedVisibleContentRect();
             return containingBlock->isHorizontalWritingMode() ? viewportRect.width() : viewportRect.height();
         }
     }
 
+    if (hasOverrideContainingBlockLogicalWidth())
+        return overrideContainingBlockContentLogicalWidth();
+
     if (containingBlock->isBox())
         return toRenderBox(containingBlock)->clientLogicalWidth();
 
     ASSERT(containingBlock->isRenderInline() && containingBlock->isRelPositioned());
 
     const RenderInline* flow = toRenderInline(containingBlock);
     InlineFlowBox* first = flow->firstLineBox();
     InlineFlowBox* last = flow->lastLineBox();
 
     // If the containing block is empty, return a width of 0.
@@ skipping 20 matching lines @@
 
     // Use viewport as container for top-level fixed-position elements.
     if (style()->position() == FixedPosition && containingBlock->isRenderView()) {
         const RenderView* view = toRenderView(containingBlock);
         if (FrameView* frameView = view->frameView()) {
             LayoutRect viewportRect = frameView->viewportConstrainedVisibleContentRect();
             return containingBlock->isHorizontalWritingMode() ? viewportRect.height() : viewportRect.width();
         }
     }
 
+    if (hasOverrideContainingBlockLogicalHeight())
+        return overrideContainingBlockContentLogicalHeight();
+
     if (containingBlock->isBox()) {
         const RenderBlock* cb = containingBlock->isRenderBlock() ?
             toRenderBlock(containingBlock) : containingBlock->containingBlock();
         return cb->clientLogicalHeight();
     }
 
     ASSERT(containingBlock->isRenderInline() && containingBlock->isRelPositioned());
 
     const RenderInline* flow = toRenderInline(containingBlock);
     InlineFlowBox* first = flow->firstLineBox();
     InlineFlowBox* last = flow->lastLineBox();
 
     // If the containing block is empty, return a height of 0.
     if (!first || !last)
         return 0;
 
     LayoutUnit heightResult;
     LayoutRect boundingBox = flow->linesBoundingBox();
     if (containingBlock->isHorizontalWritingMode())
         heightResult = boundingBox.height();
     else
         heightResult = boundingBox.width();
     heightResult -= (containingBlock->borderBefore() + containingBlock->borderAfter());
     return heightResult;
 }
 
 static void computeInlineStaticDistance(Length& logicalLeft, Length& logicalRight, const RenderBox* child, const RenderBoxModelObject* containerBlock, LayoutUnit containerLogicalWidth)
 {
-    if (!logicalLeft.isAuto() || !logicalRight.isAuto())
+    if ((!logicalLeft.isAuto() || !logicalRight.isAuto()) && !child->parent()->isRenderGrid())
         return;
 
     // FIXME: The static distance computation has not been patched for mixed writing modes yet.
     if (child->parent()->style()->direction() == LTR) {
         LayoutUnit staticPosition = child->layer()->staticInlinePosition() - containerBlock->borderLogicalLeft();
         for (RenderObject* curr = child->parent(); curr && curr != containerBlock; curr = curr->container()) {
             if (curr->isBox()) {
                 staticPosition += toRenderBox(curr)->logicalLeft();
                 if (toRenderBox(curr)->isRelPositioned())
                     staticPosition += toRenderBox(curr)->relativePositionOffset().width();
             } else if (curr->isInline()) {
                 if (curr->isRelPositioned()) {
                     if (!curr->style()->logicalLeft().isAuto())
                         staticPosition += curr->style()->logicalLeft().value();
                     else
                         staticPosition -= curr->style()->logicalRight().value();
                 }
             }
         }
+        if (!logicalLeft.isAuto())
+            staticPosition += valueForLength(logicalLeft, containerLogicalWidth);
+        else if (!logicalRight.isAuto())
+            staticPosition -= valueForLength(logicalRight, containerLogicalWidth);

[Julien - ping for review, 2014/10/20 19:34:43]

It's unclear to me why this doesn't impact regular blocks as we should change
their offset too. I also couldn't see any justification as to why it's good.

Also there is probably a case to be made to split between grid and other blocks
for readability and avoid introducing unintended bugs for the other classes
relying on RenderBlock. If we could do it without extra code duplication, /me
would be more happy!

[Manuel Rego, 2014/10/23 12:48:38]

Basically it doesn't affect regular blocks, because of the early return
at the beginning of the method.
If any of these conditions are true, we're sure that our parent is a grid.
Anyway, I agree with you that it'd be simpler to understand if
we split it, so I've done it in the new version of the patch.

         logicalLeft.setValue(Fixed, staticPosition);
     } else {
         RenderBox* enclosingBox = child->parent()->enclosingBox();
         LayoutUnit staticPosition = child->layer()->staticInlinePosition() + containerLogicalWidth + containerBlock->borderLogicalLeft();
         for (RenderObject* curr = child->parent(); curr; curr = curr->container()) {
             if (curr->isBox()) {
                 if (curr != containerBlock) {
                     staticPosition -= toRenderBox(curr)->logicalLeft();
                     if (toRenderBox(curr)->isRelPositioned())
                         staticPosition -= toRenderBox(curr)->relativePositionOffset().width();
                 }
                 if (curr == enclosingBox)
                     staticPosition -= enclosingBox->logicalWidth();
             } else if (curr->isInline()) {
                 if (curr->isRelPositioned()) {
                     if (!curr->style()->logicalLeft().isAuto())
                         staticPosition -= curr->style()->logicalLeft().value();

[Julien - ping for review, 2014/10/20 19:34:43]

O_o

That looks like a bug to me. It will happily do the following conversion: 100%
-> 100px

[Manuel Rego, 2014/10/22 14:06:20]

Though this is unrelated to my patch, I've been checking it and you're right,
it's doing the wrong conversion as you explained.

I've just reported the bug
http://code.google.com/p/chromium/issues/detail?id=425991

                     else
                         staticPosition += curr->style()->logicalRight().value();
                 }
             }
             if (curr == containerBlock)
                 break;
         }
+        if (!logicalRight.isAuto())
+            staticPosition += valueForLength(logicalRight, containerLogicalWidth);
+        else if (!logicalLeft.isAuto())
+            staticPosition -= valueForLength(logicalLeft, containerLogicalWidth);

[Manuel Rego, 2014/10/23 12:48:38]

Like in the previous method, I've done something similar here too.

         logicalRight.setValue(Fixed, staticPosition);
     }
 }
 
 void RenderBox::computePositionedLogicalWidth(LogicalExtentComputedValues& computedValues) const
 {
     if (isReplaced()) {
         computePositionedLogicalWidthReplaced(computedValues);
         return;
     }
@@ skipping 302 matching lines @@
     }
 
     if (containerBlock->isBox() && toRenderBox(containerBlock)->scrollsOverflowY() && containerBlock->style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft()) {
         logicalLeftValue = logicalLeftValue + toRenderBox(containerBlock)->verticalScrollbarWidth();
     }
 
     computedValues.m_position = logicalLeftValue + marginLogicalLeftValue;
     computeLogicalLeftPositionedOffset(computedValues.m_position, this, computedValues.m_extent, containerBlock, containerLogicalWidth);
 }
 
-static void computeBlockStaticDistance(Length& logicalTop, Length& logicalBottom, const RenderBox* child, const RenderBoxModelObject* containerBlock)
+static void computeBlockStaticDistance(Length& logicalTop, Length& logicalBottom, const RenderBox* child, const RenderBoxModelObject* containerBlock, LayoutUnit containerLogicalHeight)
 {
-    if (!logicalTop.isAuto() || !logicalBottom.isAuto())
+    if ((!logicalTop.isAuto() || !logicalBottom.isAuto()) && !child->parent()->isRenderGrid())
         return;
 
     // FIXME: The static distance computation has not been patched for mixed writing modes.
     LayoutUnit staticLogicalTop = child->layer()->staticBlockPosition() - containerBlock->borderBefore();
     for (RenderObject* curr = child->parent(); curr && curr != containerBlock; curr = curr->container()) {
         if (curr->isBox() && !curr->isTableRow())
             staticLogicalTop += toRenderBox(curr)->logicalTop();
     }
+    if (!logicalTop.isAuto())
+        staticLogicalTop += valueForLength(logicalTop, containerLogicalHeight);
+    else if (!logicalBottom.isAuto())
+        staticLogicalTop -= valueForLength(logicalBottom, containerLogicalHeight);
     logicalTop.setValue(Fixed, staticLogicalTop);
 }
 
 void RenderBox::computePositionedLogicalHeight(LogicalExtentComputedValues& computedValues) const
 {
     if (isReplaced()) {
         computePositionedLogicalHeightReplaced(computedValues);
         return;
     }
 
@@ skipping 28 matching lines @@
      * But rather than actually calculating the dimensions of that hypothetical
      * box, user agents are free to make a guess at its probable position.
      *
      * For the purposes of calculating the static position, the containing block
      * of fixed positioned elements is the initial containing block instead of
      * the viewport.
     \*---------------------------------------------------------------------------*/
 
     // see FIXME 1
     // Calculate the static distance if needed.
-    computeBlockStaticDistance(logicalTopLength, logicalBottomLength, this, containerBlock);
+    computeBlockStaticDistance(logicalTopLength, logicalBottomLength, this, containerBlock, containerLogicalHeight);
 
     // Calculate constraint equation values for 'height' case.
     LayoutUnit logicalHeight = computedValues.m_extent;
     computePositionedLogicalHeightUsing(styleToUse->logicalHeight(), containerBlock, containerLogicalHeight, bordersPlusPadding, logicalHeight,
                                         logicalTopLength, logicalBottomLength, marginBefore, marginAfter,
                                         computedValues);
 
     // Avoid doing any work in the common case (where the values of min-height and max-height are their defaults).
     // see FIXME 2
 
@@ skipping 385 matching lines @@
     // are dealt with in computeReplacedHeight().  This means that the steps to produce
     // correct max/min in the non-replaced version, are not necessary.
     computedValues.m_extent = computeReplacedLogicalHeight() + borderAndPaddingLogicalHeight();
     const LayoutUnit availableSpace = containerLogicalHeight - computedValues.m_extent;
 
     /*-----------------------------------------------------------------------*\
      * 2. If both 'top' and 'bottom' have the value 'auto', replace 'top'
      *    with the element's static position.
     \*-----------------------------------------------------------------------*/
     // see FIXME 1
-    computeBlockStaticDistance(logicalTop, logicalBottom, this, containerBlock);
+    computeBlockStaticDistance(logicalTop, logicalBottom, this, containerBlock, containerLogicalHeight);
 
     /*-----------------------------------------------------------------------*\
      * 3. If 'bottom' is 'auto', replace any 'auto' on 'margin-top' or
      *    'margin-bottom' with '0'.
     \*-----------------------------------------------------------------------*/
     // FIXME: The spec. says that this step should only be taken when bottom is
     // auto, but if only top is auto, this makes step 4 impossible.
     if (logicalTop.isAuto() || logicalBottom.isAuto()) {
         if (marginBefore.isAuto())
             marginBefore.setValue(Fixed, 0);
@@ skipping 927 matching lines @@
     ASSERT(style()->hasBackground() || style()->hasBoxDecorations());
 
     if (m_rareData && m_rareData->m_previousBorderBoxSize.width() != -1)
         return m_rareData->m_previousBorderBoxSize;
 
     // We didn't save the old border box size because it was the same as the size of oldBounds.
     return previousBoundsSize;
 }
 
 } // namespace blink