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

RESOLVED FIXED in Firefox 67

Status

()

defect
P3
normal
RESOLVED FIXED
5 months ago
3 months ago

People

(Reporter: Oriol, Assigned: mats)

Tracking

(Blocks 1 bug)

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Reporter

Description

5 months ago
Posted file 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.

Priority: -- → P3
Assignee

Comment 1

5 months ago

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

Assignee

Comment 2

5 months ago
Posted patch wip (obsolete) — Splinter Review

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?

Assignee: nobody → mats
Assignee

Updated

5 months ago
Flags: needinfo?(oriol-bugzilla)
Assignee

Updated

5 months ago
Blocks: 1521988
Reporter

Comment 3

5 months ago

(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).

Flags: needinfo?(oriol-bugzilla)
Assignee

Comment 4

5 months ago

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.)

Assignee

Comment 5

5 months ago
Posted patch fix+wpt (obsolete) — Splinter Review
Attachment #9038387 - Attachment is obsolete: true
Attachment #9038497 - Flags: review?(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.
Assignee

Comment 7

5 months ago
Posted patch fix+wpt (obsolete) — Splinter Review

(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.)

Attachment #9038497 - Attachment is obsolete: true
Attachment #9038497 - Flags: review?(dholbert)
Attachment #9039066 - Flags: review?(dholbert)
Assignee

Comment 8

5 months ago
Attachment #9039067 - Flags: review?(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 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 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)
Attachment #9039067 - Flags: review?(dholbert) → review+

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

Assignee

Comment 13

5 months ago
Posted patch fix+wptSplinter Review

(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.

Attachment #9039363 - Flags: review?(dholbert)
Assignee

Updated

5 months ago
Attachment #9039066 - Attachment is obsolete: true
Attachment #9039066 - Flags: review?(dholbert)
Comment on attachment 9039363 [details] [diff] [review]
fix+wpt

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

OK, thanks for the explanation. r=me
Attachment #9039363 - Flags: review?(dholbert) → review+

Comment 15

5 months ago
Pushed by mpalmgren@mozilla.com:
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

5 months ago
bugherder
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15163 for changes under testing/web-platform/tests
Depends on: 1535986
You need to log in before you can comment on or make changes to this bug.