feat(Dialog): add unmountOnHide prop#2662
Open
benjamincanac wants to merge 10 commits into
Open
Conversation
When set to false, Dialog content and overlay stay in the DOM when closed (hidden via v-show) instead of being unmounted. Useful for SEO and performance by avoiding remounts on every open. Closes unovue#1727
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an ChangesDialog unmountOnHide Implementation
Sequence DiagramsequenceDiagram
participant Client
participant DialogRoot
participant Presence
participant DialogContentModal
participant DialogContentNonModal
participant DialogOverlayImpl
Client->>DialogRoot: mount with unmountOnHide (true|false)
DialogRoot->>Presence: provide :present/:force-mount (computed)
Presence->>DialogContentModal: present slot value
Presence->>DialogContentNonModal: present slot value
Presence->>DialogOverlayImpl: present slot value
DialogContentModal->>DialogContentModal: use present to set aria-hidden target & pointer events
DialogContentModal->>DialogContentModal: watch present -> restore focus when false
DialogOverlayImpl->>DialogOverlayImpl: watch present -> toggle body scroll lock
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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. Comment |
commit: |
benjamincanac
changed the title
unmountOnHide prop
Also fixes focus restoration when unmountOnHide=false — since FocusScope never unmounts, close-auto-focus doesn't fire. Added a watcher on present to manually restore focus to trigger when the dialog hides.
|
You dont believe how happy you make me with this PR! Will test soon, hope it gets merged eventually... |
…n-hide # Conflicts: # packages/core/src/Dialog/Dialog.test.ts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/Dialog/Dialog.test.ts (1)
121-124: ⚡ Quick winStrengthen the aria-hidden assertion by exercising the open→close cycle.
This test only checks the initial (never-opened) state. The core value of the
present-gateduseHideOthersis removingaria-hiddenfrom the rest of the page after the dialog has been opened and then closed (while the content stays mounted). Consider opening, then pressing Escape, then assertingbodyhas noaria-hiddento actually cover that path.💚 Suggested test enhancement
it('should not apply aria-hidden to body when closed', async () => { + await fireEvent.click(trigger.element) + await nextTick() + await fireEvent.keyDown(document.activeElement!, { key: 'Escape' }) await nextTick() expect(document.body.getAttribute('aria-hidden')).toBeNull() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/Dialog/Dialog.test.ts` around lines 121 - 124, The test currently only verifies the initial state; update 'should not apply aria-hidden to body when closed' to exercise the open→close cycle: render/open the Dialog (use the component's present/open API or simulate the trigger to set present=true), await nextTick, simulate closing via Escape (dispatch a KeyboardEvent or use the library's close method), await nextTick again, then assert document.body.getAttribute('aria-hidden') is null; this will exercise useHideOthers' present-gated behavior and confirm aria-hidden is removed after close while content remains mounted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/Dialog/Dialog.test.ts`:
- Around line 121-124: The test currently only verifies the initial state;
update 'should not apply aria-hidden to body when closed' to exercise the
open→close cycle: render/open the Dialog (use the component's present/open API
or simulate the trigger to set present=true), await nextTick, simulate closing
via Escape (dispatch a KeyboardEvent or use the library's close method), await
nextTick again, then assert document.body.getAttribute('aria-hidden') is null;
this will exercise useHideOthers' present-gated behavior and confirm aria-hidden
is removed after close while content remains mounted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67453a23-6bee-4ca2-9e43-b4cc6aff568c
📒 Files selected for processing (2)
packages/core/src/Dialog/Dialog.test.tspackages/core/src/Dialog/DialogOverlayImpl.vue
The focus-restoration watcher only lived in DialogContentModal, so a non-modal dialog with unmountOnHide=false never returned focus to the trigger on close (close-auto-focus doesn't fire while mounted). Mirror the watcher in DialogContentNonModal, respecting interact-outside, and document why the watcher exists in both.
Assert the rest of the page stays accessible after a full open/close cycle (content remains mounted) rather than just checking initial state.
…hile mounted When `disableOutsidePointerEvents` toggles true→false without unmounting (the unmountOnHide=false case), the watchEffect cleanup read the prop reactively and saw the new false value, skipping the body pointer-events restore and leaving the page frozen. Capture the value at run time for cleanup, and read the layer set size via toRaw to avoid self-retriggering.
With unmountOnHide=false the FocusScope stays mounted across open/close, so the mount auto-focus (keyed off physical mount) never re-fires on reopen and focus never enters the content. Watch the trapped false->true transition and re-run the mount auto-focus. The default unmount-on-close path mounts already trapped, so this transition never happens there and behavior is unchanged.
2 tasks
When a force-mounted scope (`unmountOnHide: false`) becomes trapped on reopen, the `v-show` visibility can apply a frame after `trapped` flips, so the first focus attempt runs while the container is still `display: none` and no-ops. Retry the focus move on the next frame without re-dispatching the auto-focus event.
8 tasks
…opes A force-mounted scope (Dialog `unmountOnHide: false`) physically mounts once while hidden via `v-show`, so keying auto-focus off physical mount fired `mountAutoFocus`/`openAutoFocus` and stole focus into a closed dialog, and never re-focused non-modal content on open (it isn't trapped). Gate the mount-time auto-focus on visibility and re-run it on the hidden -> visible transition via a MutationObserver, covering first open and reopen for both modal and non-modal. Replaces the trapped watcher + rAF retry, which only handled the modal case.
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
2 participants
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.
Resolves #1727
Adds
unmountOnHidetoDialogRoot. Whenfalse, dialog content and overlay stay in the DOM when closed (hidden viav-show) instead of being unmounted. Useful for SEO and avoiding remounts.Based on #2494 (credit to @MickL), with fixes for
aria-hiddenleaking while closed, thepresentprop leaking to the DOM, scroll lock staying on when hidden, focus restoration when the content stays mounted (modal + non-modal), and aDismissableLayerfix so body pointer-events restore whendisableOutsidePointerEventstoggles off without unmounting.Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes