Conversation
A RuiDialog is not a history entry, so a back gesture pops the entry underneath it and leaves the page it was sitting on, and with dialogs stacked one press skips all of them. It is not fixable from outside in a general way: a consumer can keep its own registry, but every dialog has to opt in by hand and each new one silently regresses. `useOverlayStack()` exposes `hasOverlay` and `dismissTop()`, and RuiDialog registers itself while open. `useDismissableOverlay()` is exported too, so anything else covering the page opts in the same way. Three things the contract has to get right, all of them learned the hard way in rotki's app-side version (rotki#579): - Dismissing is a request the layer may decline, so nothing is removed from the stack on dismissal. Layers leave when their own open state says so, and `dismissTop` looks for the topmost one still showing. Removing on dismissal deregisters a dialog that is still covering the page, and the next gesture sails past it. - Registration follows `modelValue` alone, never `persistent`. A dialog that turns persistent partway through its life must stay in the stack and refuse, or the gesture starts passing through exactly once there is work to lose. - `dismissTop` reports whether it handled the gesture, and a layer that refuses still counts as handled, so the caller can tell an aborted navigation from one to let through. Departures from the shape the issue proposed, each for a reason: - `hasOverlay` counts layers still showing rather than layers registered. Since nothing is removed on dismissal, the registered count says a dialog is up after it has gone. That is why a layer stores a getter rather than a boolean: the stack has to read it live, and a computed over a plain closure would never re-evaluate. - `closeTop` is `dismissTop`, since a persistent dialog answers it without closing. - Only RuiDialog registers. RuiBottomSheet renders one, so registering in both would put every sheet in the stack twice. - Ordering is registration order, not `zIndex`. Every dialog defaults to the same z-index, so it says nothing about which is on top. A new `dismiss` emit makes the gesture observable, rather than reusing `click:esc` and lying about where it came from. `resetOverlayStack()` is a test seam the unit project needs, since its pool shares one module registry across the files in a worker. Telling a pop from a push stays the consumer's business. The example app shows the wiring, taking the direction from vue-router's own history listener: a raw `popstate` listener cannot do it, because the restoration of an aborted pop is itself a pop and reads as a second gesture.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #584 +/- ##
==========================================
+ Coverage 87.33% 87.41% +0.07%
==========================================
Files 168 169 +1
Lines 6522 6563 +41
Branches 2057 2062 +5
==========================================
+ Hits 5696 5737 +41
Misses 826 826 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Four cases the first pass left open, one of which turned up a nuance worth pinning: - A dialog lets go of the stack the moment it closes, rather than when it finishes leaving. How that overlaps the leave transition is not observable in a unit test, since happy-dom runs no CSS and Vue resolves the leave at once, so the test says only what it can see. - Escape and dismissal stay apart: neither fires the other's event. - A refusing dialog keeps taking the gesture instead of passing it to the one underneath. `persistent` is the supported way to refuse: a consumer that simply declines to write `modelValue` back does not keep its place, because `defineModel` sets its own copy first and the registration follows that. - A bottom sheet hands its consumer the dismissal, which reaches it through attribute fallthrough rather than a declared emit, and an e2e drives a real back gesture at one. Coverage of the new code is complete: overlay-stack.ts is at 100% of statements, branches and functions, and RuiDialog.vue leaves only the `@after-leave` hook, which predates this and cannot fire without a real transition.
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.
Closes #579.
A
RuiDialogis not a history entry, so a back gesture pops the entry underneath it and leaves the page it was sitting on; with dialogs stacked, one press skips all of them. A consumer can keep its own registry, but every dialog has to opt in by hand and each new one silently regresses.The API
RuiDialogregisters itself while open, andRuiBottomSheetcomes along because it renders one.What the contract has to get right
All three of these come from the app-side version and are covered by tests:
dismissToplooks for the topmost one still showing. Removing on dismissal deregisters a dialog still covering the page, and the next gesture sails past it.modelValuealone, neverpersistent. A dialog that turns persistent partway through its life has to stay in the stack and refuse, or the gesture starts passing through exactly once there is work to lose.dismissTopreports whether it handled the gesture, and a layer that refuses still counts as handled.Where this departs from the issue, and why
hasOverlay= anything registeredcloseTopdismissTopRuiDialogandRuiBottomSheetRuiDialogonlyRuiBottomSheetrenders aRuiDialog, so doing both puts every sheet in the stack twice.zIndexRuiDialog.vue:38defaultszIndexto 9999 for every dialog, so it says nothing about which is on top.Two additions the issue didn't call for: a
dismissemit, so the gesture is observable without reusingclick:escand lying about where it came from; andresetOverlayStack(), which the unit project needs because its pool shares one module registry across the files in a worker.Telling a pop from a push stays out of the library
As the issue argues, the router wiring is the consumer's business. The example app shows it, and the interesting part is what does not work: a raw
popstatelistener double-fires, because vue-router's restoration of an aborted pop is itself a pop and reads as a second gesture — which closed two dialogs per press when I first wired it that way. Taking the direction from vue-router's own history listener is what works:Verified
Lint (0 errors), both typecheck projects, 1429 unit tests (16 new across two specs), 468 storybook tests, 388 e2e tests,
build:prod.The e2e drives real
goBack()gestures on a new/overlay-stackpage: nested dialogs come down one at a time before the page is left, a persistent dialog swallows two gestures and refuses both, and a link inside a dialog still navigates.