fix: grid template is parsed incorrectly by chrome #1396
Conversation
🦋 Changeset detectedLatest commit: 7bede51 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Came across this PR and it's one I've seen, too and wanted to add some additional detail to help illustrate the issue. I put together a gist and debug video w/ a sample recording that demonstrates the issue. For reference, this is what the layout should look like. 
|
|
If you add a changeset file (see changeset-bot comment on this page) it should re-trigger the tests. |
Ah, sorry. How do I create with permissions? |
|
In addition to the note above about the |
https://issues.chromium.org/issues/40227336 We omitted from using |
There's a ' Allow edits and access to secrets by maintainers' checkbox in the rhs column |
|
You might have missed my review query; |
I did... in fact, I can't see it anywhere 🙈 This was quite a while ago so my brain has jettisoned all context 🤣 We've had a single user mention it, and they're not chasing us. So, I'm just as happy to get validation that the approach/files changed are about right (even if the actual implementation isn't desirable) and close the PR |
| documentNode, | ||
| documentTypeNode, | ||
| serializedNode, | ||
| serializedNodeWithId, |
There was a problem hiding this comment.
Thanks for your contribution!
Small note: reordering here will make it harder to merge in other in-progress PRs that might make changes in the same lines and makes it harder to review; looks like there are no changes?
|
|
||
| it('fixes incorrectly parsed grid template rules', () => { | ||
| const cssText = | ||
| '#wrapper { display: grid; grid-template: "header header" max-content / repeat(2, 1fr); margin: 0px auto; }'; |
There was a problem hiding this comment.
This doesn't appear to be valid grid-template syntax
"Note: The repeat() function isn't allowed in these track listings, as the tracks are intended to visually line up one-to-one with the rows/columns in the "ASCII art"."
https://developer.mozilla.org/en-US/docs/Web/CSS/grid-template#values
| 'grid-template-areas:', | ||
| ); | ||
| if ( | ||
| isCSSStyleRule(rule) && |
There was a problem hiding this comment.
This isCSSStyleRule check has already been performed outside of the function call
| @@ -125,7 +165,11 @@ export function stringifyRule(rule: CSSRule): string { | |||
| return fixSafariColons(rule.cssText); | |||
| } | |||
|
|
|||
| return importStringified || rule.cssText; | |||
| if (isCSSStyleRule(rule)) { | |||
There was a problem hiding this comment.
This check will be performed twice at this point; we create one } else if (isCSSStyleRule(rule)) { section and have this grid fix and the safari colons fix within it
|
Also, do you recall whether the problem was manifesting in a |
|
I don't remember at all and we've not had further reports... since I captured a fiddle to demo the bug at reporting time I'll defer to #1458 and check if that fixes this We can re-open this if need be then but let's not add code for an very rare case if we can avoid it :) |
|
OK there may be a new approach possible; see Basically we stringify A job for another day, but if you think you can salvage the tests out of this one (I mentioned some seemingly incorrect syntax) then we could commit that albeit commented out with a TODO until we can fix this. |
|
I've opened a new PR that builds on the work from this one. We had been using the code from the closed PR, but hit an issue w/ the regex that was in the original work so I've updated to address that in the new one |
fixes: #1395
Chrome is not correctly providing
cssTextwhen the style includes agrid-template-arearule. This PR attempts to detect and correct this...(it's my first PR against the repo, so have opened early to get feedback and guidance :))