[css-grid] Fix height computation for items with intrinsic aspect ratios

third_party/WebKit/Source/core/layout/LayoutGrid.cpp

 /*
  * Copyright (C) 2011 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 667 matching lines @@
     }
 
     return GridTrackSize(minTrackBreadth, maxTrackBreadth);
 }
 
 LayoutUnit LayoutGrid::logicalHeightForChild(LayoutBox& child, Vector<GridTrack>& columnTracks)
 {
     SubtreeLayoutScope layoutScope(child);
     LayoutUnit oldOverrideContainingBlockContentLogicalWidth = child.hasOverrideContainingBlockLogicalWidth() ? child.overrideContainingBlockContentLogicalWidth() : LayoutUnit();
     LayoutUnit overrideContainingBlockContentLogicalWidth = gridAreaBreadthForChild(child, ForColumns, columnTracks);
-    if (child.hasRelativeLogicalHeight() || oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth) {
+    bool shouldClearContainingBlockLogicalHeight = child.hasRelativeLogicalHeight()
+        || (!child.styleRef().logicalHeight().isSpecified() && itemSpansContentSizedOrFlexibleSizedTracks(cachedGridSpan(child, ForRows), ForRows, ContentSizedAndFlexible));

[Manuel Rego, 2016/03/14 14:38:09]

I've a hard time trying to understand the 2nd part of the ||.
Maybe we need a comment explaining it.

In any case instead of:
  !child.styleRef().logicalHeight().isSpecified()
Maybe it's more explicit:
  child.styleRef().logicalHeight().isIntrinsicOrAuto()

[svillar, 2016/03/17 13:33:54]

Isn't the function name explicit enough?
 
OK

+    if (shouldClearContainingBlockLogicalHeight || oldOverrideContainingBlockContentLogicalWidth != overrideContainingBlockContentLogicalWidth)
         layoutScope.setNeedsLayout(&child, LayoutInvalidationReason::GridChanged);
-    }
 
     bool hasOverrideHeight = child.hasOverrideLogicalContentHeight();
     // We need to clear the stretched height to properly compute logical height during layout.
     if (hasOverrideHeight && child.needsLayout())
         child.clearOverrideLogicalContentHeight();
 
     child.setOverrideContainingBlockContentLogicalWidth(overrideContainingBlockContentLogicalWidth);
     // If |child| has a relative logical height, we shouldn't let it override its intrinsic height, which is
     // what we are interested in here. Thus we need to set the override logical height to -1 (no possible resolution).
-    if (child.hasRelativeLogicalHeight())
+    if (shouldClearContainingBlockLogicalHeight)
         child.setOverrideContainingBlockContentLogicalHeight(LayoutUnit(-1));
     child.layoutIfNeeded();
     // If the child was stretched we should use its intrinsic height.
-    return (hasOverrideHeight ? childIntrinsicHeight(child) : child.logicalHeight()) + child.marginLogicalHeight();
+    return child.logicalHeight() + child.marginLogicalHeight();
 }
 
 LayoutUnit LayoutGrid::minSizeForChild(LayoutBox& child, GridTrackSizingDirection direction, Vector<GridTrack>& columnTracks)
 {
     bool hasOrthogonalWritingMode = child.isHorizontalWritingMode() != isHorizontalWritingMode();
     // TODO(svillar): Properly support orthogonal writing mode.
     if (hasOrthogonalWritingMode)
         return LayoutUnit();
 
     bool isRowAxis = direction == ForColumns;
@@ skipping 65 matching lines @@
     LayoutBox& gridItem() const { return *m_gridItem; }
     GridSpan getGridSpan() const { return m_gridSpan; }
 
     bool operator<(const GridItemWithSpan other) const { return m_gridSpan.integerSpan() < other.m_gridSpan.integerSpan(); }
 
 private:
     LayoutBox* m_gridItem;
     GridSpan m_gridSpan;
 };
 
-bool LayoutGrid::spanningItemCrossesFlexibleSizedTracks(const GridSpan& span, GridTrackSizingDirection direction) const
+bool LayoutGrid::itemSpansContentSizedOrFlexibleSizedTracks(const GridSpan& span, GridTrackSizingDirection direction, SpanMode spanMode) const
 {
+    bool checkContentSized = spanMode == ContentSizedAndFlexible;
     for (const auto& trackPosition : span) {
         const GridTrackSize& trackSize = gridTrackSize(direction, trackPosition);
-        if (trackSize.minTrackBreadth().isFlex() || trackSize.maxTrackBreadth().isFlex())
+        if (trackSize.minTrackBreadth().isFlex() || trackSize.maxTrackBreadth().isFlex()
+            || (checkContentSized && (trackSize.minTrackBreadth().isContentSized() || trackSize.maxTrackBreadth().isContentSized())))
             return true;
     }
 
     return false;
 }
 
 void LayoutGrid::resolveContentBasedTrackSizingFunctions(GridTrackSizingDirection direction, GridSizingData& sizingData)
 {
     sizingData.itemsSortedByIncreasingSpan.shrink(0);
     HashSet<LayoutBox*> itemsSet;
     for (const auto& trackIndex : sizingData.contentSizedTracksIndex) {
         GridIterator iterator(m_grid, direction, trackIndex);
         GridTrack& track = (direction == ForColumns) ? sizingData.columnTracks[trackIndex] : sizingData.rowTracks[trackIndex];
         while (LayoutBox* gridItem = iterator.nextGridItem()) {
             if (itemsSet.add(gridItem).isNewEntry) {
                 const GridSpan& span = cachedGridSpan(*gridItem, direction);
                 if (span.integerSpan() == 1) {
                     resolveContentBasedTrackSizingFunctionsForNonSpanningItems(direction, span, *gridItem, track, sizingData.columnTracks);
-                } else if (!spanningItemCrossesFlexibleSizedTracks(span, direction)) {
+                } else if (!itemSpansContentSizedOrFlexibleSizedTracks(span, direction, FlexibleOnly)) {
                     sizingData.itemsSortedByIncreasingSpan.append(GridItemWithSpan(*gridItem, span));
                 }
             }
         }
     }
     std::sort(sizingData.itemsSortedByIncreasingSpan.begin(), sizingData.itemsSortedByIncreasingSpan.end());
 
     auto it = sizingData.itemsSortedByIncreasingSpan.begin();
     auto end = sizingData.itemsSortedByIncreasingSpan.end();
     while (it != end) {
@@ skipping 811 matching lines @@
         // If we overflow our alignment container and overflow is 'true' (default), we
         // ignore the overflow and just return the value regardless (which may cause data
         // loss as we overflow the 'start' edge).
         return offset;
     }
 
     ASSERT_NOT_REACHED();
     return LayoutUnit();
 }
 
-static inline LayoutUnit constrainedChildIntrinsicContentLogicalHeight(const LayoutBox& child)
-{
-    LayoutUnit childIntrinsicContentLogicalHeight = child.intrinsicContentLogicalHeight();
-    return child.constrainLogicalHeightByMinMax(childIntrinsicContentLogicalHeight + child.borderAndPaddingLogicalHeight(), childIntrinsicContentLogicalHeight);
-}
-
-// FIXME: This logic is shared by LayoutFlexibleBox, so it should be moved to LayoutBox.
-bool LayoutGrid::needToStretchChildLogicalHeight(const LayoutBox& child) const
-{

[Manuel Rego, 2016/03/14 14:38:09]

Shouldn't you remove the method signature in the header?

[svillar, 2016/03/17 13:33:54]

Ups forgot about that.

-    if (ComputedStyle::resolveAlignment(styleRef(), child.styleRef(), ItemPositionStretch) != ItemPositionStretch)
-        return false;
-
-    return isHorizontalWritingMode() && child.style()->height().isAuto();
-}
-
-// FIXME: This logic is shared by LayoutFlexibleBox, so it should be moved to LayoutBox.
-LayoutUnit LayoutGrid::childIntrinsicHeight(const LayoutBox& child) const

[Manuel Rego, 2016/03/14 14:38:09]

Ditto.

-{
-    if (child.isHorizontalWritingMode() && needToStretchChildLogicalHeight(child))
-        return constrainedChildIntrinsicContentLogicalHeight(child);
-    return child.size().height();
-}
-
-// FIXME: This logic is shared by LayoutFlexibleBox, so it should be moved to LayoutBox.
-LayoutUnit LayoutGrid::childIntrinsicWidth(const LayoutBox& child) const

[Manuel Rego, 2016/03/14 14:38:09]

Ditto.

-{
-    if (!child.isHorizontalWritingMode() && needToStretchChildLogicalHeight(child))
-        return constrainedChildIntrinsicContentLogicalHeight(child);
-    return child.size().width();
-}
-
-// FIXME: This logic is shared by LayoutFlexibleBox, so it should be moved to LayoutBox.
-LayoutUnit LayoutGrid::intrinsicLogicalHeightForChild(const LayoutBox& child) const

[Manuel Rego, 2016/03/14 14:38:09]

Ditto.

-{
-    return isHorizontalWritingMode() ? childIntrinsicHeight(child) : childIntrinsicWidth(child);
-}
-
 // FIXME: This logic is shared by LayoutFlexibleBox, so it should be moved to LayoutBox.
 LayoutUnit LayoutGrid::marginLogicalHeightForChild(const LayoutBox& child) const
 {
     return isHorizontalWritingMode() ? child.marginHeight() : child.marginWidth();
 }
 
 LayoutUnit LayoutGrid::computeMarginLogicalHeightForChild(const LayoutBox& child) const
 {
     if (!child.styleRef().hasMargin())
         return LayoutUnit();
@@ skipping 377 matching lines @@
 
     return LayoutPoint(rowAxisOffset, columnAxisOffsetForChild(child));
 }
 
 void LayoutGrid::paintChildren(const PaintInfo& paintInfo, const LayoutPoint& paintOffset) const
 {
     GridPainter(*this).paintChildren(paintInfo, paintOffset);
 }
 
 } // namespace blink