Skip to content

fix: clamp completion range when scanner crosses tag boundary#247

Open
maruthang wants to merge 2 commits intomicrosoft:mainfrom
maruthang:fix/issue-273226-clamp-across-tags
Open

fix: clamp completion range when scanner crosses tag boundary#247
maruthang wants to merge 2 commits intomicrosoft:mainfrom
maruthang:fix/issue-273226-clamp-across-tags

Conversation

@maruthang
Copy link
Copy Markdown
Contributor

Summary

Follow-up to #239 for the same issue microsoft/vscode#273226. That PR handled the case where the attribute value has no closing quote anywhere; @federicobrancasi reported that the bug still reproduces in 1.117.0 Insiders when the document contains an unrelated later quote.

Root cause

In htmlCompletion.ts the replace range uses valueEnd, which comes from the scanner. When the opening " of an attribute value has no matching close on the same logical context, the scanner continues across </tag> boundaries and latches onto the next " it finds — e.g. the opening " of a different title="..." attribute elsewhere in the document. At that point text[valueEnd - 1] === text[valueStart] is true, so my previous else clamp didn't trigger and the replace range spanned the entire intervening markup.

Accepting checkbox from a completion on

<th><input type="che|</th><p title="later"></p>

would produce

<th><input type="checkbox title="later"></p>

wiping out </th><p . The regression test <th><input type="che|</th><p title="later"></p> (added in this PR) fails on main with exactly that output; see the test output in the repro notes below.

Fix

Even when a matching closing quote is found, scan the stretch from the cursor to that quote. If any < appears in that region, treat the value as unclosed and clamp valueContentEnd to offset. Any legitimate attribute-value content containing < would already be HTML-escaped to &lt;, so an unescaped < in that region is a reliable signal that the scanner crossed a tag boundary.

Changes

  • src/services/htmlCompletion.ts — detect tag-boundary-crossing between cursor and alleged closing quote; clamp to offset in that case.
  • src/test/completion.test.ts — three regression tests:
    • unclosed quote + stray " on the same line (exact federicobrancasi scenario)
    • unclosed quote + stray " on a later line
    • unclosed quote crossing a \n before the closing tag

Testing

Fixes microsoft/vscode#273226 (round two).

When an attribute value has an unclosed quote but the scanner latches onto
an unrelated later quote in the document, the completion replace range
could still extend across HTML tag boundaries, wiping markup between the
cursor and that later quote. Extend the clamp to also trigger when any
'<' appears between the cursor and the alleged closing quote.

Refs microsoft/vscode#273226

Signed-off-by: Maruthan G <maruthang4@gmail.com>
Comment thread src/services/htmlCompletion.ts Outdated
// delete subsequent markup. See microsoft/vscode#273226.
let crossesTagBoundary = false;
for (let i = offset; i < valueEnd - 1; i++) {
if (text.charCodeAt(i) === 0x3C /* < */) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

use the constant in the htmlScanner

Signed-off-by: Maruthan G <maruthang4@gmail.com>
@maruthang
Copy link
Copy Markdown
Contributor Author

Done in 0365fb7 — exported _LAN from htmlScanner.ts and use it in htmlCompletion.ts. All 155 tests still pass. PTAL.

@maruthang
Copy link
Copy Markdown
Contributor Author

Friendly ping @aeschli — addressed the inline comment in 0365fb7 by exporting _LAN from htmlScanner.ts and importing it in htmlCompletion.ts (no more duplicated literal). All 155 tests still pass. Ready for another look whenever convenient. Thanks!

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.

[html] completions: better handle unclosed string literals

2 participants