Bug 209460 – [css-grid] grid padding reduces available space for abspos inside grid items

• Component: CSS
• Status: NEW

[Carlos Alberto Lopez Perez]

Created attachment 394343 [details]
test case

What steps will reproduce the problem?
(1) Create a CSS grid with `position: relative; grid: 100px / 100px; padding-left: 20px`
(2) Place a grid item inside it.
(3) Place an abspos element inside the grid item.
(4) Leave the abspos in its static position, but with `grid-column: 1 / 2`
(5) Add two floats inside the abspos, each one with `width: 50px`. Or more generally, any content with total width >80px and <=100px, but which can be broken is parts narrower than <=80px each.

What is the expected result?
The max-content width of the abspos is 50px+50px = 100px, and the grid area is 100px wide, so the abspos has enough space to grow so that the content fits in a single line.

What happens instead?
For some reason, the `padding-left: 20px` of the grid only lets the abspos grow to be 80px wide, so the floats will appear in different lines.

Firefox does this correctly since https://bugzilla.mozilla.org/show_bug.cgi?id=1520584
Chromium its affected, tracked at https://bugs.chromium.org/p/chromium/issues/detail?id=921722

[Carlos Alberto Lopez Perez]

Other test cases are WPT tests css/css-grid/abspos/descendant-static-position-???.html

Created attachment 436888 [details]
Patch

Created attachment 437126 [details]
Patch

[Javier Fernandez]

Comment on attachment 437126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437126&action=review

> Source/WebCore/rendering/RenderGrid.cpp:1002
> +    // For an abspos element that is a child of the grid item but containing block is the grid container,

I'm not sure I understand this comment, or the code it tries to explain. It says, "abspos element that is "child of the grid item", but then in the code you are using "!child.IsGridItem()"; I don't understand it.

Also, the comment says "the containing block is the grid container", but the combination of both clauses is an element that IS NOT a grid item and the RenderGrid element IS NOT the child's containing block; I'm a bit lost, sorry.

> Source/WebCore/rendering/RenderGrid.cpp:1017
> +        setLogicalOffsetForChild(child, ForColumns);

If the abspos is not a grid item, are we calling setLogicalOffsetForChild again ? (didn't we call it in line 1006 already ? )

Created attachment 438034 [details]
Patch

Created attachment 438118 [details]
Patch

[Oriol Brufau]

Comment on attachment 438118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438118&action=review

> Source/WebCore/rendering/RenderBox.cpp:4088
> +            if (containerBlock.isRenderGrid() && !isGridItem() && style().hasStaticInlinePosition(isHorizontalWritingMode()))

Maybe store this in a variable to be reused below? Like

    bool isAbsPosGridChild = containerBlock.isRenderGrid() && !isGridItem() && style().hasStaticInlinePosition(isHorizontalWritingMode();
    if (isAbsPosGridChild)
        availableSpace += absoluteValue(this->logicalLeft() - containerBlock.borderLogicalLeft() - containerLogicalWidth);
    LayoutUnit availableWidth = availableSpace - logicalRightValue;
    computedValues.m_extent = std::min(std::max(preferredMinWidth, availableWidth), preferredWidth);
    logicalLeftValue = availableSpace - (computedValues.m_extent + logicalRightValue);
    if (isAbsPosGridChild)
        logicalLeftValue -= absoluteValue(this->logicalLeft() - containerBlock.borderLogicalLeft() - containerLogicalWidth);

Or maybe

    LayoutUnit absPosGridChildAdjustment = 0;
    if (containerBlock.isRenderGrid() && !isGridItem() && style().hasStaticInlinePosition(isHorizontalWritingMode()) {
        absPosGridChildAdjustment = absoluteValue(this->logicalLeft() - containerBlock.borderLogicalLeft() - containerLogicalWidth);
        availableSpace += absPosGridChildAdjustment;
    }
    LayoutUnit availableWidth = availableSpace - logicalRightValue;
    computedValues.m_extent = std::min(std::max(preferredMinWidth, availableWidth), preferredWidth);
    logicalLeftValue = availableSpace - (computedValues.m_extent + logicalRightValue + absPosGridChildAdjustment);

> Source/WebCore/rendering/RenderGrid.cpp:1004
> +    if (!child.isGridItem() && hasStaticPositionForChild(child, ForColumns))

Again, seems like the conditions could be reused.

    bool isAbsPosWithStaticPositionForColumns = !child.isGridItem() && hasStaticPositionForChild(child, ForColumns);
    if (isAbsPosWithStaticPositionForColumns)
        setLogicalOffsetForChild(child, ForColumns);
    bool isAbsPosWithStaticPositionForRows = !child.isGridItem() && hasStaticPositionForChild(child, ForRows);
    if (isAbsPosWithStaticPositionForRows)
        setLogicalOffsetForChild(child, ForRows);
    // ...
    if (!isAbsPosWithStaticPositionForColumns)
        setLogicalOffsetForChild(child, ForColumns);
    if (!isAbsPosWithStaticPositionForRows)
        setLogicalOffsetForChild(child, ForRows);

[Oriol Brufau]

(In reply to Oriol Brufau from comment #7)
>     bool isAbsPosGridChild = containerBlock.isRenderGrid() && !isGridItem()
> && style().hasStaticInlinePosition(isHorizontalWritingMode();

isAbsPosGridItemChild I guess, or whichever name sounds good.

[Oriol Brufau]

The patch seems bad for dynamic pages. Add this at the end of the attached testcase:

<script>
absolute.style.marginLeft = "200px";
absolute.offsetLeft;
absolute.style.marginLeft = "150px";
</script>

Then the content area of the abspos is 100px wide, but due to the margin it should only be the min-content, 50px.

Created attachment 445273 [details]
Patch

[Oriol Brufau]

Comment on attachment 445273 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445273&action=review

> Source/WebCore/rendering/RenderBox.h:676
> +    bool isAbsPosGridChild() const  { return !isGridItem() && style().hasStaticInlinePosition(isHorizontalWritingMode()); }

The name doesn't describe well what this does, e.g. it doesn't check that the containing block is a grid container. Though when calling from RenderGrid we don't need to check that...
Maybe move the method into RenderGrid, call it on the containing block, passing the child as an argument?
Then it will be clear that it's just for grid layout even if you don't check that.
Maybe call it RenderGrid::NonDirectChildHasStaticInlinePosition or something.

Also, I think the isHorizontalWritingMode() should be called on the containing block. Otherwise, the testcase shows some wrong red when adding

  #absolute {
    writing-mode: vertical-lr;
    left: 50px;
  }

or

  #grid {
    writing-mode: vertical-lr;
  }
  #absolute {
    writing-mode: horizontal-tb;
    top: 50px;
  }

Can you add these variations as new WPTs?

> Source/WebCore/rendering/RenderGrid.cpp:1046
> +        gridAreaPositionForOutOfFlowChild(child, ForColumns, m_startOfColumnForChild, m_endOfColumnForChild);

Maybe the value should be cleared after RenderBlock::layoutPositionedObject?
It doesn't seem harmful, but it seems strange to remember the value for the last abspos that matches this condition...

Also, maybe the value could be a pointer, or a std::optional<LayoutUnit>.
At the start of RenderGrid::layoutPositionedObject it would be nullptr or !has_value(), here we would set the value, and then clear it again before leaving the function.
Then RenderBox::computePositionedLogicalWidthUsing would only have to check if the value is set, without having to check !isGridItem() and style().hasStaticInlinePosition()

> Source/WebCore/rendering/RenderGrid.cpp:1047
> +        gridAreaPositionForOutOfFlowChild(child, ForRows, m_startOfRowForChild, m_endOfRowForChild);

No need for ForRows calculation if it's not used.

> Source/WebCore/rendering/RenderGrid.h:215
> +    LayoutUnit m_endOfColumnForChild { 0 };

No need for m_endOfColumnForChild, m_startOfRowForChild and m_endOfRowForChild if they are not used?

Created attachment 445567 [details]
Patch

Created attachment 445570 [details]
Patch

[Oriol Brufau]

Comment on attachment 445570 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445570&action=review

> Source/WebCore/rendering/RenderBox.cpp:4151
> +        if (containerBlock.isRenderGrid() && downcast<RenderGrid>(containerBlock).startOfColumnForNonDirectChildWithStaticInlinePosition())

Just `if (containerBlock.isRenderGrid())` is enough.

> Source/WebCore/rendering/RenderGrid.cpp:1035
> +bool RenderGrid::nonDirectChildHasStaticInlinePosition(const RenderBox& child) const

The function now has a single caller, so you don't need it.

> Source/WebCore/rendering/RenderGrid.cpp:1050
> +        LayoutUnit startOfColumnForChild;

Not an expert in memory management. But I worry that if the variable is declared inside the block, it can die when leaving the block, causing a use-after-free when using the pointer.
Better declare it outside the conditional.

> Source/WebCore/rendering/RenderGrid.cpp:1064
> +    if (m_startOfColumnForNonDirectChildWithStaticInlinePosition)

Is there any benefit in checking the condition instead of just assigning to null?

> Source/WebCore/rendering/RenderGrid.h:85
> +    LayoutUnit* startOfColumnForNonDirectChildWithStaticInlinePosition() const { return m_startOfColumnForNonDirectChildWithStaticInlinePosition; }

Maybe the name should make it clear that this is basically a temporary value not always available?
Possibly adding "cached" in the name, and then no need to be that comprehensive with the rest of the name.
cachedStartOfColumnForChild? cachedStartOfColumnForChildIfNeeded? startOfColumnForComputingPositionedLogicalWidth?
Not very good at naming things

[Oriol Brufau]

Comment on attachment 445570 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445570&action=review

> Source/WebCore/rendering/RenderGrid.cpp:1048
> +    if (nonDirectChildHasStaticInlinePosition(child)) {

Nit: assert that m_startOfColumnForNonDirectChildWithStaticInlinePosition is null before the conditional.

[Oriol Brufau]

Comment on attachment 445570 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445570&action=review

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/abspos/descendant-static-position-005-expected.html:43
> +<div style="margin-top:20px;">

Instead of adding these divs (which are not closed and the nested structure is more tedious to inspect with devtools), why not

  .grid { margin-bottom: 20px }

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/abspos/descendant-static-position-005.html:54
> +<div style="margin-top:20px;">

Ditto.

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/abspos/descendant-static-position-006-expected.html:22
> +  float: top;

This is invalid

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/abspos/descendant-static-position-006.html:31
> +  float: top;

Ditto

Created attachment 445828 [details]
Patch

[Oriol Brufau]

Comment on attachment 445828 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445828&action=review

> Source/WebCore/rendering/RenderBox.cpp:4166
> +                absPosGridChildAdjustment = absoluteValue(*startOfColumn - containerBlock.borderLogicalLeft());

Is the absoluteValue() needed here? If so, why not in the "RULE 3" case below?

> Source/WebCore/rendering/RenderBox.cpp:4167
> +            availableSpace += absPosGridChildAdjustment;

Nit: instead of

  availableSpace += absPosGridChildAdjustment;
  LayoutUnit availableWidth = availableSpace - logicalRightValue;
  computedValues.m_extent = std::min(std::max(preferredMinWidth, availableWidth), preferredWidth);
  logicalLeftValue = availableSpace - absPosGridChildAdjustment - (computedValues.m_extent + logicalRightValue);

you could use

  LayoutUnit availableWidth = availableSpace + absPosGridChildAdjustment - logicalRightValue;
  computedValues.m_extent = std::min(std::max(preferredMinWidth, availableWidth), preferredWidth);
  logicalLeftValue = availableSpace - (computedValues.m_extent + logicalRightValue);

so you avoid having to subtract absPosGridChildAdjustment at the end.

> Source/WebCore/rendering/RenderBox.cpp:4182
> +            availableSpace += absPosGridChildAdjustment;

Nit: for consistency with the above,

  availableSpace += absPosGridChildAdjustment;
  LayoutUnit availableWidth = availableSpace - logicalLeftValue;

could be

  LayoutUnit availableWidth = availableSpace + absPosGridChildAdjustment - logicalLeftValue;

Created attachment 446023 [details]
Patch

(In reply to Oriol Brufau from comment #18)
> Comment on attachment 445828 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445828&action=review
> 
> > Source/WebCore/rendering/RenderBox.cpp:4166
> > +                absPosGridChildAdjustment = absoluteValue(*startOfColumn - containerBlock.borderLogicalLeft());
> 
> Is the absoluteValue() needed here? If so, why not in the "RULE 3" case
> below?
>

Because the m_columnPositions's data is stored ignoring the direction. *startOfColumn can be a value less than 0 for RTL writing mode. And here we are looking at available width adjustment.
 
> > Source/WebCore/rendering/RenderBox.cpp:4167
> > +            availableSpace += absPosGridChildAdjustment;
> 
> Nit: instead of
> 
>   availableSpace += absPosGridChildAdjustment;
>   LayoutUnit availableWidth = availableSpace - logicalRightValue;
>   computedValues.m_extent = std::min(std::max(preferredMinWidth,
> availableWidth), preferredWidth);
>   logicalLeftValue = availableSpace - absPosGridChildAdjustment -
> (computedValues.m_extent + logicalRightValue);
> 
> you could use
> 
>   LayoutUnit availableWidth = availableSpace + absPosGridChildAdjustment -
> logicalRightValue;
>   computedValues.m_extent = std::min(std::max(preferredMinWidth,
> availableWidth), preferredWidth);
>   logicalLeftValue = availableSpace - (computedValues.m_extent +
> logicalRightValue);
> 
> so you avoid having to subtract absPosGridChildAdjustment at the end.
> 
> > Source/WebCore/rendering/RenderBox.cpp:4182
> > +            availableSpace += absPosGridChildAdjustment;
> 
> Nit: for consistency with the above,
> 
>   availableSpace += absPosGridChildAdjustment;
>   LayoutUnit availableWidth = availableSpace - logicalLeftValue;
> 
> could be
> 
>   LayoutUnit availableWidth = availableSpace + absPosGridChildAdjustment -
> logicalLeftValue;

Patch updated. Thank very much!

[Oriol Brufau]

Comment on attachment 446023 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446023&action=review

> Source/WebCore/rendering/RenderBox.cpp:4166
> +                absPosGridChildAdjustment = absoluteValue(*startOfColumn - containerBlock.borderLogicalLeft());

absoluteValue() still seems wrong. Add this to the bug testcase:

  /* Negative RenderGrid::startOfColumnForChild() */
  #grid {
    width: 0px;
    justify-content: center;
  }
  /* Use Rule 1 */
  #absolute {
    writing-mode: vertical-lr;
    inset-inline-end: 0;
  }

Then you will see red, unlike Firefox and Chromium.
Removing absoluteValue() fixes it?

[Oriol Brufau]

Comment on attachment 446023 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446023&action=review

>> Source/WebCore/rendering/RenderBox.cpp:4166
>> +                absPosGridChildAdjustment = absoluteValue(*startOfColumn - containerBlock.borderLogicalLeft());
> 
> absoluteValue() still seems wrong. Add this to the bug testcase:
> 
>   /* Negative RenderGrid::startOfColumnForChild() */
>   #grid {
>     width: 0px;
>     justify-content: center;
>   }
>   /* Use Rule 1 */
>   #absolute {
>     writing-mode: vertical-lr;
>     inset-inline-end: 0;
>   }
> 
> Then you will see red, unlike Firefox and Chromium.
> Removing absoluteValue() fixes it?

Also, this seems broken in orthogonal cases in general (not just negative values), because startOfColumn and containerBlock.borderLogicalLeft() are based on the writing mode of the grid.

<!DOCTYPE html>
<html>
<meta charset="utf-8" />
<style>
#grid {
  position: relative;
  display: grid;
  grid: 100px / 100px;
  border: 3px solid;
  padding-left: 20px;
  width: 0px;
}
#absolute {
  inset-inline-end: 0;
  writing-mode: vertical-lr;
  line-height: 0;
  position: absolute;
  background: red;
  grid-column: 1 / 2;
}
#absolute > div {
  float: left;
  width: 50px;
  height: 100px;
  background: green;
}
</style>
There should be no red:
<div id="grid">
  <div>
    <div id="absolute">
      <div></div>
      <div></div>
    </div>
  </div>
</div>
</html>

This is another case of "containing block is not parent" for abpos as bug 233812. 
A generic solution for both grid and flexbox probably is the way forward in long term. I'd prefer to this bug open for anyone who has time and effort to take it.

[Radar WebKit Bug Importer]

<rdar://problem/99778160>