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

Fix tag names interfering with property names #243

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

winstliu
Copy link
Contributor

@winstliu winstliu commented Nov 8, 2017

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

This is another attempt to fix tag names incorrectly always receiving priority over property names. It changes property name matching to look for a colon on the same line and then makes it higher priority than selectors. While having a colon on a separate line is not forbidden in CSS, I think this is the closest that we can get to a comprehensive fix.

Alternate Designs

There's quite a few alternatives, such as changing the selectors begin pattern instead of properties'. I like this alternative the best because if first-mate supported multiline regex patterns then this pattern would be a complete fix.

Benefits

Tag names will no longer always override property names.

Possible Drawbacks

Unforeseen bugs such as #237.

Applicable Issues

Fixes #226

/cc @dsifford

@dsifford
Copy link
Contributor

dsifford commented Nov 8, 2017

@50Wliu thanks for stepping up and knocking this out. I had some other work pile up this week and hadn't had an opportunity to go over this again.

Let me know if there's something you'd like me to do here.

@winstliu
Copy link
Contributor Author

winstliu commented Nov 8, 2017

Well. This bug is...quite tricky to get right. I tried doing the same thing for language-less but thankfully that had a test that revealed other incorrect behavior:

.foo:hover {
  span:last-of-type { // span will get incorrectly tokenized as property-name because it sees the pseudo-class colon!
    font-weight: bold;
  }
}

@winstliu
Copy link
Contributor Author

winstliu commented Nov 8, 2017

@Alhadis I would love if you had any suggestions for how to fix this.

@Alhadis
Copy link
Contributor

Alhadis commented Nov 8, 2017

Make sure you're embedding the source.css rulesets in a specific order which prioritises the source.css#pseudo-elements and source.css#pseudo-classes' blocks. If they're being matched after the property-name match, well, that could be why...

That's more-or-less guessing on my part. I remember hitting this issue with language-less, although my memory has already faded of how I surmounted it.

@dsifford
Copy link
Contributor

dsifford commented Nov 8, 2017

@50Wliu Circling back to comments I made in (I think) one of my PHP pull requests....

What would be the performance loss of making the selectors specific enough so that the order doesn't matter? Seems like that strategy would lead to a much more sane experience for language evolution.

There will obviously be situations where order will matter, but for the most part increasing scope specificity would allow maintainers to offload that concern when updating the grammar.

@winstliu
Copy link
Contributor Author

winstliu commented Nov 9, 2017

property-name-pseudo-class
Nope. Still gets tokenized as a property name before the pseudo-class.

@dsifford If you can find a way to tighten up the patterns to prevent this, I'll gladly go with that solution. I just think atom/language-css#127 is too drastic in that regard.

@dsifford
Copy link
Contributor

dsifford commented Nov 9, 2017

@50Wliu Here is the comment I was referring to: atom/language-php#285 (comment)

IMHO, the best way to create resilient and future-proof grammars is to follow two rules:

  1. Be as specific as possible
  2. In-line nothing; i.e, create small named items and them use those items to compose larger structures.

CSS currently fails both of these, for example.

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.

Color for 'cursor' for sass (SCSS) is wrong
3 participants