Closed Bug 1707643 Opened 1 year ago Closed 6 months ago

Firefox doesn't properly account for border/padding, when computing static pos of abspos grid children with end/center alignment

Categories

(Core :: Layout: Grid, defect)

defect

Tracking

()

RESOLVED FIXED
96 Branch
Tracking Status
firefox96 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

STR:

  1. Load this testcase:
data:text/html,<div style="display:grid;align-items:end; height: 200px; padding: 50px; background: teal"><div style="position:absolute">Inside-teal

EXPECTED RESULTS:
The "Inside-teal" text should be inside the teal area (specifically, 50px above the bottom of it)

ACTUAL RESULTS:
The text is outside the teal area.

It seems like we're not properly accounting for border/padding when determining the end position (and similar for "center", not tested in this testcase)

We apparently have some WPT tests (written by me) that improperly expect our current rendering. I think Chromium folks may be fixing those tests in https://chromium-review.googlesource.com/c/chromium/src/+/2803842 , and we'll start failing those tests after that point. (and we'll start passing them again when we fix this bug) (Or they just might mark them as expected-fail on their end; not sure.)

Those tests with wrong expectations are (according to :IanK):

css/css-grid/abspos/grid-abspos-staticpos-align-self-002.html
css/css-grid/abspos/grid-abspos-staticpos-align-self-rtl-003.html
css/css-grid/abspos/grid-abspos-staticpos-align-self-rtl-004.html
css/css-grid/abspos/grid-abspos-staticpos-justify-self-002.html
css/css-grid/abspos/grid-abspos-staticpos-justify-self-rtl-003.html
css/css-grid/abspos/grid-abspos-staticpos-justify-self-rtl-004.html

I just looked at the first one, and I agree the test's expectations (and Firefox's rendering of the test) are wrong in how they account for border & padding when determining the end alignment positions (at least).

Marking this as blocking the "compat2021" metabug, since these tests are included in https://github.com/Ecosystem-Infra/wpt-results-analysis/blob/main/compat-2021/css-grid-tests.txt

Blocks: compat2021

It looks like this is actually responsible for 12 test failures -- all of the grid-abspos-staticpos-* ones listed here:
https://wpt.fyi/results/css/css-grid/abspos?label=master&label=experimental&aligned&q=firefox%3Afail

The 12 failing tests are the six from comment 1, plus these six:

grid-abspos-staticpos-align-self-img-002.html
grid-abspos-staticpos-align-self-vertWM-003.html
grid-abspos-staticpos-align-self-vertWM-004.html
grid-abspos-staticpos-justify-self-img-002.html
grid-abspos-staticpos-justify-self-vertWM-003.html
grid-abspos-staticpos-justify-self-vertWM-004.html
Assignee: nobody → dholbert
Status: NEW → ASSIGNED

The main issue here is that we're aligning within the grid's padding-box (or rather, we're aligning within a box that's the size of the grid's padding-box -- but then we use the result as an offset within the grid's content-box).

We're using the padding-box for this because the spec used to say so, as quoted in bug 1269017 comment 0 (The static position [CSS21] of an absolutely-positioned child of a grid container is determined as if it were the sole grid item in a grid area whose edges coincide with the padding edges of the grid container.)

The relevant spec text now says to use the content edges of the grid container, though:
https://drafts.csswg.org/css-align/#staticpos-rect
https://drafts.csswg.org/css-grid/#static-position

One exception is for abspos content for whom the grid container also forms the abspos containing block -- in that case, we are still supposed to use the padding edges by default, as laid out in grid section 10.1 (an auto value for a grid-placement property contributes a special line to the placement whose position is that of the corresponding padding edge of the grid container). So the padding-box is still the relevant alignment container for those cases. But IIRC, that's not relevant to the WPT tests that are failing here; the grid container is not the abspos containing block in any of those tests.

Depends on: 1269017

DONTBUILD since this patch is just making some non-functional changes to tests.

These tests were previously split out from the more general alignment tests
(which live alongside them without "last-baseline" in their filename). These
last-baseline-specific tests are meant to compmartmentalize the "last baseline"
alignment value, since not all browser engines implement that yet. (And then
the more general sibling-test covers all of the other alignment modes.)

In that previous splitting-out task, the last-baseline reference cases carried
along some cruft that turned out not to be necessary. In particular: they only
need to mock up "end" alignment, but they received some copypasted CSS rules
for "start" and "center" alignemnt as well, and those rules are unused.

This patch removes those unused rules. It also makes a cosmetic change to
grid-abspos-staticpos-justify-self-img-last-baseline-002.html (and its
reference case), to match the styling used in its non-"last-baseline" analog
(grid-abspos-staticpos-justify-self-img-002.html). This cosmetic change just
avoids near-overlap between adjacent content in the test, and doesn't impact
the actual functionality being tested.

In the conditions where we hit this partiuclar codepath, the spec used to
require us to align within the grid's padding box (which is what we were trying
to do[1]), but now it requires us to align within the grid's content box.

This patch makes us start passing a bunch of WPT tests (whose .ini files I'm
removing here).

This patch also changes the reference case for some "last-baseline"-specific
tests in the same directory, by cribbing the "alignEnd" rule from the
neighboring non-"last-baseline" analog (since "last-baseline" falls back to end
alignment and is meant to be equivalent in these testcases). Before this
patch, the affected reference cases were inadvertently depending on the Firefox
bug that I'm fixing here, and they were expecting the testcase's end-aligned
content to be at the wrong position as a result. Chromium developers fixed
this issue in the neighboring non-"last-baseline" tests in [2], but they didn't
catch the files that I'm fixing here, because Chromium doesn't implement "last
baseline" alignment yet.

[1] Before this patch, we actually weren't quite aligning within the
padding-box, even though we were trying to. We were computing an alignment
offset based on the size of the padding box, and then we were using that offset
as a position within the content box. This could result in us pushing
end-aligned content outside the grid entirely, as in the testcase on the bug
report for this bug.

[2] https://chromium-review.googlesource.com/c/chromium/src/+/2803842

Depends on D130712

The additional 6 tests in comment 6 nearly pass with the patch, modulo what looks like a test-bug for the alignCenter cases.

Tomorrow I'll post a "part 3" to fix those tests. In the meantime they're annotated as failing which is still technically correct, though they're nearly-passing now. And I'll go ahead and land parts 1 and 2.

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f09021021385
part 1: Non-functional test cleanup (mostly in reference cases) for grid "last-baseline" alignment tests. r=jwatt,emilio
https://hg.mozilla.org/integration/autoland/rev/a4c82c6495f6
part 2: When computing static position of abspos grid child, perform alignment within content-box, rather than padding box. r=emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/31555 for changes under testing/web-platform/tests

tl;dr: an easy way to verify that this change is correct: the offset for
center-alignment must be precisely half of the offset for end-alignment (and
it is, after this patch).

Some history of these tests & reasoning behind this change:

The tests that I'm editing in this patch were all originally written[1] with an
assumption that the static positions of abspos grid children should be
determined by performing alignment (e.g. centering) inside of the grid
container's padding box. That is what the spec used to require, but that
changed -- now the spec says to align within the grid container's content
box
, as discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1707643#c4 .
This bug's previous patch implemented that change, but that wasn't quite enough
to make us pass these tests, for reasons described below.

Earlier this year, the Chromium folks landed[2] some patches to fix these tests
(per the aforementioned spec change), to make the tests expect that alignment
happens within the grid's content box. But that fix was incomplete. It
correctly updated these tests' .alignEnd cases, to reduce the offsets by 2px
(to account for the smaller box that we're doing alignment within); but it did
not fix the .alignCenter cases to make a corresponding change there. Really,
the .alignCenter cases needed a 1px change to match the .alignEnd cases' 2px
change (since a center-alignment offset is exactly half of an end-alignment
offset).

This patch ties off that loose end by fixing the .alignCenter case in these
tests as well. This restores the invariant that the reference cases'
.alignCenter offsets are all exactly half of the .alignEnd offsets.

[1] in e.g. https://hg.mozilla.org/mozilla-central/rev/ef2f15749bb9#l7.3

[2] in e.g. https://github.com/web-platform-tests/wpt/pull/29435/commits/10a79b147d2d4220d1cf9ccea3920840d7c1f140

Attachment #9249925 - Attachment description: Bug 1707643 part 3: Fix mistakes in center-alignment reference-cases in some abspos grid item WPT testcases, and mark them as passing. r?#layout-reviewers → Bug 1707643 part 3: Fix mistakes in center-alignment reference-cases in some abspos grid item WPT tests, and mark them as passing. r?#layout-reviewers
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cce2b134d299
part 3: Fix mistakes in center-alignment reference-cases in some abspos grid item WPT tests, and mark them as passing. r=emilio
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.