Repository navigation
Conversation
Re-provide `ReduceMotionContext` in `Portal`, alongside the settings, locale and theme contexts already forwarded across the portal boundary, so portal content stops falling back to the context default of `false`.
Compare the key when looking up the queued `mount` to replace, so an update that arrives before the `PortalManager` ref is attached no longer overwrites an unrelated queued portal.
Add an opt-in `overlay` prop to `Portal` that hides every layer below it -- the app content and any portal mounted earlier -- from assistive technology and from the web focus order, while portals mounted on top stay reachable.
|
Found potential problems with the pull request:
|
Address review feedback on callstack#5126: - rename the `overlay` prop to `modal` - rename `PortalManager`'s `pageContent` prop to `children` and make it required, since a portal host doesn't render a page - move the `collapsable` comment onto the prop it explains - rewrite the `modal` prop documentation
A `Modal` is an overlay, so it always needs a `Portal` with `modal` set to hide the content behind it. Render one itself instead of asking every call site to wrap the modal and pass the prop. BREAKING CHANGE: `Modal` and `Dialog` no longer need to be wrapped in a `Portal`.
Every dialog now hides the content behind it, so the dedicated "Inert background" example no longer has anything of its own to show.
`Dialog` renders itself in a `Portal`, so the examples no longer need to wrap it in one.
26f1f8e to
4ce304d
Compare
# Conflicts: # src/components/Modal.tsx # src/components/Portal/PortalHost.tsx # src/components/Portal/PortalManager.tsx # src/components/__tests__/Portal.test.tsx # src/components/__tests__/__snapshots__/Modal.test.tsx.snap
# Conflicts: # src/components/Modal.tsx # src/components/__tests__/Modal.test.tsx
| describe('DialogActions', () => { | ||
| it('renders passed children', async () => { | ||
| await render( | ||
| <Dialog.Actions> | ||
| <Button testID="button-cancel">Cancel</Button> | ||
| <Button testID="button-ok">Ok</Button> | ||
| </Dialog.Actions> | ||
| <Portal.Host> | ||
| <Dialog.Actions> | ||
| <Button testID="button-cancel">Cancel</Button> | ||
| <Button testID="button-ok">Ok</Button> | ||
| </Dialog.Actions> | ||
| </Portal.Host> | ||
| ); | ||
|
|
||
| expect(screen.getByTestId('button-cancel')).toBeOnTheScreen(); | ||
| expect(screen.getByTestId('button-ok')).toBeOnTheScreen(); | ||
| }); | ||
|
|
||
| it('applies default styles', async () => { | ||
| await render( | ||
| <Dialog.Actions testID="dialog-actions"> | ||
| <Button>Cancel</Button> | ||
| <Button>Ok</Button> | ||
| </Dialog.Actions> | ||
| <Portal.Host> | ||
| <Dialog.Actions testID="dialog-actions"> | ||
| <Button>Cancel</Button> | ||
| <Button>Ok</Button> | ||
| </Dialog.Actions> | ||
| </Portal.Host> | ||
| ); | ||
|
|
||
| const dialogActionsContainer = screen.getByTestId('dialog-actions'); | ||
| const dialogActionButtons = dialogActionsContainer.children; | ||
|
|
||
| expect(dialogActionsContainer).toHaveStyle({ | ||
| paddingBottom: 24, | ||
| paddingHorizontal: 24, | ||
| }); | ||
| expect(dialogActionButtons[0]).toHaveStyle({ marginRight: 8 }); | ||
| expect(dialogActionButtons[1]).toHaveStyle({ marginRight: 0 }); | ||
| }); | ||
|
|
||
| it('applies custom styles', async () => { | ||
| await render( | ||
| <Dialog.Actions testID="dialog-actions"> | ||
| <Button style={styles.spacing}>Cancel</Button> | ||
| <Button style={styles.noSpacing}>Ok</Button> | ||
| </Dialog.Actions> | ||
| <Portal.Host> | ||
| <Dialog.Actions testID="dialog-actions"> | ||
| <Button style={styles.spacing}>Cancel</Button> | ||
| <Button style={styles.noSpacing}>Ok</Button> | ||
| </Dialog.Actions> | ||
| </Portal.Host> | ||
| ); |
There was a problem hiding this comment.
Portal.Host wrappers are unnecessary around dialog actions.
Each Portal.Host now closes only its topmost modal portal on the Android back button, the Escape key on web, and the screen reader's escape gesture, replacing the global useOverlayDismiss stack. BREAKING CHANGE: Modal and Dialog drop `dismissableBackButton`. `dismissable` now controls the back button, the Escape key, the screen reader's escape gesture and the dismiss button, and the new `dismissableOverlay` prop controls closing on an outside tap.
konstmar
changed the title
| const listensForKeys = Platform.OS === 'web' && 'document' in global; | ||
|
|
||
| if (listensForKeys) { | ||
| document.addEventListener('keydown', handleKeyDown); |
There was a problem hiding this comment.
Escape and the back button pick different modals when there is more than one Portal.Host.
BackHandler calls the last registered listener first. document calls keydown listeners in the order they were added. Each host has its own topmost modal, so each host has one enabled listener. With a Portal.Host inside a modal:
<Portal.Host>
<Portal modal onDismiss={closeOuter}>
<Portal.Host>
<Portal modal onDismiss={closeInner}>...</Portal>
</Portal.Host>
</Portal>
</Portal.Host>I ran this in a local test. The back button calls closeInner. Escape calls closeOuter, which closes the outer modal and the inner modal on top of it.
A fix is one module level document listener that walks the enabled modals from the last one added to the first, the same order as BackHandler. (via Claude, verify if it makes sense)
| modal?: boolean; | ||
| /** | ||
| * Callback that is called when the user asks to close a `modal` portal. | ||
| * Only the topmost `modal` portal in a `Portal.Host` can be dismissed. | ||
| */ | ||
| onDismiss?: () => void; | ||
| /** | ||
| * Whether the user can dismiss a `modal` portal. | ||
| */ | ||
| dismissable?: boolean; |
There was a problem hiding this comment.
The types allow states that do not work:
<Portal modal>with noonDismissconsumes the Android back button and Escape and does nothing. On Android the user can't close it or go back. The test "doesn't pass the press on from a modal without onDismiss" locks this in, and the docs don't say thatonDismissis needed.onDismissanddismissableon a portal that is notmodalare ignored with no warning.
React Native's Modal has the same problem with onRequestClose on Android. Consider making onDismiss required when modal is set, for example with a union of { modal: boolean; onDismiss: () => void; dismissable?: boolean } and { modal?: false }. At least document that a modal portal without onDismiss blocks the back button. (via Claude, verify if it makes sense)
| Tapping outside will not close this dialog, however you can still close | ||
| it with the back button or the Escape key! |
There was a problem hiding this comment.
The button for this dialog now shows on all platforms. iOS has no back button, and Escape is only handled on web. So on iOS the text points to ways of closing that don't exist.
| Tapping outside will not close this dialog, however you can still close | |
| it with the back button or the Escape key! | |
| Tapping outside will not close this dialog, however you can still close | |
| it with the Android back button or the Escape key on the web! |
(via Claude, verify if it makes sense)
satya164
left a comment
satya164
left a comment
There was a problem hiding this comment.
Review by Claude
See the inline comments. (via Claude, verify if it makes sense)
| onAccessibilityEscape={ | ||
| index === topmostModalIndex && dismissable | ||
| ? onDismiss | ||
| : undefined | ||
| } |
There was a problem hiding this comment.
With dismissable={false}, the topmost modal gets no onAccessibilityEscape, so the VoiceOver escape gesture is not handled here. iOS then passes it on to the next ancestor that handles it. The Android back button and Escape on web are consumed in the same case (PortalDismiss returns true and calls preventDefault), so the platforms behave differently.
Where it breaks:
- A
Portal.Hostinside a modal, with<Portal modal dismissable={false}>in it. The escape gesture goes to the outer modal layer and calls the outeronDismiss, which also removes the inner modal. The Android back button keeps both open.
Always set the handler on the topmost modal and call onDismiss only when it is dismissable, the same as PortalDismiss:
| onAccessibilityEscape={ | |
| index === topmostModalIndex && dismissable | |
| ? onDismiss | |
| : undefined | |
| } | |
| onAccessibilityEscape={ | |
| index === topmostModalIndex | |
| ? () => { | |
| if (dismissable) { | |
| onDismiss?.(); | |
| } | |
| } | |
| : undefined | |
| } |
The Modal snapshots for non-dismissable modals then need an update. (via Claude, verify if it makes sense)
There was a problem hiding this comment.
Thanks for reworking this around PortalManager,@konstmar. Keeping the stack in one place and exposing modal, onDismiss and dismissable on Portal is what the first round asked for. Before the line-by-line pass:
Blockers:
- Rebase now.
src/components/__tests__/Dialog.test.tsxconflicts withmainin its imports: keepmain'simport type { ComponentProps } from 'react';and your narrowedreact-nativeimport. The rest merges cleanly, and CI needs a run on the new head. - Escape and Android back pick different modals with more than one
Portal.Host(the open thread onPortalDismiss.tsx). With aPortal.Hostinside aModalholding a secondModal, back closes the inner one, but Escape closes the outer one and the inner with it:BackHandlerruns the latest listener first,documentthe earliest. One module-levelkeydownlistener that walks the enabled modals from the latest would make them agree, and a test with two nested hosts for both inputs would keep it that way. - On the web, Escape in a Dialog with an open Menu now closes the Dialog (on
mainit closes only the Menu), because the Dialog'skeydownruns before Menu'skeyup. Since this PR adds Escape to Modal and Dialog, please bring the Menu change in here, as you offered in that thread. One way: Menu renders amodalportal withonDismissand drops its own back andkeyuplisteners (untried, so check it against Menu's focus handling). That would also stop Android back from closing both. A Menu-inside-Dialog test for back and Escape would cover it.
Also in this PR:
- VoiceOver escape on a non-dismissable modal (the thread on
PortalManager.tsx): absorb the gesture there, as back and Escape already are, so it doesn't reach an outer modal. modalwithoutonDismiss(the thread onPortal.tsx): it swallows back and Escape, so on Android the user can't leave. Please type it as suggested there, withonDismissrequired whenmodalis set, and say in the JSDoc what a modal portal does with back and Escape.- Migration guide: one sentence that
dismissableBackButton={false}on a dismissable modal has no replacement (on 5.x it had no effect there anyway). - Description: please link #5150 under Related issue instead of the private page.
Doberjohn
added
needs pickup
|
The 6.0 overlay work continues in #5175. Seven of this branch's nine non-merge commits already landed through #5124 and #5126, and #5175 carries your topmost-only dismissal commit from here and your two focus commits from #5129, with your authorship. 6.0 keeps |
Motivation
Some of our overlay components have their own dismiss logic (back on native, Escape on web), and none of them route it to the topmost overlay. This PR moves dismissal logic into
Portal, so any overlay built on amodalportal gets it.Portal.Hostasks only its topmostmodalportal to close.ModalandDialogare the first to use it.Menuwill adopt it separately, in another PR.Modalstops being the topmost modal as soon as it starts closing.Breaking changes
dismissablenow controls the back button, Escape, the screen reader's escape gesture and the hidden dismiss button.dismissable={false}blocks every way of closing.dismissableBackButtonprop is removed.dismissableOverlayprop controls an outside tap, and applies only when the modal isdismissable.The migration guide is updated.
Related issue
Notion
Screenshots / Videos
No visual change.
Test plan
yarn testcovers the hook (ranking, one press per overlay, absorbed presses, Escape) and a back-press test onModal.