Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Add support for literally every modern CSS feature #99

Merged
merged 74 commits into from
Jan 11, 2017
Merged

Add support for literally every modern CSS feature #99

merged 74 commits into from
Jan 11, 2017

Conversation

Alhadis
Copy link
Contributor

@Alhadis Alhadis commented Oct 22, 2016

There're too many things happening with this PR, so I won't bother summarising. The commit log holds the full details.

Most notable improvements:

Syntax preview

Fixes: #24, #42
Supersedes: #50, #65

Alhadis added 30 commits October 18, 2016 02:44
It's extremely unlikely this is only intended to match one space only.
CSS doesn't use dollar signs to introduce variables, and it never did.
* Now matched only at the start of a document
* Unquoted strings are marked invalid
* Scope-name choices improved
* Word boundaries fixed to avoid matching @charset"UTF-8";

See: https://developer.mozilla.org/en-US/docs/Web/CSS/@charset
Note that @charset statements don't allow injected comments.
These rules have a very rigid syntax; the spec mandates that exactly one
space be used to separate the rule from the quoted character set. Things
like case-insensitivity, comments and extraneous whitespace invalidate a
@charset.

Note that while the spec states that double-quotes be used to enclose an
encoding type, agents permit single-quotes for certain identifiers. This
behaviour isn't consistent between browsers:

    @charset 'iso-8859-15';    # Valid in Firefox and Chrome
    @charset 'US-ASCII';       # Invalid in Chrome; okay in Firefox

References:
  * https://www.w3.org/TR/CSS22/syndata.html#x57
  * https://developer.mozilla.org/en-US/docs/Web/CSS/@charset
The CSS 2.2 spec states that an underscore may also introduce a vendored
extension, though this is rare in practice.

Several historical extensions have also been added, as documented in the
spec - https://www.w3.org/TR/CSS22/syndata.html#vendor-keyword-history
Support for the following missing keywords has also been added:

    * currentColor
    * rebeccapurple
    * transparent
Functional values are composed of the function's name affixed to an open
bracket. Whitespace is permitted in the arguments list, but not before:

    rgb(0,0,0);   # Valid
    rgb (0,0,0);  # Invalid

References:
  * https://drafts.csswg.org/css-values-3/#functional-notations
  * https://www.w3.org/TR/css-syntax-3/#function-token-diagram
@Alhadis
Copy link
Contributor Author

Alhadis commented Jan 8, 2017

BTW, I didn't notice the missing question mark, and thought you were referring to the removal of ;|(?=[@{]). So in other words, the middle part was actually doing nothing all along and should have been dropped ages ago.

@winstliu
Copy link
Contributor

winstliu commented Jan 8, 2017

Glad we're on the same page, finally. The missing question mark was what I was referring to the whole time; probably could have made that a bit clearer.

I'll give a few more days for esdoppio and in case anyone else wants to review. I already said that I'm done, so anything else I spot will come in the form of post-merge revisions.

Copy link
Contributor

@winstliu winstliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

(?:[-a-zA-Z_] | [^\\x00-\\x7F]) # First letter
(?:[-a-zA-Z0-9_] | [^\\x00-\\x7F] # Remainder of identifier
|\\\\(?:[0-9a-fA-F]{1,6}|.)
)*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you show me which "pattern for matching numbers is tested first?" And since you have decided to support escape sequences, care to also cover situations like "foo\ae bar"?

'match': '''(?xi)
(?<![\\w-]) (from|to) (?![\\w-]) # Keywords for 0% | 100%
|
([-+]?(?:\\d+(?:\\.\\d+)?|\\.\\d+)%) # Percentile value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Care to make this stricter to match only percentages from 0% to 100%, or tokenize percentages smaller or higher as invalid? Up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah.

'name': 'keyword.control.at-rule.document.css'
'1':
'name': 'punctuation.definition.keyword.css'
'end': '(?=\\s*{)'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a ;?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3f7fb1e and 807168b, respectively.

}
# Shapes
{
'begin': '(?i)(?<![\\w-])(circle|ellipse|inset|polygon|rect)(\\()'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rect() is not a Shapes function, and it is used along with the deprecated feature clip. And there would probably be named a rectangle() rather than rect() in the future. Consider removing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. I'm well-aware that rect() is non-standard, but it (along with clip) is still used in a lot of boilerplate code in the wild. Hence the reason for its inclusion.

'patterns': [
{
'name': 'variable.argument.css'
'match': '--[\\w-]+'
Copy link
Contributor

@hediyi hediyi Jan 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about making it "any valid identifier that starts with two dashes?"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 687567b.

'match': '(?<=["\\s])[a-zA-Z*]+(-[a-zA-Z0-9*]*+)*+(?=["\\s])'
'name': 'support.constant.language-range.css'
}
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't tokenize quoted language code containing escapes as intended.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you were the one who added this. I left it alone because I wasn't sure what it was you were trying to do, and the possessive capturing suggested it needed to be very specific. Can you explain exactly what it was your code was trying to do?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know what I was thinking lol, it should be fine to just replace those possessive quantifiers. Fwiw, I think we should just leave out the escapes part here (who would really use that in language range? -> code not needed). Also, if you decide to support escapes, you'll also need to take care of the trailing whitespace. It's just not worth it. Let's waste our energy on something more important.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now you're speakin' my language! :D And yeah, you're right: every language-code is just pure ASCII, I can't imagine anybody in their right mind would be using escape-codes for es-CO or whatever.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'll get to this in a moment, I'm in the middle of attempting a "delicate" fix for Nuclide and File-Icons v2. And by "delicate", I mean this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol. No problem.

Copy link
Contributor Author

@Alhadis Alhadis Jan 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, finally. Sorry that took so long, let me tend to this...

'name': 'keyword.other.unit.percentage.css'
'2':
'name': 'keyword.other.unit.${2:/downcase}.css'
'match': '''(?xi) (?<!\\w|-)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep it consistent: (?<![\\w-]).


(?: # Scientific notation
(?<=[0-9]) # Exponent must follow a digit
[Ee] # Exponent indicator
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're in case-insensitive mode, make it just e if not too much trouble.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... how the f-

No idea how I let that one slip past...

'2':
'name': 'invalid.illegal.colon.css'
'match': '''(?xi)
(:)(:*)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:* is for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To extend angry red highlighting across every repeated colon. Unless for some reason only the second colon of a::::::::first-child should be marked as invalid...

'include': '#font-features'
}
{
# Custom properties
'match': '--[\\w-]+'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to update this?

| first|left|right # @page
| host # shadow DOM
)(?![\\w-])
-?+ # possessive to avoid matching '-' followed by [0-9]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this possessive quantifier still necessary?

Copy link
Contributor Author

@Alhadis Alhadis Jan 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. Killing it.

)(?![\\w-])
-?+ # possessive to avoid matching '-' followed by [0-9]
(?![0-9])
(?:[-a-zA-Z_0-9]|[^\\x00-\\x7F]|\\\\(?:[0-9a-fA-F]{1,6}|.))+
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, if we are to support escapes, we should also take care of the whitespace separating the seqences and normal characters—this is not good enough. Still, we can't expect to make things perfect in one PR. Up to you if you wanna take it into subsequent PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I think this is something that can be left for a follow-up PR, should it ever be needed. I'm itching to see this thing merged so I can get it used across GitHub.

@Alhadis
Copy link
Contributor Author

Alhadis commented Jan 10, 2017

@50Wliu Sorry to take our progress backwards, but I'll need you to review b6081eb and tell me if there's anything that needs improvement.

Rather than use possessive capturing to avoid matching invalid patterns, I decided to flag them as errors instead. Not only does this simplify implementation, but it's also more informative for users.

@@ -1080,7 +1080,7 @@
'include': '#escapes'
}
{
'match': "(?<=['\\s])[a-zA-Z*]+(-[a-zA-Z0-9*]*+)*+(?=['\\s])"
'match': "(?<=['\\s])[a-zA-Z*]+(-[a-zA-Z0-9*]*+)*(?=['\\s])"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you missed the inner possessive match with this one compared to L1063.

@winstliu
Copy link
Contributor

I like it. Only one comment.

@Alhadis
Copy link
Contributor Author

Alhadis commented Jan 10, 2017

Ah, good catch. Fixed!

@winstliu winstliu merged commit 2f8e02c into atom:master Jan 11, 2017
@winstliu
Copy link
Contributor

winstliu commented Jan 11, 2017

Wheeeeeee
Only took 250 comments and 3 1/2 months. Thanks for sticking with this.

@Alhadis
Copy link
Contributor Author

Alhadis commented Jan 11, 2017

This is the happiest day of my life.

Will hook this into the Less and Sass grammars later-ish. Will probably only limit those PRs to keyword-embedding only, because I'm really not that confident in my knowledge of precompilers (certainly not compared with CSS).

@Alhadis Alhadis deleted the fruit-basket branch January 11, 2017 03:41
@pchaigno
Copy link
Contributor

Wow! Congrats @Alhadis! 🎉

@Alhadis
Copy link
Contributor Author

Alhadis commented Jan 11, 2017

Merci!

(Haven't even started on the Shell and Make grammars yet... hahhaha)

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

Successfully merging this pull request may close these issues.

5 participants