ItsJonQ / g2

✨ An experimental reimagining of WordPress components
http://g2-components.com/
MIT License
105 stars 12 forks source link

Select Dropdown: CSS parsing error breaks the editor #266

Closed WunderBart closed 3 years ago

WunderBart commented 3 years ago

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 commented 3 years ago

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

ockham commented 3 years ago

Thanks for flagging @WunderBart, and for fixing @ItsJonQ :heart:

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

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? :thinking:

ockham commented 3 years ago

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 https://github.com/ItsJonQ/g2/commit/52417c778c7684ab9986c0dbb8af6fb1ab5b9c61#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 https://github.com/WordPress/gutenberg/pull/28707 (or for any PR filed since) :thinking:

cc/ @youknowriad @gziolo @sarayourfriend

ockham commented 3 years ago

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 commented 3 years ago

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

I've just filed https://github.com/WordPress/gutenberg/issues/29248 to capture that (and because the bug is still present in GB :sweat: ).

As a follow-up, I'll

ockham commented 3 years ago

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.

https://github.com/WordPress/gutenberg/issues/29250

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

https://github.com/WordPress/gutenberg/issues/29251

fullofcaffeine commented 3 years ago

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).