Closed Bug 1881495 Opened 1 year ago Closed 1 year ago

grid with place-items: center and fixed height lets children overflow with height 100%

Categories

(Core :: Layout: Grid, defect)

Product:
Core
Component:
Layout: Grid
Version:
Firefox 123
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
125 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- unaffected
firefox123 --- wontfix
firefox124 --- verified
firefox125 --- verified

People

(Reporter: o.zwagers, Assigned: dholbert)

References

(Regression)

Details

(Keywords: regression)

Attachments

(11 files)

firefox bug grid items center.jpg
240.91 KB, image/jpeg
Details
Test-case from comment 0.
656 bytes, text/html
Details
testcase with a variety of 'justify-self' values
1.35 KB, text/html
Details
Bug 1881495 part 2: Add WPT for justify-self alignment of a percent-height grid item with an aspect-ratio. r?TYLin
48 bytes, text/x-phabricator-request
Details | Review
Bug 1881495 part 1: Add ShrinkWrap flag for grid items that are self-aligned (not stretched) in MeasuringReflow(). r?dholbert
48 bytes, text/x-phabricator-request
Details | Review
Bug 1881495 part 3: Add two additional WPT tests, adding display:table to the aligned grid items. r?TYLin
48 bytes, text/x-phabricator-request
Details | Review
Bug 1881495 part 4: Add a reftest-style WPT to test the linewrapping issue from the duplicate bug. r?TYLin
48 bytes, text/x-phabricator-request
Details | Review
Bug 1881495 part 1: Add ShrinkWrap flag for grid items that are self-aligned (not stretched) in MeasuringReflow(). r?dholbert
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-beta+
Details | Review
Bug 1881495 part 2: Add WPT for justify-self alignment of a percent-height grid item with an aspect-ratio. r?TYLin
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-beta+
Details | Review
Bug 1881495 part 3: Add two additional WPT tests, adding display:table to the aligned grid items. r?TYLin
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-beta+
Details | Review
Bug 1881495 part 4: Add a reftest-style WPT to test the linewrapping issue from the duplicate bug. r?TYLin
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-beta+
Details | Review

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:123.0) Gecko/20100101 Firefox/123.0

Steps to reproduce:

i have a grid that is used as a container to center a item within it. I use the place-items: center property to do so. The container has a fixed height (lets say 240px) and the child has a dynamic height of 100%.

Actual results:

the child is overflowing the container and therefore messing up the other content on my page.

This happens to me in firefox 123 and 125 (nightly)

Expected results:

on firefox 120, safari, and chrome the child is contained in the container with a fixed height. So when the container is 240px, the child takes up all the space in the container (but no more) and therefore also 240px

The Bugbug bot thinks this bug should belong to the 'Core::Layout: Grid' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Component: Untriaged → Layout: Grid
Product: Firefox → Core

when setting the propertyjustify-content to center the child won't overflow (not that this is a fix, but maybe it will help while debugging the problem).

Component: Layout: Grid → Untriaged
Product: Core → Firefox
Component: Untriaged → Layout: Grid
Product: Firefox → Core

Can you attach a testcase to this bug?

Flags: needinfo?(o.zwagers)

I have attached a piece of HTML that would recreate the problem. Opening the page in safari, chrome and firefox 120 would show a gray bar which contains a aqua colored square. Firefox 123 has a aqua colored square that is full width and overflows that grid container.

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <title>Document</title>

    <style>
        .container {
            height: 20rem;
            width: 100%;
            display: grid;
            place-items: center;    
            background-color: whitesmoke;
        }

        .child {
            height: 100%;
            background-color: aquamarine;
            aspect-ratio: 9 / 16;
        }
    </style>
</head>
<body>
    <div class="container">
        <div class="child">
        </div>
    </div>
</body>
</html>
Flags: needinfo?(o.zwagers)

Thanks for the testcase!
Bisection points to:

Bug 1350037 Part 3 - Prevent table caption from honoring justify-/align- when table is a grid item. r=dholbert
Differential Revision: https://phabricator.services.mozilla.com/D196705

Keywords: regression
Regressed by: 1350037
Flags: needinfo?(aethanyc)
Duplicate of this bug: 1881671

Calling this S2 for now since this seems to have been an unintended recently-shipped behavior-change, apparently affecting real web content.

TYLin's away for a few more days; I'll take a look in a debugger tomorrow and see if there's an obvious reason/fix.

Severity: -- → S2
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(dholbert)

Set release status flags based on info from the regressing bug 1350037

status-firefox123: --- → affected
status-firefox124: --- → affected
status-firefox125: --- → affected
status-firefox-esr115: --- → unaffected
Flags: needinfo?(dholbert)

(In reply to Mayank Bansal from comment #6)

Bisection points to:

Bug 1350037 Part 3 - Prevent table caption from honoring justify-/align- when table is a grid item. r=dholbert
Differential Revision: https://phabricator.services.mozilla.com/D196705

This is indeed the exact patch (part 3 in particular) where we started having the current behavior. (The patch wasn't as table-specific in its impact as it intended to be, I think.)

Specifically: if I restore the removed chunk of code here...
https://hg.mozilla.org/mozilla-central/rev/6e8085865f74#l1.12
...then we get the expected results here (smaller left/right/center-aligned purple boxes for all but the final line of https://bugzilla.mozilla.org/attachment.cgi?id=9388827 )

This patch essentially restores a chunk of code that we removed in this commit:
https://hg.mozilla.org/mozilla-central/rev/6e8085865f74

I think that commit intended to exempt this code from being run for
table-wrapper-frames that are grid items; but we need that code to get
shrink-wrapping (and alignment) for other sorts of frames that are grid items.

Assignee: nobody → dholbert
Status: NEW → ASSIGNED

Daniel, should this patch have landed in Nightly by now? I am on build ID 20240303091237 and can still see https://codepen.io/scheinercc/pen/MWxMWqP from bug 1881671.

No, the patch hasn't even been reviewed yet. Once the patch reaches mozilla-central, a comment will be left in the bug and it will be closed as RESOLVED FIXED. After that, it should be in the following nightly build.

Wontfix for 123 given that our planned dot release builds today.

status-firefox123: affected → wontfix

Bug 1350037 Part 3 [1] removed the code in ReflowInput that adds ShrinkWrap
flag self-aligned grid items, to prevent the table-caption from getting aligned
behavior, but it accidentally broke self-aligned behavior for other types of
frames that are grid items.

We actually already check if we need to add ShrinkWrap in grid item's final
reflow [2], but missing the same logic in MeasuringReflow(). This patch adds
that.

[1] https://hg.mozilla.org/mozilla-central/rev/6e8085865f74
[2] https://searchfox.org/mozilla-central/rev/202c48686136360a23b73a49b611a19e64f3e1b8/layout/generic/nsGridContainerFrame.cpp#7651,7657

Attachment #9388865 - Attachment description: Bug 1881495: Use shrink-wrap inline-sizing for grid items that are self-aligned (not stretched). r?TYLin → Bug 1881495 part 1: Use shrink-wrap inline-sizing for grid items that are self-aligned (not stretched). r?TYLin
Attachment #9388865 - Attachment description: Bug 1881495 part 1: Use shrink-wrap inline-sizing for grid items that are self-aligned (not stretched). r?TYLin → Bug 1881495 part 2: Add WPT for justify-self alignment of a percent-height grid item with an aspect-ratio. r?TYLin
Attachment #9389226 - Attachment description: Bug 1881495 part 2: Add two additional WPT tests with tables as grid-items. r?TYLin → Bug 1881495 part 3: Add two additional WPT tests, adding display:table to the aligned grid items. r?TYLin
Attachment #9389225 - Attachment description: Bug 1881495 - Add ShrinkWrap flag for grid items that are self-aligned (not stretched) in MeasuringReflow(). r?dholbert → Bug 1881495 part 1: Add ShrinkWrap flag for grid items that are self-aligned (not stretched) in MeasuringReflow(). r?dholbert

Try run, with the current patch stack: https://treeherder.mozilla.org/jobs?repo=try&revision=90669c22313d121810db2e6e8437ff030ef6918b
(reftests, crashtests, mochitests, web-platform-tests; excluding ASAN/TSAN),

Daniel, thank your for fixing this regression of mine while I'm away!

Flags: needinfo?(aethanyc)

Sure! I was hoping to finish off the fix last week; thanks for the suggested alternate approach & helping push it over the line! :)

This test's setup is a little more special (some text with a linewrap that's
forced by a negative margin-inline-end), so I'm not bothering to give it a
particularly descriptive name; I'm just following the convention of
[browser]-[bug]-[number] that we have for e.g. chrome-bug-001.html nearby (with
the actual bug number, in this case).

This test is based on the testcase from the duplicate bug, which was:
https://bugzilla.mozilla.org/show_bug.cgi?id=1881671

Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/14866c74663f part 1: Add ShrinkWrap flag for grid items that are self-aligned (not stretched) in MeasuringReflow(). r=dholbert https://hg.mozilla.org/integration/autoland/rev/09c6c8619b8f part 2: Add WPT for justify-self alignment of a percent-height grid item with an aspect-ratio. r=TYLin https://hg.mozilla.org/integration/autoland/rev/90da9f730286 part 3: Add two additional WPT tests, adding display:table to the aligned grid items. r=TYLin https://hg.mozilla.org/integration/autoland/rev/89e481cfc788 part 4: Add a reftest-style WPT to test the linewrapping issue from the duplicate bug. r=TYLin
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/44926 for changes under testing/web-platform/tests

The patch landed in nightly and beta is affected.
:dholbert, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox124 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(dholbert)

Bug 1350037 Part 3 [1] removed the code in ReflowInput that adds ShrinkWrap
flag self-aligned grid items, to prevent the table-caption from getting aligned
behavior, but it accidentally broke self-aligned behavior for other types of
frames that are grid items.

We actually already check if we need to add ShrinkWrap in grid item's final
reflow [2], but we were missing the same logic in MeasuringReflow(). This
patch adds that.

[1] https://hg.mozilla.org/mozilla-central/rev/6e8085865f74
[2] https://searchfox.org/mozilla-central/rev/202c48686136360a23b73a49b611a19e64f3e1b8/layout/generic/nsGridContainerFrame.cpp#7651,7657

Original Revision: https://phabricator.services.mozilla.com/D203514

Attachment #9389460 - Flags: approval-mozilla-beta?
Attachment #9389461 - Flags: approval-mozilla-beta?
Attachment #9389462 - Flags: approval-mozilla-beta?

This test's setup is a little more special (some text with a linewrap that's
forced by a negative margin-inline-end), so I'm not bothering to give it a
particularly descriptive name; I'm just following the convention of
[browser]-[bug]-[number] that we have for e.g. chrome-bug-001.html nearby (with
the actual bug number, in this case).

This test is based on the testcase from the duplicate bug, which was:
https://bugzilla.mozilla.org/show_bug.cgi?id=1881671

Original Revision: https://phabricator.services.mozilla.com/D203534

Attachment #9389463 - Flags: approval-mozilla-beta?

I verified that the attached testcases render as-expected (and match Chrome's rendering) with the latest Nightly, 125.0a1 (2024-03-05) (64-bit)

Marking VERIFIED for v125, and requesting beta uplift.

Status: RESOLVED → VERIFIED
status-firefox125: fixed → verified
Flags: needinfo?(dholbert)

Uplift Approval Request

  • String changes made/needed: None
  • Risk associated with taking this patch: Low
  • Is Android affected?: yes
  • User impact if declined: Layout issues (content overlapping and/or too large) on certain sites that use CSS Grid
  • Fix verified in Nightly: yes
  • Code covered by automated testing: yes
  • Explanation of risk level: Extremely targeted change, which makes the code more consistent and returns us to doing something that we were previously doing before this regressed.
  • Steps to reproduce for manual QE testing: Load https://bugzilla.mozilla.org/attachment.cgi?id=9388827 and compare rendering to Chrome. Only the bottom two purple boxes should overflow from their black container.
  • Needs manual QE test: yes
Flags: qe-verify+
Attachment #9389460 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9389461 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9389462 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9389463 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

:dholbert can you take a look at the code review failure on https://phabricator.services.mozilla.com/D203647?

(edit) must of been an weird error lando was throwing. I was not able to land this, however as you can see below, it did land.

Flags: needinfo?(dholbert)
Flags: needinfo?(dholbert)
QA Whiteboard: [qa-triaged]
Upstream PR merged by moz-wptsync-bot

Verified as fixed in our latest Beta 124.0b8.

QA Whiteboard: [qa-triaged]
status-firefox124: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: