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.

Sign up for GitHub

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

Jump to bottom

Fix unescaped special characters in minihtml #2036

Merged
merged 1 commit into from
Aug 29, 2022
Merged

Fix unescaped special characters in minihtml #2036

merged 1 commit into from
Aug 29, 2022

Conversation

jwortmann
Copy link
Member

Make sure special characters like & are properly escaped in minihtml content.

For example with LSP-html and hover over the link in the following file:

<!DOCTYPE html>
<html>
<body>
    <img src="https://avatars.githubusercontent.com/u/684879?s=200&v=4" />
</body>
</html>

I assume that this might fix #2034, but due to lack of motivation to install Rust + rust-analyzer manually, I have not tested it.
@minerscale If you want, you could give it a try and see if it fixes your issue :)

@rchl
Copy link
Member

rchl commented Aug 29, 2022

I assume that this might fix #2034, but due to lack of motivation to install Rust + rust-analyzer manually, I have not tested it.

By looking at the changes I would assume that it's not gonna fix that issue as your change only handles links and title elements and the one in that issue is none of those.

@jwortmann
Copy link
Member Author

I can't say for sure without the payload from rust-analyzer, but how do you know that it's not the title element in the linked issue that causes the parsing bug? The label is already properly escaped

LSP/plugin/inlay_hint.py

Line 123 in bd4e014

result += html.escape(label)

so I guess the tooltip / title is the only possible cause that is left.

@rchl
Copy link
Member

rchl commented Aug 29, 2022

Because tooltip shows up on hovering the phantom, as tooltips do :)

@rchl
Copy link
Member

rchl commented Aug 29, 2022

And actually tooltip is not rendered using minihtml so shouldn't be escaped.

@jwortmann
Copy link
Member Author

But it's already given in the html content when created the phantom. Here:

LSP/plugin/inlay_hint.py

Lines 90 to 95 in bd4e014

<div class="inlay-hint" title="{tooltip}">
{label}
</div>
</body>
""".format(
tooltip=tooltip,

There is probably an error in the console with "parsing error", so that ST doesn't want to render the phantom at all.

@rchl
Copy link
Member

rchl commented Aug 29, 2022

Sorry, my bad. The title is declared through the title attribute in minihtml so it does need to be escaped and it fixes that issue (I've checked).

@rchl rchl merged commit 5e6f49d into sublimelsp:main Aug 29, 2022
@KristonCosta KristonCosta mentioned this pull request Aug 29, 2022
Closed
@minerscale
Copy link

Huzzah! It works great! Thanks very much everyone y'all great.

@jwortmann jwortmann deleted the fix-minihtml-escape branch August 30, 2022 07:57
rchl added a commit that referenced this pull request Aug 30, 2022
@rchl
Merge branch 'main' into feat/diag-annotations
72a0878 
* main:
  fix 3.8 compatibility by not using named argument in set_timeout_async
  Optionally fallback to goto_reference in lsp_symbol_references (#2029)
  Fix unescaped special characters in minihtml (#2036)
  Add inlay hints to docs (#2028)
  Add option to highlight hover range (#2030)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rchl rchl rchl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ampersands (&) in inlay hints are broken
3 participants
@jwortmann @rchl @minerscale