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

Update CSS Language Syntax #115480

Closed
cssinate opened this issue Jan 31, 2021 · 12 comments
Closed

Update CSS Language Syntax #115480

cssinate opened this issue Jan 31, 2021 · 12 comments
Assignees
Labels
feature-request Request for new features or functionality grammar Syntax highlighting grammar insiders-released Patch has been released in VS Code Insiders verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@cssinate
Copy link

cssinate commented Jan 31, 2021

  • VSCode Version: 1.52.1
  • OS Version: Windows 10.0.18363

Steps to Reproduce:

  1. Look at
    "match": "(?xi) (?<![\\w:-])\n(?:\n # HTML\n a|abbr|acronym|address|applet|area|article|aside|audio|b|base|basefont|bdi|bdo|bgsound\n | big|blink|blockquote|body|br|button|canvas|caption|center|cite|code|col|colgroup|command\n | content|data|datalist|dd|del|details|dfn|dialog|dir|div|dl|dt|element|em|embed|fieldset\n | figcaption|figure|font|footer|form|frame|frameset|h[1-6]|head|header|hgroup|hr|html|i\n | iframe|image|img|input|ins|isindex|kbd|keygen|label|legend|li|link|listing|main|map|mark\n | marquee|math|menu|menuitem|meta|meter|multicol|nav|nextid|nobr|noembed|noframes|noscript\n | object|ol|optgroup|option|output|p|param|picture|plaintext|pre|progress|q|rb|rp|rt|rtc\n | ruby|s|samp|script|section|select|shadow|slot|small|source|spacer|span|strike|strong\n | style|sub|summary|sup|table|tbody|td|template|textarea|tfoot|th|thead|time|title|tr\n | track|tt|u|ul|var|video|wbr|xmp\n\n # SVG\n | altGlyph|altGlyphDef|altGlyphItem|animate|animateColor|animateMotion|animateTransform\n | circle|clipPath|color-profile|cursor|defs|desc|discard|ellipse|feBlend|feColorMatrix\n | feComponentTransfer|feComposite|feConvolveMatrix|feDiffuseLighting|feDisplacementMap\n | feDistantLight|feDropShadow|feFlood|feFuncA|feFuncB|feFuncG|feFuncR|feGaussianBlur\n | feImage|feMerge|feMergeNode|feMorphology|feOffset|fePointLight|feSpecularLighting\n | feSpotLight|feTile|feTurbulence|filter|font-face|font-face-format|font-face-name\n | font-face-src|font-face-uri|foreignObject|g|glyph|glyphRef|hatch|hatchpath|hkern\n | line|linearGradient|marker|mask|mesh|meshgradient|meshpatch|meshrow|metadata\n | missing-glyph|mpath|path|pattern|polygon|polyline|radialGradient|rect|set|solidcolor\n | stop|svg|switch|symbol|text|textPath|tref|tspan|use|view|vkern\n\n # MathML\n | annotation|annotation-xml|maction|maligngroup|malignmark|math|menclose|merror|mfenced\n | mfrac|mglyph|mi|mlabeledtr|mlongdiv|mmultiscripts|mn|mo|mover|mpadded|mphantom|mroot\n | mrow|ms|mscarries|mscarry|msgroup|msline|mspace|msqrt|msrow|mstack|mstyle|msub|msubsup\n | msup|mtable|mtd|mtext|mtr|munder|munderover|semantics\n)\n(?=[+~>\\s,.\\#|){:\\[]|/\\*|$)",
  2. Compare to https://github.com/atom/language-css/blob/21cfdb766587c351ac9873ecce0075b8c05799c5/grammars/css.cson#L2035

Does this issue occur when all extensions are disabled?: Yes

There was a fix some time ago for this issue: atom/language-sass#226 where in SCSS, property names that shared a name with a tag name were colored incorrectly. That fix has not been ported over to VSCode. As per

"This file has been converted from https://github.com/octref/language-css/blob/master/grammars/css.cson",
I am requesting an update to this file. Thank you!

@alexr00
Copy link
Member

alexr00 commented Feb 2, 2021

We get our css grammar from https://github.com/octref/language-css, so changes to https://github.com/atom/language-css would need to pulled into there.

@octref is https://github.com/octref/language-css being maintained?

@alexr00 alexr00 added grammar Syntax highlighting grammar upstream Issue identified as 'upstream' component related (exists outside of VS Code) labels Feb 2, 2021
@cssinate
Copy link
Author

cssinate commented Feb 2, 2021

Ah! I'm not crazy. I first started looking into the bad highlighting in SASS a long time ago and gave up when I couldn't figure it out, and only started digging into it again recently. When I first looked into it, it was derived directly from atom's version of the language:

e62554e#diff-41108266fb098e0fb5d4f0fbdab607f29ffc0143e1c4914c83fbdc28da29ab6fL7

I neglected to check that line to notice it had changed. I made a PR on the octoref version. Could we please leave this issue open until it's confirmed that it's either merged or won't be merged for some reason?

@cssinate
Copy link
Author

cssinate commented Feb 9, 2021

@alexr00 - how long do we wait for a response? Does this issue only move forward with octref's help? Their blog says they're not even at Microsoft anymore. I don't want to bother them. It seems strange that it got moved away from Atom's grammar as reference anyway, for exactly this reason: Atom isn't going to stop being maintained anytime soon.

@alexr00
Copy link
Member

alexr00 commented Feb 9, 2021

Many of our grammars are maintained by non-Microsoft developers, so this is very normal. However, since this isn't the only css grammar related issue that hasn't gotten a response, I'm happy to change grammars to the one that is maintained (atom/language-css).

Here are the bugs/missing features that we would be accepting by changing to atom/language-css. This may not be a complete list, but based on the diffs this is what I see:
#54515
Maybe #57407, would need to test.
Some of these properties may have been added in the other repo: octref/language-css@6d3a2d0
octref/language-css@ea1d7e3

@alexr00 alexr00 added this to the February 2021 milestone Feb 9, 2021
@alexr00 alexr00 added feature-request Request for new features or functionality and removed upstream Issue identified as 'upstream' component related (exists outside of VS Code) labels Feb 9, 2021
@cssinate
Copy link
Author

cssinate commented Feb 9, 2021

I'm just a bit confused by the second part; are there parts that are in octref's repo that aren't in Atom's? Do you need a hand comparing octo's to Atom's and/or making PRs to Atom's for discrepancies?

@alexr00
Copy link
Member

alexr00 commented Feb 9, 2021

are there parts that are in octref's repo that aren't in Atom's?

Yes.

Do you need a hand comparing octo's to Atom's and/or making PRs to Atom's for discrepancies?

I already hand compared (which is how I came up with the list of bugs/missing features). We try not to be in the business of writing grammars since there are many languages and we aren't experts in many of them. This is why we get our grammars from OSS. If a member of the community wanted to take a close look at my list here and make PRs for bugs and missing features in the atom/language-css grammar (or verify that the issue doesn't exist in the atom grammar), then I would have no reason not to use that grammar 😉.

That said, I'm inclined to adopt atom/language-css even if no one from the community wants to do that. A maintained grammar should be better on average.

@cssinate
Copy link
Author

cssinate commented Feb 9, 2021

Cool! I've just about had it up to here with language grammars for a while, but I don't mind circling back in a month or two to take a deeper look at those changes and making PRs against atom if necessary. Thanks, Alex 😃

@cssinate
Copy link
Author

As an update, I did some homework and made PRs for the things that made sense to.

@alexr00
Copy link
Member

alexr00 commented Feb 15, 2021

I've moved to March to give the folks over in atom/language-css time to look at @cssinate's PRs.

@alexr00
Copy link
Member

alexr00 commented Mar 11, 2021

It doesn't seem like those PRs are getting any attention. However, since the grammar is well maintained I will try switching over to it anyway. If there aren't any big concerns after switching, then I'll keep the change in.

@cssinate
Copy link
Author

cssinate commented Mar 11, 2021 via email

@alexr00 alexr00 added the verification-needed Verification of issue is requested label Mar 22, 2021
@alexr00
Copy link
Member

alexr00 commented Mar 22, 2021

This can be verified by seeing that the integration tests pass (specifically the colorizer integration tests).

@github-actions github-actions bot locked and limited conversation to collaborators Apr 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality grammar Syntax highlighting grammar insiders-released Patch has been released in VS Code Insiders verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

5 participants
@isidorn @aeschli @cssinate @alexr00 and others