fix(builders): open every choice builder with focus on Done - #1812
Conversation
Obsidian's Modal.open() focuses the first focusable element. The Macro builder builds its content before open(), so its title rename button got focus and a focus ring; the Template/Capture forms mount after open(), so their footer Done got it. addAutosaveFooter now focuses Done and both builder hosts call it after open(). Amp-Thread-ID: https://ampcode.com/threads/T-01a0df6a-959f-74bb-a96a-d523ad2e20e9 Co-authored-by: Christian Bager Bach Houmann <christian@bagerbach.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe autosave footer now focuses its Done button. ChoiceBuilder and MacroBuilder install the footer after opening their modals. A test checks the initial focus in Macro, Template, and Capture builders. ChangesBuilder initial focus
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The builders are set up to focus Done when opened. No issue identified here prevents merging after normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit taps the Done button bright, Comment |
2 participants
Summary
The choice builders opened with focus on different elements. The Macro builder focused its title rename button, which showed a focus ring around the title (visible in the #1809 screenshots). The Template and Capture builders focused the footer Done button. Now all three open with focus on Done.
Cause (confirmed in Obsidian 1.13.7)
Modal.prototype.open()in Obsidian'sapp.jsrunsthis.onOpen()and then, whenhasPhysicalKeyboard && this.hasInitialInputFocus, calls a helper that focuses the first visible element inmodalElthat matches a focusable selector, in DOM order (it ignoresautofocus). No QuickAdd code set focus.MacroBuilderbuildscontentEl(title first) beforeopen(), so the title's rename button was first.ChoiceBuilder(Template/Capture) callsopen()from its constructor, and the Svelte form mounts after that, so the footer's Done was the only focusable element atopen().MultiChoiceSettingsModal) focuses its Name input. I left it unchanged on purpose: it is a small Save/Cancel form without the autosave footer, and typing a name is what the user opens it for.Fix
addAutosaveFooternow callsdone.focus(), and both builder hosts (ChoiceBuilder,MacroBuilder) call it afteropen(), so it runs after Obsidian's autofocus. Two statements were reordered and one line was added. No new state or options.Why Done:
mod-cta) and already the target in Template/Capture, so only the Macro builder changes.On devices without a physical keyboard, Obsidian skips its own autofocus. This change focuses Done unconditionally, which does not open a soft keyboard because Done is a button.
Before / after (real Obsidian 1.13.7, isolated e2e vault, made-up choices)
Macro builder: before, the focus ring is on the title. After, it is on Done.
Template builder: unchanged (Done both times).
Capture builder: unchanged (Done both times).
Verification
I opened each builder through the real UI (Settings → QuickAdd → Configure <choice>) with
pnpm run obsidian:e2e -- eval, readdocument.activeElement800 ms after the click, then closed it with a real Escape key (webContents.sendInputEvent). There was exactly one builder open at each reading, and none left after Escape. The instance was restarted before the run.document.activeElementon masterbutton.mod-cta"Done" in.qa-builder-footerbutton.mod-cta"Done" in.qa-builder-footerbutton.choiceNameHeaderButton.qa-rename-title-buttoninh2.choiceNameHeaderbutton.mod-cta"Done" in.qa-builder-footerinputReal keyboard input on this branch (
sendInputEvent):input), and Tab returns to Done.dev:errors: "No errors captured."Checks:
pnpm run build-with-lint: passes.pnpm run test: 448 files passed, 5 skipped; 5811 tests passed, 24 skipped.src/gui/ChoiceBuilder/builderInitialFocus.test.tsopens the Macro, Template, and Capture builders and asserts thatdocument.activeElementis the footer's Done. The Obsidian stub'sopen()does not autofocus, so the test wraps it to focus the first focusable element inmodalEl, as Obsidian does. On master the Macro case fails. It also fails for the plausible wrong fix, wheredone.focus()is added but the call is left beforeopen(), because the emulated autofocus then moves focus back to the title.Notes
main.jsandstyles.cssare gitignored).Note
Fix choice builders to focus Done button after modal open
Modal.open(). The autosave footer is now added after the modal opens, so its Done button becomes the final focus target.addAutosaveFootercalls focus on the Done button right after insertion, and constructors in choiceBuilder.ts and MacroBuilder.ts were reordered to open the modal before installing the footer.Macroscope summarized 2d14325.
Summary by CodeRabbit