Modal: fix focus on close from complex table patterns #2825
Conversation
🦋 Changeset detectedLatest commit: 7e5c73d The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
|
Size Change: +50 B (+0.05%) Total Size: 109 kB
ℹ️ View Unchanged
|
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (8920852) and published all packages with changesets to npm. You can install the packages in ./dev/tools/deploy_wonder_blocks.js --tag="PR2825"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2825 |
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-kvtokmarwc.chromatic.com/ Chromatic results:
|
| }, | ||
| }; | ||
|
|
||
| export const WithOpenedTrue = () => { |
There was a problem hiding this comment.
It would be helpful to include the purpose of the story in the description, or we could make the story name more specific!
There was a problem hiding this comment.
I added a description! I think the OpenedTrue part is helpful to see when you're working with these modals.
| // Use the scheduler to ensure testability | ||
| schedule.animationFrame(() => focusElement.focus(), { | ||
| schedulePolicy: SchedulePolicy.Immediately, | ||
| clearPolicy: ClearPolicy.Resolve, |
There was a problem hiding this comment.
Interesting, I haven't worked with our timing utilities yet, so this is good to know!
Looking into it, the default schedulePolicy for animationFrame is ClearPolicy.Cancel. Does that mean setting it to ClearPolicy.Resolve is what addresses the bug?
There was a problem hiding this comment.
Apparently so! The clearPolicy change is the critical line to making the scheduler fire for some reason. Without it, the code fails silently. The schedulePolicy doesn't seem to have an effect.
marcysutton
force-pushed
the
dialog-upgrades
branch
from
2b3ef88 to
f62979c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dialog-upgrades #2825 +/- ##
=======================================
=======================================
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
…launched from tables
| It specifically tests the case where a modal starts with opened=true and | ||
| needs to return focus to the last focused element when closed. | ||
| */ | ||
| export const ControlledModalFocusTest: StoryComponentType = () => { |
There was a problem hiding this comment.
This one is failing to work now (I thought I remembered it working, but maybe not?). I'll play with it, but nothing is logged at all when the state to control the dialog is set to false....I suspect it's because the ModalLauncher isn't rendered at all after it gets closed, so there is no logging or focus anymore at that point.
Summary:
There was an issue with ModalLauncher not returning focus when called from inside of a table. It was hard to reproduce, so it took a while to find the problem. I finally narrowed it down to the scheduler not quite working properly for this scenario. I added a story reproduction that will show it working before the change and fixed after.
A real page where this can be reproduced: https://classroom.khanacademy.dev/teacher/class/HQV5EE72/reports/assignments
Issue: WB-2103
Test plan:
/?path=/story/packages-modal-modallauncher--focus-management-pattern