[css-grid] Fix placement for unknown named grid lines

third_party/WebKit/Source/core/style/GridResolvedPosition.cpp

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/style/GridResolvedPosition.h"
 
 #include "core/layout/LayoutBox.h"
 #include "core/style/GridCoordinate.h"
 #include <algorithm>
 
@@ skipping 29 matching lines @@
 static void initialAndFinalPositionsFromStyle(const ComputedStyle& gridContainerStyle, const LayoutBox& gridItem, GridTrackSizingDirection direction, GridPosition& initialPosition, GridPosition& finalPosition)
 {
     initialPosition = (direction == ForColumns) ? gridItem.style()->gridColumnStart() : gridItem.style()->gridRowStart();
     finalPosition = (direction == ForColumns) ? gridItem.style()->gridColumnEnd() : gridItem.style()->gridRowEnd();
 
     // We must handle the placement error handling code here instead of in the StyleAdjuster because we don't want to
     // overwrite the specified values.
     if (initialPosition.isSpan() && finalPosition.isSpan())
         finalPosition.setAutoPosition();
 
-    // Try to early detect the case of non existing named grid lines. This way we could assume later that
-    // GridResolvedPosition::resolveGrisPositionFromStyle() always return a valid resolved position.
-    if (initialPosition.isNamedGridArea() && !GridResolvedPosition::isValidNamedLineOrArea(initialPosition.namedGridLine(), gridContainerStyle, GridResolvedPosition::initialPositionSide(direction)))
-        initialPosition.setAutoPosition();
+    if (gridItem.isOutOfFlowPositioned()) {
+        // Early detect the case of non existing named grid lines for positioned items.
+        if (initialPosition.isNamedGridArea() && !GridResolvedPosition::isValidNamedLineOrArea(initialPosition.namedGridLine(), gridContainerStyle, GridResolvedPosition::initialPositionSide(direction)))
+            initialPosition.setAutoPosition();
 
-    if (finalPosition.isNamedGridArea() && !GridResolvedPosition::isValidNamedLineOrArea(finalPosition.namedGridLine(), gridContainerStyle, GridResolvedPosition::finalPositionSide(direction)))
-        finalPosition.setAutoPosition();
+        if (finalPosition.isNamedGridArea() && !GridResolvedPosition::isValidNamedLineOrArea(finalPosition.namedGridLine(), gridContainerStyle, GridResolvedPosition::finalPositionSide(direction)))
+            finalPosition.setAutoPosition();
+    }
 
     // If the grid item has an automatic position and a grid span for a named line in a given dimension, instead treat the grid span as one.
     if (initialPosition.isAuto() && finalPosition.isSpan() && !finalPosition.namedGridLine().isNull())
         finalPosition.setSpanPosition(1, String());
     if (finalPosition.isAuto() && initialPosition.isSpan() && !initialPosition.namedGridLine().isNull())
         initialPosition.setSpanPosition(1, String());
 }
 
-static GridSpan definiteGridSpanWithInitialNamedSpanAgainstOpposite(size_t resolvedOppositePosition, const GridPosition& position, const Vector<size_t>& gridLines)
+static int lookAheadForNamedGridLine(int start, int numberOfLines, const Vector<size_t>& namedGridLinesIndexes, int gridLastLine)
 {
-    if (resolvedOppositePosition == 0)
-        return GridSpan::untranslatedDefiniteGridSpan(resolvedOppositePosition, resolvedOppositePosition + 1);
+    ASSERT(numberOfLines > 0);
+    if (start > gridLastLine)
+        return start + (numberOfLines - 1);
+    if (start < 0)
+        start = 0;

[svillar, 2016/01/26 09:29:37]

Not sure I understand this condition.

[Manuel Rego, 2016/01/26 11:07:35]

This is needed because of the last change on the spec.

See this example:
https://drafts.csswg.org/css-grid/#example-64f80282

I've added a comment to clarify it.

 
-    size_t firstLineBeforeOppositePositionIndex = 0;
-    const size_t* firstLineBeforeOppositePosition = std::lower_bound(gridLines.begin(), gridLines.end(), resolvedOppositePosition);
-    if (firstLineBeforeOppositePosition != gridLines.end())
-        firstLineBeforeOppositePositionIndex = firstLineBeforeOppositePosition - gridLines.begin();
-    size_t gridLineIndex = std::max<int>(0, firstLineBeforeOppositePositionIndex - position.spanPosition());
-    size_t resolvedGridLinePosition = gridLines[gridLineIndex];
-    if (resolvedGridLinePosition >= resolvedOppositePosition)
-        resolvedGridLinePosition = resolvedOppositePosition - 1;
-    return GridSpan::untranslatedDefiniteGridSpan(resolvedGridLinePosition, resolvedOppositePosition);
+    for (int end = start; numberOfLines > 0; end++) {
+        if (end > gridLastLine || end < 0 || namedGridLinesIndexes.contains(static_cast<size_t>(end))) {
+            numberOfLines--;
+            if (!numberOfLines)
+                return end;
+        }
+    }

[svillar, 2016/01/26 09:29:37]

This loop looks terribly confusing, we're using a variable to iterate and then
some other in the exit condition. What's more, the very same condition to stop
de loop is used to early return.

[Manuel Rego, 2016/01/26 11:07:35]

I've tried a different approach, I think it's a bit better.

+    ASSERT_NOT_REACHED();
+    return 0;
 }
 
-static GridSpan definiteGridSpanWithFinalNamedSpanAgainstOpposite(size_t resolvedOppositePosition, const GridPosition& position, const Vector<size_t>& gridLines)
+static int lookBackForNamedGridLine(int end, int numberOfLines, const Vector<size_t>& namedGridLinesIndexes, int gridLastLine)
 {
-    ASSERT(gridLines.size());
-    size_t firstLineAfterOppositePositionIndex = gridLines.size() - 1;
-    const size_t* firstLineAfterOppositePosition = std::upper_bound(gridLines.begin(), gridLines.end(), resolvedOppositePosition);
-    if (firstLineAfterOppositePosition != gridLines.end())
-        firstLineAfterOppositePositionIndex = firstLineAfterOppositePosition - gridLines.begin();
-    size_t gridLineIndex = std::min(gridLines.size() - 1, firstLineAfterOppositePositionIndex + position.spanPosition() - 1);
-    size_t resolvedGridLinePosition = gridLines[gridLineIndex];
-    if (resolvedGridLinePosition <= resolvedOppositePosition)
-        resolvedGridLinePosition = resolvedOppositePosition + 1;
+    ASSERT(numberOfLines > 0);
+    if (end < 0)
+        return end - (numberOfLines - 1);
+    if (end > gridLastLine)
+        end = gridLastLine;
 
-    return GridSpan::untranslatedDefiniteGridSpan(resolvedOppositePosition, resolvedGridLinePosition);
+    for (int start = end; numberOfLines > 0; start--) {
+        if (start > gridLastLine || start < 0 || namedGridLinesIndexes.contains(static_cast<size_t>(start))) {
+            numberOfLines--;
+            if (!numberOfLines)
+                return start;
+        }
+    }

[svillar, 2016/01/26 09:29:37]

Same comments than in lookAhead

[Manuel Rego, 2016/01/26 11:07:35]

Acknowledged.

+    ASSERT_NOT_REACHED();
+    return 0;
 }
 
-static GridSpan definiteGridSpanWithNamedSpanAgainstOpposite(size_t resolvedOppositePosition, const GridPosition& position, GridPositionSide side, const Vector<size_t>& gridLines)
+static GridSpan definiteGridSpanWithNamedSpanAgainstOpposite(int resolvedOppositePosition, const GridPosition& position, GridPositionSide side, const Vector<size_t>& gridLines, int lastLine)
 {
-    if (side == RowStartSide || side == ColumnStartSide)
-        return definiteGridSpanWithInitialNamedSpanAgainstOpposite(resolvedOppositePosition, position, gridLines);
+    int start, end;
 
-    return definiteGridSpanWithFinalNamedSpanAgainstOpposite(resolvedOppositePosition, position, gridLines);
+    if (side == RowStartSide || side == ColumnStartSide) {

[svillar, 2016/01/26 09:29:37]

I think we had a isStartSide() or something like that doing exactly what this
condition does.

[Manuel Rego, 2016/01/26 11:07:35]

Mmmm, I cannot find it.

We can add it if needed, but there're more parts of the code
with a condition like this one. We should change all of them.

+        start = lookBackForNamedGridLine(resolvedOppositePosition - 1, position.spanPosition(), gridLines, lastLine);
+        end = resolvedOppositePosition;
+    } else {
+        start = resolvedOppositePosition;
+        end = lookAheadForNamedGridLine(resolvedOppositePosition + 1, position.spanPosition(), gridLines, lastLine);
+    }
+
+    return GridSpan::untranslatedDefiniteGridSpan(start, end);
 }
 
-static GridSpan resolveNamedGridLinePositionAgainstOppositePosition(const ComputedStyle& gridContainerStyle, size_t resolvedOppositePosition, const GridPosition& position, GridPositionSide side)
+size_t GridResolvedPosition::explicitGridColumnCount(const ComputedStyle& gridContainerStyle)
+{
+    return std::min<size_t>(gridContainerStyle.gridTemplateColumns().size(), kGridMaxTracks);
+}
+
+size_t GridResolvedPosition::explicitGridRowCount(const ComputedStyle& gridContainerStyle)
+{
+    return std::min<size_t>(gridContainerStyle.gridTemplateRows().size(), kGridMaxTracks);
+}
+
+static size_t explicitGridSizeForSide(const ComputedStyle& gridContainerStyle, GridPositionSide side)
+{
+    return (side == ColumnStartSide || side == ColumnEndSide) ? GridResolvedPosition::explicitGridColumnCount(gridContainerStyle) : GridResolvedPosition::explicitGridRowCount(gridContainerStyle);
+}
+
+static GridSpan resolveNamedGridLinePositionAgainstOppositePosition(const ComputedStyle& gridContainerStyle, int resolvedOppositePosition, const GridPosition& position, GridPositionSide side)
 {
     ASSERT(position.isSpan());
     ASSERT(!position.namedGridLine().isNull());
     // Negative positions are not allowed per the specification and should have been handled during parsing.
     ASSERT(position.spanPosition() > 0);
 
     const NamedGridLinesMap& gridLinesNames = gridLinesForSide(gridContainerStyle, side);
     NamedGridLinesMap::const_iterator it = gridLinesNames.find(position.namedGridLine());
-
-    // If there is no named grid line of that name, we resolve the position to 'auto' (which is equivalent to 'span 1' in this case).
-    // See http://lists.w3.org/Archives/Public/www-style/2013Jun/0394.html.
-    if (it == gridLinesNames.end()) {
-        if ((side == ColumnStartSide || side == RowStartSide) && resolvedOppositePosition)
-            return GridSpan::untranslatedDefiniteGridSpan(resolvedOppositePosition - 1, resolvedOppositePosition);
-        return GridSpan::untranslatedDefiniteGridSpan(resolvedOppositePosition, resolvedOppositePosition + 1);
-    }
-
-    return definiteGridSpanWithNamedSpanAgainstOpposite(resolvedOppositePosition, position, side, it->value);
+    size_t lastLine = explicitGridSizeForSide(gridContainerStyle, side);
+    return definiteGridSpanWithNamedSpanAgainstOpposite(resolvedOppositePosition, position, side, it == gridLinesNames.end() ? Vector<size_t>() : it->value, lastLine);

[svillar, 2016/01/26 09:29:37]

Uhu, so each time it == gridLinesNames.end() then we create an empty vector. We
cannot do that. You should modify definiteGridSpanWithNamedSpanAgainstOpposite()
to accept a pointer to a vector instead of a reference and deal with that there
and inside the functions it calls.

[Manuel Rego, 2016/01/26 11:07:35]

True, changed to use pointers.

 }
 
 static GridSpan definiteGridSpanWithSpanAgainstOpposite(size_t resolvedOppositePosition, const GridPosition& position, GridPositionSide side)
 {
     size_t positionOffset = position.spanPosition();
     if (side == ColumnStartSide || side == RowStartSide)
         return GridSpan::untranslatedDefiniteGridSpan(resolvedOppositePosition - positionOffset, resolvedOppositePosition);
 
     return GridSpan::untranslatedDefiniteGridSpan(resolvedOppositePosition, resolvedOppositePosition + positionOffset);
 }
 
-static GridSpan resolveGridPositionAgainstOppositePosition(const ComputedStyle& gridContainerStyle, size_t resolvedOppositePosition, const GridPosition& position, GridPositionSide side)
+static GridSpan resolveGridPositionAgainstOppositePosition(const ComputedStyle& gridContainerStyle, int resolvedOppositePosition, const GridPosition& position, GridPositionSide side)
 {
     if (position.isAuto()) {
         if (side == ColumnStartSide || side == RowStartSide)
             return GridSpan::untranslatedDefiniteGridSpan(resolvedOppositePosition - 1, resolvedOppositePosition);
         return GridSpan::untranslatedDefiniteGridSpan(resolvedOppositePosition, resolvedOppositePosition + 1);
     }
 
     ASSERT(position.isSpan());
     ASSERT(position.spanPosition() > 0);
 
@@ skipping 15 matching lines @@
 
     if (initialPosition.isAuto() && finalPosition.isAuto())
         return 1;
 
     GridPosition position = initialPosition.isSpan() ? initialPosition : finalPosition;
     ASSERT(position.isSpan());
     ASSERT(position.spanPosition());
     return position.spanPosition();
 }
 
-size_t GridResolvedPosition::explicitGridColumnCount(const ComputedStyle& gridContainerStyle)
-{
-    return std::min<size_t>(gridContainerStyle.gridTemplateColumns().size(), kGridMaxTracks);
-}
-
-size_t GridResolvedPosition::explicitGridRowCount(const ComputedStyle& gridContainerStyle)
-{
-    return std::min<size_t>(gridContainerStyle.gridTemplateRows().size(), kGridMaxTracks);
-}
-
-static size_t explicitGridSizeForSide(const ComputedStyle& gridContainerStyle, GridPositionSide side)
-{
-    return (side == ColumnStartSide || side == ColumnEndSide) ? GridResolvedPosition::explicitGridColumnCount(gridContainerStyle) : GridResolvedPosition::explicitGridRowCount(gridContainerStyle);
-}
-
-static size_t resolveNamedGridLinePositionFromStyle(const ComputedStyle& gridContainerStyle, const GridPosition& position, GridPositionSide side)
+static int resolveNamedGridLinePositionFromStyle(const ComputedStyle& gridContainerStyle, const GridPosition& position, GridPositionSide side)
 {
     ASSERT(!position.namedGridLine().isNull());
 
     const NamedGridLinesMap& gridLinesNames = gridLinesForSide(gridContainerStyle, side);
     NamedGridLinesMap::const_iterator it = gridLinesNames.find(position.namedGridLine());
-    if (it == gridLinesNames.end()) {
-        if (position.isPositive())
-            return 0;
-        size_t lastLine = explicitGridSizeForSide(gridContainerStyle, side);
-        return lastLine;
-    }
-
-    size_t namedGridLineIndex;
+    const Vector<size_t>& gridLines = it == gridLinesNames.end() ? Vector<size_t>() : it->value;

[svillar, 2016/01/26 09:29:37]

Same thing about creating Vectors.

[Manuel Rego, 2016/01/26 11:07:35]

Done.

+    size_t lastLine = explicitGridSizeForSide(gridContainerStyle, side);
     if (position.isPositive())
-        namedGridLineIndex = std::min<size_t>(position.integerPosition(), it->value.size()) - 1;
+        return lookAheadForNamedGridLine(0, abs(position.integerPosition()), gridLines, lastLine);
     else
-        namedGridLineIndex = std::max<int>(it->value.size() - abs(position.integerPosition()), 0);
-    return it->value[namedGridLineIndex];
+        return lookBackForNamedGridLine(lastLine, abs(position.integerPosition()), gridLines, lastLine);
 }
 
 static int resolveGridPositionFromStyle(const ComputedStyle& gridContainerStyle, const GridPosition& position, GridPositionSide side)
 {
     switch (position.type()) {
     case ExplicitPosition: {
         ASSERT(position.integerPosition());
 
         if (!position.namedGridLine().isNull())
             return resolveNamedGridLinePositionFromStyle(gridContainerStyle, position, side);
 
         // Handle <integer> explicit position.
         if (position.isPositive())
             return position.integerPosition() - 1;
 
         size_t resolvedPosition = abs(position.integerPosition()) - 1;
         size_t endOfTrack = explicitGridSizeForSide(gridContainerStyle, side);
 
         return endOfTrack - resolvedPosition;
     }
     case NamedGridAreaPosition:
     {
         // First attempt to match the grid area's edge to a named grid area: if there is a named line with the name
         // ''<custom-ident>-start (for grid-*-start) / <custom-ident>-end'' (for grid-*-end), contributes the first such
         // line to the grid item's placement.
         String namedGridLine = position.namedGridLine();
-        ASSERT(GridResolvedPosition::isValidNamedLineOrArea(namedGridLine, gridContainerStyle, side));
+        ASSERT(!position.namedGridLine().isNull());
 
         const NamedGridLinesMap& gridLineNames = gridLinesForSide(gridContainerStyle, side);
         NamedGridLinesMap::const_iterator implicitLineIter = gridLineNames.find(implicitNamedGridLineForSide(namedGridLine, side));
         if (implicitLineIter != gridLineNames.end())
             return implicitLineIter->value[0];
 
         // Otherwise, if there is a named line with the specified name, contributes the first such line to the grid
         // item's placement.
         NamedGridLinesMap::const_iterator explicitLineIter = gridLineNames.find(namedGridLine);
         if (explicitLineIter != gridLineNames.end())
             return explicitLineIter->value[0];
 
-        // If none of the above works specs mandate us to treat it as auto BUT we should have detected it before calling
-        // this function in GridResolvedPosition::resolveGridPositionsFromStyle(). We should be also covered by the
-        // ASSERT at the beginning of this block.
-        ASSERT_NOT_REACHED();
-        return 0;
+        ASSERT(!GridResolvedPosition::isValidNamedLineOrArea(namedGridLine, gridContainerStyle, side));
+        // If none of the above works specs mandate to assume that all the lines in the implicit grid have this name.
+        size_t lastLine = explicitGridSizeForSide(gridContainerStyle, side);
+        return lastLine + 1;
     }
     case AutoPosition:
     case SpanPosition:
         // 'auto' and span depend on the opposite position for resolution (e.g. grid-row: auto / 1 or grid-column: span 3 / "myHeader").
         ASSERT_NOT_REACHED();
         return 0;
     }
     ASSERT_NOT_REACHED();
     return 0;
 }
@@ skipping 28 matching lines @@
 
     if (resolvedFinalPosition < resolvedInitialPosition)
         std::swap(resolvedFinalPosition, resolvedInitialPosition);
     else if (resolvedFinalPosition == resolvedInitialPosition)
         resolvedFinalPosition = resolvedInitialPosition + 1;
 
     return GridSpan::untranslatedDefiniteGridSpan(resolvedInitialPosition, resolvedFinalPosition);
 }
 
 } // namespace blink