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

fix css syntax range definition notation #4629

Merged

Conversation

idoros
Copy link
Contributor

@idoros idoros commented Sep 6, 2021

Formal syntax for range restrictions and range definition notation is broken. Currently grouping brackets are matching the range notation within data types and the regex that matches the data types doesn't match the bracketed range notation.

This PR prevents bracket groups from matching within data type angle brackets and adds the bracketed range notation to the data type regex.

Examples where syntax currently breaks:

https://developer.mozilla.org/en-US/docs/Web/CSS/overflow-clip-margin#formal_syntax
https://drafts.csswg.org/css-overflow/#overflow-clip-margin
<visual-box> || [0,∞]>
should be:
<visual-box> || <length [0,∞]>

https://developer.mozilla.org/en-US/docs/Web/CSS/font-weight#formal_syntax
https://drafts.csswg.org/css-fonts/#font-weight-prop

<font-weight-absolute> = normal | bold | [1,1000]>
should be:
<font-weight-absolute> = [normal | bold | <number [1,1000]>]

https://developer.mozilla.org/en-US/docs/Web/CSS/animation-timing-function#formal_syntax
https://drafts.csswg.org/css-easing-1/#typedef-cubic-bezier-easing-function

<cubic-bezier-timing-function> = ease | ease-in | ease-out | ease-in-out | cubic-bezier([0,1]>, <number>, [0,1]>, <number>)
should be
<cubic-bezier-easing-function> = ease | ease-in | ease-out | ease-in-out | cubic-bezier(<number [0,1]>, <number>, <number [0,1]>, <number>)

- don't match group bracket withing angle brackets
- match range notation as part of data type
@escattone escattone requested review from wbamberg and removed request for escattone September 8, 2021 16:01
@escattone
Copy link
Contributor

I'm adding @wbamberg as the reviewer, since he knows this macro best.

@wbamberg
Copy link
Collaborator

wbamberg commented Sep 8, 2021

Thanks @idoros for this PR!

@Elchi3 this is a great example of why we should be using css-tree instead of the current regex-driven approach.

@idoros
Copy link
Contributor Author

idoros commented Sep 9, 2021

@wbamberg - I was going to offer to replace the regex with a walk over some parsed AST (although I think it would be good to take the fix before replacing the implementation).

Also, I didn't find any tests for this parsing, so non were added.

@wbamberg
Copy link
Collaborator

@wbamberg - I was going to offer to replace the regex with a walk over some parsed AST (although I think it would be good to take the fix before replacing the implementation).

Thank you! Actually I was just looking at it this week and filed #4656. But I'd be really happy to talk about it.

@wbamberg
Copy link
Collaborator

As for a review: I don't have the permissions to formally approve this and regexes make my head hurt, but I have tested this out on a few pages and it looks good, so r+ . If I could merge it, I would :).

@escattone
Copy link
Contributor

As for a review: I don't have the permissions to formally approve this and regexes make my head hurt, but I have tested this out on a few pages and it looks good, so r+ . If I could merge it, I would :).

@wbamberg Just FYI, I gave you write privs last week, so you should be able to approve. I'll take this as approval and go ahead and merge this.

@escattone escattone merged commit c568d0a into mdn:main Sep 13, 2021
@wbamberg
Copy link
Collaborator

Thanks Ryan!

@idoros idoros deleted the ido/fix-css-syntax-range-definition-notation branch September 14, 2021 07:44
escattone pushed a commit that referenced this pull request Nov 9, 2021
- don't match group bracket withing angle brackets
- match range notation as part of data type
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 this pull request may close these issues.

3 participants