1520584 - [css-grid] grid padding reduces available space for abspos inside grid items

Status: RESOLVED FIXED

(Core :: Layout: Grid, defect, P3)

Description

[Oriol Brufau [:Oriol]]

Attachment: testcase.htm

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.

Chromium is also affected: https://crbug.com/921722. Edge does it correctly.

Comment 1

[Mats Palmgren (inactive)]

Seems like a valid bug to me, both intuitively and after reading the specs:
https://drafts.csswg.org/css-align-3/#staticpos-rect
https://drafts.csswg.org/css-grid/#abspos-items

Comment 2

[Mats Palmgren (inactive)]

Attachment: wip

This seems to fix it for me. It appears we calculate the "hypothetical
position" correctly but we fail to translate it into the right coordinate
space.

Oriol, does the WPT in this patch pass in Edge?

Comment 3

[Oriol Brufau [:Oriol]]

(In reply to Mats Palmgren (:mats) from comment #2)

Oriol, does the WPT in this patch pass in Edge?

Yes, Edge passes it (after replacing the logical size properties with the physical ones).

Comment 4

[Mats Palmgren (inactive)]

Thanks. I'll change it to use physical sizes instead.

(The test did test different writing-modes at one point too,
so it was convenient to use logical sizes, but I decided to
treat that a separate bug since it's not specific to just
Grid layout: bug 1521988.)

Comment 5

[Mats Palmgren (inactive)]

Attachment: fix+wpt

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 9038497 [details] [diff] [review]
fix+wpt

Review of attachment 9038497 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/ReflowInput.cpp
@@ +1666,5 @@
>        hypotheticalPos.mBStart = nscoord(0);
>      } else {
>        CalculateHypotheticalPosition(aPresContext, placeholderFrame,
>                                      aReflowInput, hypotheticalPos, aFrameType);
> +      if (aReflowInput->mFrame->IsGridContainerFrame()) {

(We really should rename |aReflowInput|, maybe to aCBReflowInput. I stared at this chunk, confused for a bit, not realizing it was for the *CB*.)

Anyway, not the fault of this patch, but if you feel like fixing it in a followup while you're here, then all the better. :)

@@ +1673,5 @@
> +        // 'hypotheticalPos' to be relative that rectangle here.
> +        nsRect cb = nsGridContainerFrame::GridItemCB(mFrame);
> +        LogicalMargin offsets(cbwm, nsMargin(cb.Y(), 0, 0, cb.X()));
> +        hypotheticalPos.mIStart -= offsets.IStart(cbwm);
> +        hypotheticalPos.mBStart -= offsets.BStart(cbwm);

Is this correct if we're in a RTL writing mode, for example?  In that scenario, I'd think mIStart doesn't care at all about cb.X() -- rather, it cares about cb.XMost().

(I think perhaps your nsMargin needs nonzero values for the far ends?)


I'm not talking about orthogonal flows like in bug 1521988 -- just a scenario with a global non-LTR writing mode.

Comment 7

[Mats Palmgren (inactive)]

Attachment: fix+wpt

(In reply to Daniel Holbert [:dholbert] from comment #6)

Comment on attachment 9038497 [details] [diff] [review]
(We really should rename |aReflowInput|, maybe to aCBReflowInput. I stared
at this chunk, confused for a bit, not realizing it was for the CB.)

It's fairly well documented in the comments though:
https://searchfox.org/mozilla-central/rev/c035ee7d3a5cd6913e7143e1bce549ffb4a566ff/layout/generic/ReflowInput.cpp#1341
https://searchfox.org/mozilla-central/rev/c035ee7d3a5cd6913e7143e1bce549ffb4a566ff/layout/generic/ReflowInput.h#1004-1005
but yeah, renaming it would make it clearer.

Is this correct if we're in a RTL writing mode, for example?

I only intended to wallpaper the common case, tbh. We need to
rewrite this code anyway for writing-mode correctness.
Anyway, I added code to handle horizontal-tb/rtl too.
(I'll punt on the rest to bug 1521988 since it's lower priority.)

Comment 8

[Mats Palmgren (inactive)]

Attachment: s/aReflowInput/aCBReflowInput/ for clarity

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 9039066 [details] [diff] [review]
fix+wpt

Review of attachment 9039066 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsAbsoluteContainingBlock.cpp
@@ -735,5 @@
> -  if (aContainingBlock.TopLeft() != nsPoint(0, 0)) {
> -    const nsStyleSides& offsets = kidReflowInput.mStylePosition->mOffset;
> -    if (!(offsets.GetLeftUnit() == eStyleUnit_Auto &&
> -          offsets.GetRightUnit() == eStyleUnit_Auto) ||
> -        (rsFlags & ReflowInput::STATIC_POS_IS_CB_ORIGIN)) {

The removal of this "auto/auto" exception worries me a bit, for the usual non-grid case... (But maybe I'm just not understanding it)

My perhaps-incorrect understanding is that the "r.x += aContainingBlock.x" arithmetic was meant to handle cases where:
 - we have left:10px *and* some nonzero padding-left on the CB
 - ...so we need to add in that CB padding (which is given as aContainingBlock.x)
 - ...UNLESS the frame is at its static position which was taken from the placeholder position (i.e. left==right==auto and STATIC_POS_IS_CB_ORIGIN flag isn't set).

With your adjustment here, that "UNLESS" opt-out seems to have been removed. So it looks superficially like this shifts even left:auto;right:auto frames based on the CB padding area, which seems like it should be an unnecessarily/redundant adjustment to perform when the placeholder is already in the corrects spot.

Could you clarify what I might be missing here, and perhaps adjust the "Offset the frame rect..." comment to make it clearer why we need to do this adjustment, even in that left:auto/right:auto scenario (in the simple/usual no-grids-involved case)?

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 9039066 [details] [diff] [review]
fix+wpt

Review of attachment 9039066 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/ReflowInput.cpp
@@ +1679,5 @@
> +          left = cb.X();
> +        } else {
> +          right = aReflowInput->ComputedWidth() +
> +                  aReflowInput->ComputedPhysicalBorderPadding().right -
> +                  cb.XMost();

I think this arithmetic is wrong... I think you really want to be finding the delta between the right edge of the padding-box and cb.XMost() (which is a rect in the coordinate system of the padding box[1]), correct?

To do that, I think you want the second term here to be
  aReflowInput->ComputedPhysicalPadding().LeftRight()

(Notably: including both sides of the padding, to get the full width of the padding box; and disregarding the border since none of our coordinate systems here are border-box-relative AFAICT.)

In particular: if that `cb` rect were snapped to the right edge of the padding box, then we'd *want* to end up with right==0. And in that scenario, I think cb.XMost() would be the full width of the padding box, which means the first two terms have to add up to the full width of the padding box in order to produce a result of 0 from the subtraction.


[1] https://searchfox.org/mozilla-central/source/layout/generic/nsGridContainerFrame.cpp#5478-5480

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 9039067 [details] [diff] [review]
s/aReflowInput/aCBReflowInput/ for clarity

Review of attachment 9039067 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the rename - thanks for doing that!

::: layout/generic/ReflowInput.cpp
@@ +1666,5 @@
>        hypotheticalPos.mBStart = nscoord(0);
>      } else {
>        // XXXmats all this is broken for orthogonal writing-modes: bug 1521988.
>        CalculateHypotheticalPosition(aPresContext, placeholderFrame,
> +                                    aCBReflowInput, hypotheticalPos, aFrameType);

Nit:  ./mach clang-format wants you to rewrap this line (the 2 extra characters make it too long, so "aFrameType" needs to bump down to its own line)

Comment 12

[Daniel Holbert [:dholbert]]

Following up a bit more on comment 9: I'm uneasy that we're removing some code that seems to be active in no-grids-involved scenarios, and which was added in a no-grids-involved patch [1], and we're replacing it with some grid-specific code.

This seems like it'd have side effects / regressions in some no-grids-involved abs/fixed-pos behavior, though I don't know precisely what.

(Though maybe I'd be more at ease if I had a better understanding of what the removed code was aiming to do precisely and how the new code does the same thing but better. :))

[1] https://hg.mozilla.org/mozilla-central/rev/abc416eac56f#l1.174

Comment 13

[Mats Palmgren (inactive)]

Attachment: fix+wpt

(In reply to Daniel Holbert [:dholbert] from comment #10)

•      right = aReflowInput->ComputedWidth() +
  

•              aReflowInput->ComputedPhysicalBorderPadding().right -
  

•              cb.XMost();
  

I think this arithmetic is wrong... I think you really want to be finding
the delta between the right edge of the padding-box and cb.XMost() (which is
a rect in the coordinate system of the padding box[1]), correct?

You're absolutely right, good catch! The tests had a right border-width
that matched the left padding so they worked, unfortunately. I've tweaked
them now so that the RTL case fails with the last patch. I also added
a couple of tests that have the grid origin at a non-zero offset from
the content edge for good measure. I suspect it might be an edge case
that we currently don't have any test for.

(In reply to Daniel Holbert [:dholbert] from comment #12)

Following up a bit more on comment 9: I'm uneasy that we're removing some code that seems to be active in no-grids-involved scenarios, and which was added in a no-grids-involved patch [1], and we're replacing it with some grid-specific code.

Right, so that code came from bug 856932 (which notably landed without
a regression test). In particular, the aContainingBlock.x/y was
the result of PresShell()->GetContentDocumentFixedPositionMargins()
which was used to offset the nsViewportFrame (for the URL bar on
Android, IIUC). That code was later removed as part of a massive
rewrite in bug 1180295, in https://hg.mozilla.org/mozilla-central/diff/258257/layout/generic/nsViewportFrame.cpp
to be precise.

However, it seems to me that the original code that made an
exception for auto/auto was simply wrong. I see no reason
a static position shouldn't also be offset from the viewport
edge. I'm only guessing of course since that code has been
removed now and there's no regression test for it.

I think I prefer to always apply it for simplicity and leave
the onus on any future hacks like that to re-introduce
the auto/auto exception if needed. Hopefully with a better
explanation of why it's needed and with a regression test
this time.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 9039363 [details] [diff] [review]
fix+wpt

Review of attachment 9039363 [details] [diff] [review]:
-----------------------------------------------------------------

OK, thanks for the explanation. r=me

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/280344f386b2
part 1 - [css-align][css-grid] Translate the static position to grid area coordinates.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/e12caed89db3
part 2 - s/aReflowInput/aCBReflowInput/ for clarity.  r=dholbert

Comment 16

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/280344f386b2
https://hg.mozilla.org/mozilla-central/rev/e12caed89db3

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15163 for changes under testing/web-platform/tests