Skip to content

fix(theme/#1933): Scope matching precedence #2301

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

Merged
merged 7 commits into from
Aug 17, 2020

Conversation

bryphe
Copy link
Member

@bryphe bryphe commented Aug 14, 2020

Issue: Our scope-selection strategy was not correct in some cases - particularly, the precedence in which we would apply the scopes.

For example, in #1933 - with the dracula theme - there'd be a case where a token would have the following textmate scopes:

  • support.function.console.js
  • meta.function-call.js
  • source.js

For the dracula theme, there is a source selector that is intended to be a fallback - however, it was taking precedence over some of the other theme selectors that should've been applied, like meta.function-call.

This would cause the entire source file to be highlighted with the source scope selector, instead of the more specific ones.

Defect: We apply selectors in reverse order (first, we apply any selectors matching source.js, and then meta.function-call.js source.js, etc...) - this gives us right precedence. However, if we had a match, we were failing to 'fall-back' to a more specific selector.

Fix: Add a test case to reproduce, and fall back to override with more specific scopes.

This looks to fix a couple of related issues:

Thanks @CrossR for the helpful tool improvements in #2255 and @thismat for the investigation to narrow down the issue in #1933 !

dracula theme - before:

image

dracula theme - after:

image

@bryphe bryphe merged commit cbc1123 into master Aug 17, 2020
@bryphe bryphe deleted the fix/theme/1933/scope-matching-precedence branch August 17, 2020 15:39
@bryphe bryphe mentioned this pull request Aug 27, 2020
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.

1 participant