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

adjust tag-names match to fix downstream issue in language-less #127

Merged
merged 1 commit into from
Jan 25, 2021
Merged

adjust tag-names match to fix downstream issue in language-less #127

merged 1 commit into from
Jan 25, 2021

Conversation

dsifford
Copy link
Contributor

@dsifford dsifford commented Oct 3, 2017

This PR addresses a specificity issue in the tag-names matcher which causes a bug in language-less.

Currently, the tag-names matcher ends with a postive lookahead of simply a single colon (among other things) after any of the valid tag names is found. The reason this is a problem is because some tag names are also property names (e.g. cursor, font, content, etc).

To fix this, all that needed to happen is a slight adjustment to the regex to increase the specificity to match only colons followed by a non-space.

Drawbacks

There really is no other way I can think of other than having the selector match a colon followed by a non-space because of all the possibilities with pseudo selectors, child selectors, etc. Because of that, there will still be a mistake in some situations where the user forgets to add a space between property keys and values that match tag names. IMHO, that's not the end of the world though.

Relevant:

Fixes atom/language-less#79

@50Wliu
Copy link
Contributor

50Wliu commented Oct 4, 2017

Hmm, is there no similar fix for language-less like the one you did for language-sass? I'd like to preserve support for cursor:auto if possible.

@dsifford
Copy link
Contributor Author

dsifford commented Oct 4, 2017

@50Wliu Without doing a full rewrite of language-less, I don't think so. The structure of less and sass grammars is wildly different unfortunately.

@dsifford
Copy link
Contributor Author

dsifford commented Oct 4, 2017

That said, I think this fix would have also likely fixed the issue in language-sass without the need to change anything.

@dsifford
Copy link
Contributor Author

dsifford commented Oct 4, 2017

I'd like to preserve support for cursor:auto if possible.

What is wrong with cursor: auto? Is there any difference?

@50Wliu
Copy link
Contributor

50Wliu commented Nov 9, 2017

Is there any difference?

No difference, however I'd like to support both methods if possible.

@DeeDeeG
Copy link

DeeDeeG commented Feb 20, 2021

This PR broke an autocomplete-css test. Might anyone here know the fix?

CSS property name and value autocompletions
  SASS files
    pseudo selectors
      it CSS property name and value autocompletions SASS files pseudo selectors autocompletes without a prefix
        TypeError: Cannot read property 'length' of null
          at jasmine.Spec.<anonymous> (/Users/runner/work/1/s/node_modules/autocomplete-css/spec/provider-spec.coffee:903:28)
          at jasmine.Spec.args.<computed> (/Users/runner/work/1/s/spec/jasmine-test-runner.coffee:89:27)

Here's the failing line of the test in autocomplete-css: https://github.com/atom/autocomplete-css/blob/v0.17.5/spec/provider-spec.coffee#L903

The issue is that the test's getCompletions() function returns null now, during that test. It's trying to find a completion near the cursor, "without a prefix", whatever that means. (This apparently only happens in this one test, only with SASS files. All other types of CSS, whether it be CSS or SCSS or LESS are all still passing.) Sorry I can't be more helpful.

(Atom's CI fails with the latest language-css 0.44.5, due to this PR. I did CI runs of Atom, one with the latest language-css 0.44.5, and one that was the same but with this PR reverted. Reverting this PR made the tests pass again. CI link with 0.44.5 (failing), CI link (reverted this PR, passing)).

This test failure is currently blocking the language-css upgrade in Atom: atom/atom#21963 (comment)

@DeeDeeG
Copy link

DeeDeeG commented Jul 20, 2021

This pull request actually caused autocompletions to stop happening for element pseudo-selectors in .sass files.

Steps to reproduce:

For example, when typing div: in a .sass file in Atom...

Expected:

Pseudo-selectors such as div::before should be automatically suggested.

Actual:

Instead, there are no autocompletion suggestions at all until you type another character, for example if you start typing another colon, it works: div:: it suggests things like div::after. Or if you type div:a it suggests things like div:active. (But it should have started suggesting auto-completions with only one colon i.e. div:.)

(Thus, the failing test in autocomplete-css is a legitimate test failure, not an incorrectly coded test.)

Note: pseudo-selectors work as intended in .css and .scss files. So... I assume there must be something uniquely wrong (or simply incompatible with this PR) in the SASS grammar... Or this PR is wrong and should be reverted? I wish I understood the language grammar files or I might try to fix this issue myself.

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

Successfully merging this pull request may close these issues.

Cursor property looks different than other properties
4 participants