fix(editbar): restore caret to pad after toolbar-select change (#7589)

[JohnMcLear]

Summary

Closes #7589. After picking a value from a toolbar `` (ep_headings' style picker is the canonical case), keyboard focus was left on the nice-select wrapper rather than returned to the pad editor. Users had to click back into the pad before typing resumed. Root cause `ToolbarItem.bind()` in `pad_editbar.ts` calls `padeditor.ace.focus()` at the end of `triggerCommand` for its own wired selects — but only selects sitting inside an `` with `data-key`. Plugin-provided selects bypass this: `ep_headings2` wraps `#heading-selection` in `<li id="headings">` with no `data-key`. It binds its own `change` handler directly: `ace.callWithAce(...)` then nothing. Focus stays on the nice-select wrapper; next keystroke is lost. Fix Add a delegated `change` handler on `#editbar select` in `pad_editbar.ts` that calls `padeditor.ace.focus()` after any toolbar select change. Deferred via `setTimeout(0)` so plugin change handlers (bound on the same event) complete first. Redundant but harmless for data-key-wired selects that were already refocused by `triggerCommand`. Tests New Playwright spec `select_focus_restore.spec.ts` that simulates the nice-select option-click (the wrapper dispatches `val(x).trigger('change')` internally — see `static/js/vendors/nice-select.ts`) and asserts typing after the change lands in the pad. Skips if `ep_headings2` isn't installed. Test plan [x] Manual verification: pick a heading style in the toolbar, keep typing — caret stays in the pad. [x] `select_focus_restore.spec.ts` passes on Chromium and Firefox.

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Restore caret to pad after toolbar select change

🐞 Bug fix 🧪 Tests

Walkthroughs

Description

• Restore keyboard focus to pad editor after toolbar select changes
• Add delegated change handler on #editbar select elements
• Deferred focus restoration via setTimeout(0) for plugin handlers
• New Playwright regression test for select focus behavior

Diagram

flowchart LR
  A["Toolbar select change event"] --> B["Delegated handler on #editbar"]
  B --> C["setTimeout defers focus call"]
  C --> D["Plugin handlers complete first"]
  D --> E["padeditor.ace.focus() restores caret"]

Loading

File Changes

1. src/static/js/pad_editbar.ts 🐞 Bug fix +15/-0

Add delegated select change handler for focus restoration

• Added delegated change handler on #editbar select to restore focus
• Uses setTimeout(0) to defer focus call after plugin handlers complete
• Ensures keyboard focus returns to pad editor after any toolbar dropdown change
• Includes detailed comments explaining the fix for issue #7589

src/static/js/pad_editbar.ts

---

2. src/tests/frontend-new/specs/select_focus_restore.spec.ts 🧪 Tests +39/-0

Add regression test for select focus restoration

• New Playwright regression test for toolbar select focus behavior
• Simulates nice-select option click via val() and dispatchEvent('change')
• Verifies typing after select change lands in pad editor, not toolbar
• Skips test if ep_headings2 plugin is not installed

src/tests/frontend-new/specs/select_focus_restore.spec.ts

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. Flaky async focus test 🐞 Bug ☼ Reliability

Description

The focus restore in pad_editbar.ts is deferred via setTimeout(0), but the Playwright spec types
immediately after dispatching the change event, so early keystrokes can be sent before focus returns
to the editor. This can cause intermittent failures depending on event-loop timing and test runner
load.

Code

src/tests/frontend-new/specs/select_focus_restore.spec.ts[R27-36]

+  await hs.evaluate((el: HTMLSelectElement) => {
+    el.value = '0';
+    el.dispatchEvent(new Event('change', {bubbles: true}));
+  });
+
+  // After the change, keyboard input should go into the pad, not the
+  // toolbar. Write a marker and verify both chunks appear in the pad.
+  await page.keyboard.type('after');
+  await page.waitForTimeout(200);
+  const bodyText = await padBody.innerText();

Evidence

The production code intentionally defers focus restoration to a later task using setTimeout(0), but
the test does not wait for that deferred focus to occur before typing, creating a race.

src/static/js/pad_editbar.ts[147-160]
src/tests/frontend-new/specs/select_focus_restore.spec.ts[27-36]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The test types immediately after triggering the select `change`, but the product code restores focus asynchronously via `setTimeout(0)`. This introduces a race where some keystrokes can be sent before focus is back on the Ace editor.

### Issue Context
The implementation in `pad_editbar.ts` defers the focus call specifically to run after other `change` handlers and after nice-select's own focus behavior.

### Fix Focus Areas
- src/tests/frontend-new/specs/select_focus_restore.spec.ts[23-38]

### Suggested change
After dispatching the `change` event, explicitly wait until the editor has focus before calling `page.keyboard.type('after')`.

Example approaches (pick one):
- `await expect(page.locator('iframe[name="ace_outer"]')).toBeFocused();`
- `await page.waitForFunction(() => (document.activeElement as HTMLElement | null)?.getAttribute?.('name') === 'ace_outer');`

Also replace `waitForTimeout(200)` with an `expect.poll(...)` / `await expect(padBody).toContainText(...)`-style wait to reduce flakiness.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Test doesn't force toolbar focus 🐞 Bug ≡ Correctness

Description

The spec clicks the pad body and then changes the select value programmatically, but it never puts
focus onto the nice-select wrapper (the real regression state), so the test can pass even if focus
restoration from the toolbar is broken. This reduces confidence that the test is exercising the
actual user flow that leaves focus on the toolbar control.

Code

src/tests/frontend-new/specs/select_focus_restore.spec.ts[R19-30]

+  const padBody = await getPadBody(page);
+  await padBody.click();
+  await page.keyboard.type('before');
+
+  // Change the heading style. The native <select> is hidden behind the
+  // nice-select wrapper, which on option click does `val(x).trigger('change')`
+  // internally (see src/static/js/vendors/nice-select.ts). Replicate that
+  // directly rather than trying to click through the wrapper UI.
+  await hs.evaluate((el: HTMLSelectElement) => {
+    el.value = '0';
+    el.dispatchEvent(new Event('change', {bubbles: true}));
+  });

Evidence

The test explicitly focuses the editor first and then dispatches a change event without
interacting with (or focusing) the wrapper. In contrast, nice-select’s UI flow focuses the
.nice-select element when the dropdown closes, which is the problematic focus state this PR is
intended to override.

src/tests/frontend-new/specs/select_focus_restore.spec.ts[19-30]
src/static/js/vendors/nice-select.ts[97-129]
src/static/js/vendors/nice-select.ts[138-150]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The regression is specifically that focus ends up on the nice-select wrapper after a toolbar selection, but the test never moves focus to the toolbar control/wrapper before triggering the change. As a result, the test may not fail when the real focus regression returns.

### Issue Context
nice-select triggers the select's `change` and then (on close) focuses the `.nice-select` wrapper, which is the focus state the product code needs to correct.

### Fix Focus Areas
- src/tests/frontend-new/specs/select_focus_restore.spec.ts[19-35]

### Suggested change
Before triggering the `change`, explicitly focus the wrapper element (e.g. `#heading-selection + .nice-select`) or otherwise ensure focus is on a toolbar element, then trigger the change and assert focus returns to the editor.

Concrete example:
1. `const wrapper = page.locator('#heading-selection + .nice-select');`
2. `await wrapper.focus();` (optionally assert `await expect(wrapper).toBeFocused();`)
3. Trigger the `change`.
4. Wait for editor focus (see other finding) and then type/assert text.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-free-for-open-source-projects]

1. Flaky async focus test 🐞 Bug ☼ Reliability

The focus restore in pad_editbar.ts is deferred via setTimeout(0), but the Playwright spec types
immediately after dispatching the change event, so early keystrokes can be sent before focus returns
to the editor. This can cause intermittent failures depending on event-loop timing and test runner
load.

Agent Prompt

### Issue description
The test types immediately after triggering the select `change`, but the product code restores focus asynchronously via `setTimeout(0)`. This introduces a race where some keystrokes can be sent before focus is back on the Ace editor.

### Issue Context
The implementation in `pad_editbar.ts` defers the focus call specifically to run after other `change` handlers and after nice-select's own focus behavior.

### Fix Focus Areas
- src/tests/frontend-new/specs/select_focus_restore.spec.ts[23-38]

### Suggested change
After dispatching the `change` event, explicitly wait until the editor has focus before calling `page.keyboard.type('after')`.

Example approaches (pick one):
- `await expect(page.locator('iframe[name="ace_outer"]')).toBeFocused();`
- `await page.waitForFunction(() => (document.activeElement as HTMLElement | null)?.getAttribute?.('name') === 'ace_outer');`

Also replace `waitForTimeout(200)` with an `expect.poll(...)` / `await expect(padBody).toContainText(...)`-style wait to reduce flakiness.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools