Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Select Dropdown: CSS parsing error breaks the editor #266

Closed
WunderBart opened this issue Feb 22, 2021 · 7 comments · Fixed by #267
Closed

Select Dropdown: CSS parsing error breaks the editor #266

WunderBart opened this issue Feb 22, 2021 · 7 comments · Fixed by #267

Comments

@WunderBart
Copy link

CSS does not support inline comments, which makes this line break the editor and throw the following error:

{
  "name": "CssSyntaxError",
  "reason": "Unknown word",
  "source": "\n\tz-index:1000000;;\n\topacity: 1;\n\toutline: none;\n\ttransition: 40ms opacity linear;\n\ttransition-delay: 20ms; // Allows for the popover to reposition without being seen.\n",
  "line": 6,
  "column": 26,
  "message": "<css input>:6:26: Unknown word",
  "input": {
    "line": 6,
    "column": 26,
    "source": "\n\tz-index:1000000;;\n\topacity: 1;\n\toutline: none;\n\ttransition: 40ms opacity linear;\n\ttransition-delay: 20ms; // Allows for the popover to reposition without being seen.\n"
  }
}

When the comment is removed, the error is gone and the page loads normally. Also, notice how this line is appending an extra semicolon ("source": "\n\tz-index:1000000;; (...)). It doesn't seem to be breaking anything but probably worth addressing as well.

cc @ItsJonQ @sarayourfriend

@ItsJonQ
Copy link
Owner

ItsJonQ commented Feb 22, 2021

Thanks for reporting this @WunderBart !! Will push an update immediately

@ockham
Copy link
Collaborator

ockham commented Feb 22, 2021

Thanks for flagging @WunderBart, and for fixing @ItsJonQ ❤️

I've filed #268 to suggest adding stylelint to prevent this sort of issue from re-appearing 😄

It'd be great if we could also catch an error like this at Gutenberg level. Do we know how come that none of Gutenberg's tests apparently flagged this? Reading internal Slack convo, it seems that this broke the editor, so I thought we'd see that reflected in some e2e tests? 🤔

@ockham
Copy link
Collaborator

ockham commented Feb 22, 2021

Thanks for flagging @WunderBart, and for fixing @ItsJonQ

I've filed #268 to suggest adding stylelint to prevent this sort of issue from re-appearing

It'd be great if we could also catch an error like this at Gutenberg level. Do we know how come that none of Gutenberg's tests apparently flagged this? Reading internal Slack convo, it seems that this broke the editor, so I thought we'd see that reflected in some e2e tests?

Trying to track this down:

It seems like the issue was introduced with 52417c7#diff-d08c6b1df7e82e560c4dacc292c91e7b0d029c08c97c09141bb96a3fdf201d99R14. According to GH (see commit header of that page), that change has been part of G2 since v0.0.149.

OTOH, for GB's 10.0.0 release, the G2 versions in package.json were set to 0.0.150.

What's still puzzling me is that GB tests didn't fail when G2 versions were first bumped to >= 0.0.149 in WordPress/gutenberg#28707 (or for any PR filed since) 🤔

cc/ @youknowriad @gziolo @sarayourfriend

@ockham
Copy link
Collaborator

ockham commented Feb 22, 2021

One thing we can try to mitigate these things is running our tests on a GB release (e.g. when tagging). (Assuming that our tests actually will fail.)

@ockham
Copy link
Collaborator

ockham commented Feb 23, 2021

Ah, I hadn't realized the bug only occurred if the language was an RTL one 😅

I've just filed WordPress/gutenberg#29248 to capture that (and because the bug is still present in GB 😓 ).

As a follow-up, I'll

  • file a GB issue to request an e2e test for RTL languages.
  • file an issue or PR to run our tests against release/ branches.

@ockham
Copy link
Collaborator

ockham commented Feb 23, 2021

Ah, I hadn't realized the bug only occurred if the language was an RTL one

I've just filed WordPress/gutenberg#29248 to capture that (and because the bug is still present in GB ).

As a follow-up, I'll

  • file a GB issue to request an e2e test for RTL languages.

WordPress/gutenberg#29250

  • file an issue or PR to run our tests against release/ branches.

WordPress/gutenberg#29251

@ItsJonQ ItsJonQ closed this as completed Feb 23, 2021
@fullofcaffeine
Copy link

fullofcaffeine commented Feb 23, 2021

It's worth documenting here that there's a suspicion that the culprit might have been related to https://github.com/MohammadYounes/rtlcss. Which is used only when an RTL language is active (i.e., Arabic or Hebrew).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants