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

(feat) Semantic document highlight #1408

Merged
merged 39 commits into from
Jan 8, 2025

Conversation

jasonlyu123
Copy link
Member

This adds support for document highlight. Making the document highlight be based on semantic/syntactic meaning instead of the current word-based default by VSCode. This also makes the document highlight available for other LSP client since a lot of them doesn't have a word-based fallback and it does look kind of weird without it.

As usual, the result is provided by vscode-css-languageservice, vscode-html-languageservice and typescript. As for the svelte plugin, I highlight the svelte block and tags. For example, if block highlights #if, :else if, :else and /if.

The downside of the current approach is that ts, svelte document highlight in the markup won't work if there's a syntax error in the markup. Also, for unsupported embed language users, like pug, sass, postcss and stylus. It might be a worse experience. With that said I think it would still be a good addition.

To discuss:

  1. Do we think this is better than the word-based in vscode? If not maybe we could make it disablable and disable this by default in vscode. But enable it in the server by default. For it to work, we have to set the server capability based on config.

  2. To address the worse experience for unsupported embed language. I am thinking if we can implement a word-based by ourselves and return the word-based result for these languages.

  3. The svelte compile result may not be reliable enough. So maybe we can blank the style and script tag like in svelte2tsx and use that instead. Or maybe even move to the typescript plugin and use the AST returned by svelte2tsx instead?

@igorlfs
Copy link

igorlfs commented Jan 12, 2024

Hey, is there any chance of this getting merged? It'd be nice to have when using other clients

@jasonlyu123
Copy link
Member Author

The main reason for the inactive is that this is not reliable. It won't work when there is a syntax error in the template. But maybe it's still better than nothing. We could probably mark it as experimental and off by default.

@jasonlyu123
Copy link
Member Author

It's now marked as experimental and off by default. We could make it on by default if people don't care much about the parser error problem. Similar to the folding range. I also added a word-based solution for unsupported embedded languages. The svelte result is also calculated using the AST returned by svelte2tsx. Unlike the folding range, there isn't a word-based fallback when there is a parser error because the result is too different.

@jasonlyu123 jasonlyu123 marked this pull request as ready for review January 17, 2024 06:11
@jasonlyu123
Copy link
Member Author

@dummdidumm What do you think about this? Do you think it makes sense to mark it as "experimental" or just off by default should be fine? Or maybe you think it can be enabled by default. The "unreliable" is similar to hover so maybe it's acceptable? I am just not sure if people would think the "unreliable" is worse than the default word highlight.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to respond - now that Svelte 5 has a loose parser, does that alleviate some of the concerns with not enabling this by default?

Could you show some screenshots of before/after and also some of the cases where it breaks down? I gotta admit I'm not that knowledgeable about the whole semantic document highlighting topic.

packages/language-server/README.md Outdated Show resolved Hide resolved
packages/svelte-vscode/package.json Outdated Show resolved Hide resolved
@jasonlyu123
Copy link
Member Author

Hmmm. While I was trying to take some screenshots of it breaking. I found that there is weirdly a word-based fallback now and HTML doesn't have the same behaviour. Not sure if this is supposed to be a new feature or a VSCode bug. I also found some bugs with nested blocks. I'll check both later. And about experimental or not, I think I am leaning toward enabling it by default now we have the loose parser.

@jasonlyu123
Copy link
Member Author

Okay, the word highlight fallback should be intentional. If we return an empty array instead of null it'll stop the fallback.

圖片

This is what it looks like when there are highlights. And in the parser error state, it is just nothing. But you could still trigger the word base highlight when double-clicking on the word so maybe it isn't that big of a deal. I think we can enable this by default. And if enough people find the parser error state annoying we can consider leveraging the word highlight fallback in svelte 4 files.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

More than 2,5 years in the making, kudos for your persistence 😄

@dummdidumm dummdidumm merged commit 931dd85 into sveltejs:master Jan 8, 2025
3 checks passed
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