Skip to content

[VSCodeSweeper MVP — test only] [html] don't validate custom event handlers as JavaScript#1

Draft
egamma wants to merge 2 commits into
mainfrom
vscodesweeper/fix-79631
Draft

[VSCodeSweeper MVP — test only] [html] don't validate custom event handlers as JavaScript#1
egamma wants to merge 2 commits into
mainfrom
vscodesweeper/fix-79631

Conversation

@egamma

@egamma egamma commented Jun 29, 2026

Copy link
Copy Markdown
Owner

⚠️ VSCodeSweeper MVP — test only. This draft PR was generated by an AI experiment
(vscodesweeper) inside a personal fork. It is
not intended for microsoft/vscode and is not a contribution proposal.

Upstream issue: microsoft#79631

Change summary

Custom HTML attributes prefixed with "on" are incorrectly validated as JavaScript event handlers, producing spurious ';' expected errors.

Validation

Add a test in extensions/html-language-features/server/src/test/embedded.test.ts asserting that an attribute like onCustomThing is NOT classified as JavaScript (languageId should be null/undefined), while known handlers like onclick still are.

Branch vscodesweeper/fix-79631 · generated fix · review loop runs separately (codex review).

@egamma egamma added the vscodesweeper-mvp VSCodeSweeper MVP test PR (fork-only) label Jun 29, 2026
@egamma

egamma commented Jun 29, 2026

Copy link
Copy Markdown
Owner Author

🤖 codex review — round 1

VSCodeSweeper MVP self-review (fork-only experiment).

The patch fixes false positives for invalid on... attributes but regresses JavaScript detection for several valid HTML event handler attributes omitted from the new allow-list.

Review comment:

  • [P2] Include valid window event handler attributes — /Users/egamma/.cache/vscodesweeper/fork/vscode/extensions/html-language-features/server/src/modes/embeddedSupport.ts:25-25
    Because this replaces the previous on\w+ handling with a fixed allow-list, valid HTML event handler attributes that are not in this set, such as onbeforeunload, onafterprint, onhashchange, onmessage, onoffline, ononline, onpagehide, onpageshow, onpopstate, onstorage, and onunload, are now treated as plain HTML. In files using those common handlers, getLanguageAtPosition and the embedded JavaScript document no longer provide JavaScript language features for the attribute value.

@egamma

egamma commented Jun 29, 2026

Copy link
Copy Markdown
Owner Author

🔧 Round 1: pushed commits addressing the review above; re-reviewing.

@egamma

egamma commented Jun 29, 2026

Copy link
Copy Markdown
Owner Author

🤖 codex review — round 2

VSCodeSweeper MVP self-review (fork-only experiment).

The patch fixes false positives for invalid on* attributes, but the hard-coded replacement list omits valid standard event handler attributes, causing regressions for those inputs.

Review comment:

  • [P2] Include all standard handler attributes — /Users/egamma/.cache/vscodesweeper/fork/vscode/extensions/html-language-features/server/src/modes/embeddedSupport.ts:27-27
    For documents that use standard window event handler attributes such as <body onbeforeprint="...">, <body onmessageerror="...">, or <body onunhandledrejection="...">, this new allow-list no longer classifies the attribute value as JavaScript even though the previous on\w+ handling did. That regresses language detection, highlighting, and embedded JS services for valid HTML event handler content attributes; the list should include the full standard set or be sourced from the HTML data used elsewhere.

@egamma

egamma commented Jun 29, 2026

Copy link
Copy Markdown
Owner Author

⚠️ Round 2: addressing the review did not complete (codex exec timed out after the run's time budget) — the remaining finding needs a human. Stopping the loop.

Standing finding (from rounds 1–2): the allow-list approach keeps missing valid standard event-handler attributes (onbeforeunload, onafterprint, onmessage, onunhandledrejection, …). Round 1 expanded the list, but round 2 still found gaps. The recommended direction — raised by the round-2 review itself — is to source handler names from the existing HTML data used elsewhere in the language service rather than hardcode a list, which avoids the whack-a-mole. That's a design call for a maintainer.

VSCodeSweeper MVP — fork-only experiment. This loop's later runs handle this stop gracefully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

vscodesweeper-mvp VSCodeSweeper MVP test PR (fork-only)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@egamma