Skip to content

feat(portal)!: dismiss only the topmost modal on back or Escape - #5127

Open
konstmar wants to merge 12 commits into
callstack:mainfrom
konstmar:overlay-dismiss-stack
Open

konstmar wants to merge 12 commits into
callstack:mainfrom
konstmar:overlay-dismiss-stack

Conversation

@konstmar

@konstmar konstmar commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

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 a modal portal gets it.

  • Each Portal.Host asks only its topmost modal portal to close.
  • Modal and Dialog are the first to use it. Menu will adopt it separately, in another PR.
  • A Modal stops being the topmost modal as soon as it starts closing.

Breaking changes

  • Modal's dismissable now controls the back button, Escape, the screen reader's escape gesture and the hidden dismiss button. dismissable={false} blocks every way of closing.
  • Modal's dismissableBackButton prop is removed.
  • Modal's new dismissableOverlay prop controls an outside tap, and applies only when the modal is dismissable.

The migration guide is updated.

Related issue

Notion

Screenshots / Videos

No visual change.

Test plan

yarn test covers the hook (ranking, one press per overlay, absorbed presses, Escape) and a back-press test on Modal.

Konstantin Marushchak added 3 commits September 15, 2026 10:07
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.
@github-actions

github-actions Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Found potential problems with the pull request:

  • The description is too long. Please keep it under 1000 characters.
  • Screenshot or video evidence is missing. Make sure to include one if it affects the UI.

Konstantin Marushchak added 5 commits September 17, 2026 15:17
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.
# 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
Comment thread src/utils/useOverlayDismiss.tsx Outdated
import { addEventListener } from './addEventListener';
import { BackHandler } from './BackHandler/BackHandler';

const visibleOverlays: Array<number> = [];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this approach. The order of modals is already in the portal manager, so duplicating it here means 2 sources of truth and possible mismatches.

It may make sense to expose this information from portal via context, or move this logic to portal manager.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic moved to Portal Manager

Comment thread src/utils/useOverlayDismiss.tsx Outdated
Comment on lines +110 to +113
event.stopImmediatePropagation();
};

document.addEventListener('keydown', handleKeyDown, true);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude says (please verify):

useOverlayDismiss.tsx:113 listens for keydown on document in the capture phase, then calls stopImmediatePropagation() at line 110.

A capture listener on document runs before any handler inside the modal. So the defaultPrevented check at line 101 never sees what closer handlers did, and the comment above it is wrong.
Stopping the event in the capture phase also means it never reaches its target. React's root listeners never get it either. So while a modal is open, no onKeyDown or onKeyPress inside the app receives Escape.
The test "stays out of the way once something nearer the key press handled it" hides this because it builds the event with defaultPrevented: true already set.
Fix: drop the true so the listener runs in the bubble phase.

Comment thread src/utils/useOverlayDismiss.tsx Outdated
/**
* Closes only the overlay on top when the user presses back or Escape.
*/
export function useOverlayDismiss({

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Menu uses portal too, but since this logic is only used in Modal, if both menu and modal are open, both will close. Probably an argument to centralize this in portal manager.

@konstmar konstmar Oct 5, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's expected, actually. Menu is out of scope and would have to adapt useOverlayDismiss in a separate task.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's expected, actually. Menu is out of scope

not quite. the problem i'm highlighting is that this is not a general solution. what about users who build their own components using Portal? how would they integrate with an internal hook?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. It makes sense to create a general solution that Paper's component can adopt, but also users can adopt in their own components.

Have not updated Menu to keep this PR small enough. If you are fine with the current solution, I will add another PR that updates Menu as well. Unless you would like to add Menu's changes in this PR instead

Comment thread src/components/Modal.tsx Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now also controls escape key, so the description is wrong. The name would also be misleading. Though maybe only dismissable should control escape key. Not sure.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dismissable now controls back button, escape, and screen reader gestures. That renders dismissableBackButton useless, because escape key and screen reader gestures on Android go through back button press handles. That's why I removed it, but added dismissableOverlay to control overlay/backdrop tap

Comment on lines 175 to 219
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>
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Portal.Host wrappers are unnecessary around dialog actions.

Comment thread src/components/__tests__/Modal.test.tsx Outdated
Comment on lines +52 to +54
beforeEach(() => {
BackHandler.exitApp.mockClear();
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to restore/clear all mocks after each test so we don't need to keep track of individual mocks

Suggested change
beforeEach(() => {
BackHandler.exitApp.mockClear();
});
afterEach(() => {
jest.restoreAllMocks();
});

Comment thread src/components/__tests__/Modal.test.tsx Outdated
expect(onDismiss).not.toHaveBeenCalled();
});

it('absorbs the Android back button for a non-dismissible modal', async () => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"absorbs" is strange wording

Suggested change
it('absorbs the Android back button for a non-dismissible modal', async () => {
it("doesn't handle the Android back button for a non-dismissible modal", async () => {

Konstantin Marushchak added 2 commits October 7, 2026 10:07
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 konstmar changed the title feat(modal): dismiss only the topmost overlay on back or escape feat(portal)!: dismiss only the topmost modal on back or Escape Oct 8, 2026
@konstmar
konstmar requested a review from satya164 October 8, 2026 11:34

@satya164 satya164 left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by Claude

See the inline comments. (via Claude, verify if it makes sense)

const listensForKeys = Platform.OS === 'web' && 'document' in global;

if (listensForKeys) {
document.addEventListener('keydown', handleKeyDown);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Comment on lines 26 to +35
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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The types allow states that do not work:

  • <Portal modal> with no onDismiss consumes 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 that onDismiss is needed.
  • onDismiss and dismissable on a portal that is not modal are 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)

Comment on lines +16 to +17
Tapping outside will not close this dialog, however you can still close
it with the back button or the Escape key!

@satya164 satya164 Oct 22, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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 satya164 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by Claude

See the inline comments. (via Claude, verify if it makes sense)

Comment on lines +78 to +82
onAccessibilityEscape={
index === topmostModalIndex && dismissable
? onDismiss
: undefined
}

@satya164 satya164 Oct 22, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.Host inside a modal, with <Portal modal dismissable={false}> in it. The escape gesture goes to the outer modal layer and calls the outer onDismiss, 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:

Suggested change
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)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants