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)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox96 | --- | fixed |
People
(Reporter: dholbert, Assigned: dholbert)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
STR:
- 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.)
|
Assignee | |
Comment 1•4 years ago
|
||
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).
|
|
Assignee | |
Comment 2•4 years ago
|
||
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
|
|
Assignee | |
Comment 3•3 years ago
•
|
||
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 | |
Updated•3 years ago
|
|
|
Assignee | |
Comment 4•3 years ago
•
|
||
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.
|
|
Assignee | |
Comment 5•3 years ago
|
||
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.
|
|
Assignee | |
Comment 6•3 years ago
|
||
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
|
|
Assignee | |
Comment 7•3 years ago
•
|
||
|
|
Assignee | |
Comment 8•3 years ago
|
||
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.
|
|
Assignee | |
Comment 11•3 years ago
|
||
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
|
||
Updated•3 years ago
|
|
||
Comment 12•3 years ago
|
||
|
||
Comment 13•3 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/f09021021385
https://hg.mozilla.org/mozilla-central/rev/a4c82c6495f6
https://hg.mozilla.org/mozilla-central/rev/cce2b134d299
Description
•