Fix manifest editor losing UI state on external XML edits - #194
Open
Chiara Mooney (chiaramooney) wants to merge 10 commits into
Open
Fix manifest editor losing UI state on external XML edits#194Chiara Mooney (chiaramooney) wants to merge 10 commits into
Chiara Mooney (chiaramooney) wants to merge 10 commits into
Conversation
The AppxManifest visual editor reset its tab, per-application sub-tab, expanded optional fields and scroll position whenever the XML was edited in the text editor. Typing an element such as `<Capability Name="x" />` passes through many transiently-unparseable states, and each one made the provider reassign `webview.html`, tearing down the webview script context and all of its state. - Keep the editor document loaded on parse failures and surface the error as an in-place overlay; only build the standalone error page when the manifest was already unparseable at open time. - Debounce external document changes so mid-word edits aren't re-rendered. - Persist tab, app sub-tab, expanded optional fields and scroll offsets via vscode.setState()/getState(), and restore scroll around list re-renders. - Clear `.tab-btn.active` when a tab is hidden for the package type, so only one tab button is ever marked selected. - Drop the dead `retryParse` message listener from the parse-error page. Test harness: launch VS Code with a dedicated `--user-data-dir`/`--extensions-dir`. Without it the launched process forwards its arguments to any VS Code already running and exits immediately, closing Playwright's Electron handle. Adds `external-edit-state.spec.ts` (VS Code) and `webview-state.spec.ts` (Chromium) as regression coverage. Fixes #192 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 82fe22e4-b778-41ce-a814-4c1a20472408
- CI: install Playwright Chromium before E2E, so webview-state.spec.ts (browser-backed, unlike the Electron specs) can run in both pipelines. - Parse-error overlay: add the "Open in Text Editor" action the standalone error page offers, and make the overlay modal by inerting/aria-hiding the form behind it and moving focus into the alert. Drop the 0.97 opacity so the paused form isn't faintly visible. - Fix restoreUiState() being consumed even when the saved tab was still hidden, which meant a tab hidden on the first update was never restored. - E2E harness: clean up the temp profile dir when a launch fails. - Guard test:e2e on verify-schemas and document sync-schemas + chromium in the E2E README; drop the now-stale "close all VS Code windows" note. - Cover the initial-parse-error -> editor recovery path, the pending-restore case, and the overlay's action/modality. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 82fe22e4-b778-41ce-a814-4c1a20472408
VSIX Build
Updated 2026-08-26 23:07:06 UTC · commit |
- Overlay is now role="alertdialog" with aria-modal and aria-describedby, and has a visible focus ring instead of a suppressed outline. - setEditorContentInert snapshots any pre-existing aria-hidden so un-inerting restores the per-panel state activateTab owns instead of erasing it. - parse-error.spec.ts launches VS Code in beforeAll, so each test runs standalone. - webview-state.spec.ts asserts aria-hidden is handed back after recovery, and adds coverage for focus restore when the re-render destroys the focused element. - Chromium install is centralized in pretest:e2e; removed the duplicated steps from the GitHub and Azure pipelines. - Updated the E2E README test table order, counts, and flaky-test note. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 82fe22e4-b778-41ce-a814-4c1a20472408
Keeping the webview alive across a parse error (the #192 fix) also keeps its 300ms input debounce alive, so an edit typed just before the XML broke could fire afterwards and rewrite the document from unparseable text. - webview: discard pending debounced changes when the parse-error overlay first appears - provider: refuse to apply field changes or flush saves when the current document text does not parse, resolving the pending save so it cannot hang - defer parse-error focus restoration until after populateForm re-renders, and re-query the target by attribute descriptor so focus lands on the rebuilt control instead of a destroyed one - trap Tab/Shift+Tab inside the parse-error dialog while it is open - reword the standalone parse-error page: it reloads automatically - clean up the parse-error spec temp dir even when setup fails - add 3 webview-state tests (debounce discard, Tab trap, never-visible tab) and refresh the e2e README inventory Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 82fe22e4-b778-41ce-a814-4c1a20472408
Round-4 review follow-ups. - discard pending webview input as soon as the document changes externally, not only when the parse-error overlay appears. XML that breaks and is fixed again inside the 250ms re-render debounce never shows an overlay, so queued input typed against the pre-edit text could replay over the newer edit. - route extension-field inputs through the shared debounce queue via a new queueDebouncedChange() helper. They previously used a private setTimeout, so they were invisible to both save-flush and parse-error discard; a save could drop the user's last extension-field keystrokes. - teach the changesFlushed save path to apply both change kinds, and give the flushed-change payload a named FlushedChange type. - cover the save-flush parse guard with a real VS Code test, plus three webview tests for the external-change discard and the extension-field flush/discard paths. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 82fe22e4-b778-41ce-a814-4c1a20472408
Two <Host> elements render two inputs with the same data-ext-field, so the queue key built from app/ext index plus field path collided and one input's queued keystroke silently replaced the other's. Before the shared queue each input had a private timer, so this was a regression; add the element ordinal to the key to restore per-input isolation, with a test that fails without it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 82fe22e4-b778-41ce-a814-4c1a20472408
The CI failures on this branch were real, not flaky. Debouncing the external- change re-render by 250ms moved it out of the old synchronous path and into the window right after a dropdown is opened, and populateForm rebuilds the app cards wholesale, so the menu closed under the user (and under the two add-extension E2E tests) before the item could be clicked. Snapshot which dropdown menus are open and reopen them after the rebuild, the same way scroll offsets and focus are already preserved, and give the extension menu a data-app-index so its key stays stable across cards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 82fe22e4-b778-41ce-a814-4c1a20472408
Drop the standalone parse-error page and its styles. The editor document now loads even when the XML is unparseable, and every parse failure raises the same in-place overlay, so there is one error view instead of two slightly different ones. This also removes the documentMode state machine. Reword the overlay copy without em dashes and cut the comments added by this PR down to two lines each. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 82fe22e4-b778-41ce-a814-4c1a20472408
The XML normally becomes unparseable while the user is typing in the text editor, and the overlay called box.focus() unconditionally, which pulled focus out of the text editor and ate keystrokes. Only move focus into the dialog when the webview already had focus, and apply the same rule when restoring it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 82fe22e4-b778-41ce-a814-4c1a20472408
The visual editor had two paths that could pull focus away from the text editor while the user was typing: the parse-error overlay focusing itself, and populateForm restoring focus on a forced (external) re-render. The latter was a regression against main, which deliberately skipped focus capture when forceAll was set. Tab and scroll position are what matter for #192; cursor position is not worth the risk. Drop the overlay focus machinery entirely and restore main's forceAll behaviour. Focus restoration during the webview's own edits is unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 82fe22e4-b778-41ce-a814-4c1a20472408
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #192
Problem
The AppxManifest visual editor lost its UI state (selected top-level tab, per-application sub-tab, expanded optional fields, and scroll position) whenever the XML was edited in the text editor.
Typing an element such as
<Capability Name="internetClient" />passes through many transiently-unparseable states. On each one, the provider reassignedwebviewPanel.webview.htmlto swap in the parse-error page, which tears down the webview's JS context and destroys all of its state. This also explains the asymmetry the reporter noticed: editing an attribute value never breaks the XML, so state survived there.Changes
Root cause 1 — document teardown on transient parse errors
The editor document now always loads, and every parse failure raises the same in-place overlay via a
parseErrormessage. Nothing reassignswebview.htmlafter load, so the JS context is never torn down.This meant picking one of the two error views, and the standalone page could not be the one that survived: it is a whole separate HTML document, so showing it is precisely the teardown that causes #192. It has been deleted (
getParseErrorContent,getErrorPageStyles, and its deadretryParsehandshake), along with thedocumentMode/errorPageMessagestate machine that chose between the two views.The overlay carries over the original's ⚠ icon, "Open in Text Editor" button, and raw parser message. The copy changed because the original's was no longer accurate — the editor does not fail to load and you do not reopen it, it recovers on its own as soon as the XML parses again:
Root cause 2 — state not preserved across re-renders
vscode.setState()/getState()and restored on load.populateFormsnapshots and re-applies scroll offsets around theinnerHTMLclears inrenderApplications/renderReorderableList.Not preserved: focus and cursor position. The webview never moves focus, in either direction. The overlay does not focus itself when it appears and does not restore focus when it clears, and
populateFormskips focus capture on forced (external) re-renders. Restoring focus meant calling.focus()inside the webview iframe, which yanks the caret out of the text editor mid-keystroke. Focus restoration during the webview's own edits is unchanged frommain. The form behind the overlay is stillinert, so keyboard and screen-reader users cannot reach paused fields.Edit-safety guards
Additional fixes
.tab-btn.active/aria-selected, so exactly one tab button is ever marked selected.Test harness fix
launchVSCodenow passes a dedicated--user-data-dir/--extensions-dir. Without profile isolation, the launchedCode.exeforwards its arguments to any VS Code already running on the machine and exits immediately, closing Playwright's Electron handle and failing the whole suite withTarget page, context or browser has been closed.Testing
src/test/e2e/webview-state.spec.ts(new, Chromium, 15 tests): renders the real generated webview HTML with a stubbedacquireVsCodeApiand drives the extension→webview protocol directly. Deterministic and fast (~20 s). Covers state restoration, overlay modality, the focus policy above, the debounce-discard guards, and dropdown preservation.src/test/e2e/external-edit-state.spec.ts(new, real VS Code, 4 tests): replays a transiently-invalid edit written to disk and asserts tab, app sub-tab, and scroll survive it; that the overlay appears in place and clears on recovery; that exactly one tab button stays active; and that saving while unparseable stores the raw text instead of rewriting the document. Verified as genuine regression coverage — 2 of 3 original tests fail without the fix. Uses its own VS Code instance because it drives on-disk edits, which conflict with the unsaved webview edits other specs leave in the shared editor.src/test/e2e/parse-error.spec.ts(2 tests) updated to assert the unified overlay, proving the initially-unparseable open path works end to end.sign-quickpicktests, which CI excludes via--grep-invert; 4 further tests are known flakes. No regressions.npm run compile,npm run compile-tsc,npm run lintall clean (11 pre-existingcurlywarnings, unchanged).