Closed Bug 1599206 Opened 11 months ago Closed 10 months ago

[css-grid] Exclude implicit tracks from grid-template-rows/columns resolved values

Categories

(Core :: Layout: Grid, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla74
Tracking Status
firefox74 --- fixed

People

(Reporter: Oriol, Assigned: emilio)

References

Details

Attachments

(1 file, 2 obsolete files)

Currently, the resolved values of grid-template-rows/columns include both implicit and explicit tracks.
However, specified values can only set explicit tracks.
Therefore, resolved values don't round-trip:

gridContainer.style.gridAutoRows = "1px";
gridContainer.style.gridTemplateRows = "2px";
var cs = getComputedStyle(gridContainer);
cs.gridTemplateRows; // "2px"
gridItem.style.gridRow = "auto / 1";
gridContainer.style.gridTemplateRows = cs.gridTemplateRows; // "1px 2px"
gridContainer.style.gridTemplateRows = cs.gridTemplateRows; // "1px 1px 2px"
gridContainer.style.gridTemplateRows = cs.gridTemplateRows; // "1px 1px 1px 2px"
gridContainer.style.gridTemplateRows = cs.gridTemplateRows; // "1px 1px 1px 1px 2px"

https://github.com/w3c/csswg-drafts/issues/4475#issuecomment-553525609 resolved to try to exclude implicit tracks and only include explicit ones.

Chromium issue: https://crbug.com/1024927

We should probably coordinate to avoid changing the tests at the same time... Do you plan to work on this soon?

I have written this patch: https://chromium-review.googlesource.com/c/chromium/src/+/1897931
It updates the WPT tests to the new behaviour. I don't know if Mozilla has internal tests other than WPT that will also need to be updated.
Since this may have compatibility problems, my plan is to send an intent to implement and ship for Chromium, and if it gets approved land the patch at the beginning of a release cycle (branch point for version 80 will be Dec 5).

Can I consider your comment as 'public support' from Firefox?

Do this only on nightly for now since we're about to enter the soft freeze.

(No test updates yet, as try is still running, and I need to figure out how to
import Oriol's changes into WPT, but I wanted to ensure that you were fine with
this)

Assignee: nobody → emilio

(In reply to Oriol Brufau [:Oriol] from comment #3)

Can I consider your comment as 'public support' from Firefox?

I mean, I think getComputedStyle() should round-trip, yeah. I think everybody in the CSSWG call agreed with that?

I don't think I speak for Mozilla as a whole, though. I'd like to check with Mats, but I think unless he opposes we should try to do this.

Well, the current property value already does round-trip in the sense that the value (with implicit tracks included) is valid. It's just not the same value. Excluding implicit tracks makes it "more the same" but far from being layout-equivalent obviously.
I guess excluding implicit tracks is a bit purer in the sense that the resolved sizes then always originates from a track size in the property's own specified value. Then again, the original goal of these resolved values was to give authors something to determine the number of tracks and their sizes in the final grid (if I understood it correctly). Perhaps that's proven to not be a very useful feature? Anyway, I don't feel strongly either way, so excluding implicit tracks is fine with me.

(And yes, we do have non-WPT tests that depend on these resolved values, IIRC. Not sure if they have implicit tracks though.)

BTW Oriol, I was waiting on WPT to get updated with your changes to land this, do you plan to submit them soon? Could you submit them to wpt directly otherwise, or should I just update the mozilla-central version of WPT or such?

Flags: needinfo?(oriol-bugzilla)

Oh sorry I forgot to send the intent to ship to Chromium. Let's see if I can send it today, and it gets approved soon. But I don't mind if you want to go ahead in Firefox.

Flags: needinfo?(oriol-bugzilla)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:emilio, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(emilio)

FYI, I sent the intent to ship to Chromium but I still need the 3rd LGTM

Flags: needinfo?(emilio)

I have landed my patch for Chromium and the WPT changes have been merged

Depends on: 1607428

Thanks Oriol!

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8bde04eafc7c
Don't serialize implicit tracks for grid-template properties, as they make the computed style not round-trip. r=mats
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ce55ea50cddd
Update a couple more test expectations.
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9888d5a5cb13
Update two other tests that are now passing.

Backed out 3 changesets (bug 1599206) for multiple wpt failures grid-template-columns

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=superseded%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&revision=8bde04eafc7c209a2919bfe08d52b7dbeb947219&selectedJob=284389286

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=284408194&repo=autoland&lineNumber=6732

Backout: https://hg.mozilla.org/integration/autoland/rev/5a4423df948f8f244a8195079598510801f9f74d

Failures on:
/css/css-grid/grid-layout-properties.html | grid-template-columns.initial - assert_equals: initial value of grid-template-columns should be 150px expected "150px" but got "none"
testing/web-platform/meta/css/css-grid/parsing/grid-template-columns-computed-withcontent.html.ini
testing/web-platform/meta/css/css-grid/parsing/grid-template-rows-computed-withcontent.html.ini
testing/web-platform/meta/css/css-grid/grid-definition/grid-template-columns-rows-resolved-values-001.html.ini
testing/web-platform/meta/css/css-grid/grid-definition/grid-support-repeat-001.html.ini
testing/web-platform/meta/css/css-grid/grid-definition/grid-support-named-grid-lines-001.html.ini
testing/web-platform/meta/css/css-grid/grid-definition/grid-support-grid-template-columns-rows-001.html.ini

Flags: needinfo?(emilio)

grid-layout-properties.html is marked as failing in Chromium so I didn't notice it needs to be updated.

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7de69291913b
Don't serialize implicit tracks for grid-template properties, as they make the computed style not round-trip. r=mats
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/21147 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74
Regressions: 1609042
Upstream PR merged by moz-wptsync-bot

Actually, in grid-layout-properties.html,

'<track-size>.<track-breadth>.<percentage>': ['100%', '150px 50px 50px'],

should have become

'<track-size>.<track-breadth>.<percentage>': ['100%', '150px'],

instead of

'<track-size>.<track-breadth>.<percentage>': ['100%', '50px'],

Firefox gets 50px because of bug 1609495.

Attachment #9120031 - Attachment is obsolete: true
Attachment #9120037 - Attachment is obsolete: true

Hi 👋, just wanted to follow up here as well since Chromium recently reverted this change since it was breaking site-authoring tools like webflow.com, educational tools like gridcritters.com, and possibly other tools that relied on the previous behavior.

To summarize, the Chromium team is postponing the fix until an API for querying grid information is available. We're hoping that all the browser vendors do the same for now 🙏.

More info:

Quick followup to 👆🏼 — Safari is also going to revert as well https://bugs.webkit.org/show_bug.cgi?id=204588#c10

You need to log in before you can comment on or make changes to this bug.