Skip to content

[fix] Modal: make a modal active when it shows, not when its animation ends - #2904

Open
ilazaridis wants to merge 1 commit into
necolas:masterfrom
ilazaridis:fix/modal-active-when-visible
Open

ilazaridis wants to merge 1 commit into
necolas:masterfrom
ilazaridis:fix/modal-active-when-visible

Conversation

@ilazaridis

Copy link
Copy Markdown

A modal joins the stack of active modals in onShow. An animated modal calls onShow only when its animation ends, 250 ms after it shows. Until then the modal below stays active:

  • it keeps role="dialog";
  • it handles Escape;
  • its focus trap moves any focus outside it back to its own first focusable element.

That last point includes focus moving inside the new modal. So with a modal opening over another, focus can't move inside the new one until it has finished animating:

  • a Tab, or a tap on a control in it, ends on the lower modal's first focusable element;
  • a programmatic focus() on mount (for example, focusing a search field) does the same.

Then, once the new modal becomes active, its own trap finds focus outside it and moves it to its first focusable element.

We hit this with a picker sheet (animationType="fade") opened from a filter sheet: its search field couldn't take focus when it opened, and a Tab right after it opened went to the filter sheet.

Change

A modal joins the stack in a layout effect when it becomes visible (through modules/useLayoutEffect, so SSR still uses useEffect), not in onShow.

  • The state updates that make the new modal active and the one below inactive are sync updates. React flushes them at the end of that commit, after the commit's passive effects. So the trap still records the element to refocus on close before the new trap moves focus, and a focus() queued on mount (in a microtask) lands inside the new, active modal.
  • onShow still fires when the animation ends.
  • Closing doesn't change: a modal stays active until it is dismissed.

Layout effects run children first. So a modal rendered inside another, with both shown in the same commit, would join the stack first, and the outer one would end up on top. A context gives each modal the id of the modal it is rendered in, and a modal is inserted below the modals inside it. The existing test escape key fires onRequestClose for top modal only with animation covers this case. It fails without the context and passes with it.

Tests

New tests:

  • a modal is active before its animation ends
  • focus moved into a modal opening over another stays there
  • escape goes to a modal opening over another before its animation ends
  • a modal shown inside another in the same commit stays on top

All four fail on master and pass with the change. The existing Modal tests pass unchanged.

  • npm run flow: no errors.
  • npm run format and npm run lint pass.
  • Unit tests: DOM suite 719 passed, 6 skipped; Node suite 6 passed (jest --config ./configs/jest.config{,.node}.js --runInBand).

We run the same change as a patch on 0.21.3, with React 19.2, together with #2879. Our end-to-end suite passes with it in Chromium and WebKit. It covers nested sheets, focus trapping, Tab cycling, Escape, focus return and drag-to-close. A test that holds the upper modal's animation fails without the change: the modal below stays the dialog, and a Tab goes to it.

This is independent of #2879 (trap and Escape as layout effects), and either can land first.

…n ends

A modal joined the stack of active modals in onShow, which an animated
modal calls once its animation ends. Until then the modal below stayed
active: it kept role="dialog", handled Escape, and its focus trap moved
any focus outside it, including focus moving inside the new modal, back
to its own first focusable element. A Tab, a tap, or a programmatic
focus inside a modal opening over another went to the modal below.

A modal now joins the stack in a layout effect when it becomes visible.
Layout effects run children first, so a modal rendered inside another
and shown in the same commit is added first; a context gives each modal
the modal it is rendered in, and a modal is inserted below the modals
inside it, so the inner one stays on top. onShow still fires once the
animation ends.
@codesandbox-ci

Copy link
Copy Markdown

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit bed61f7:

Sandbox Source
react-native-web-examples Configuration

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant