-
Notifications
You must be signed in to change notification settings - Fork 88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WNMGDS-2555] Add isOpen
to Drawer
and Dialog
to fix focus-management issue
#2890
Conversation
I've temporarily refactored `NativeDialog`, `Dialog`, and the dialog stories to use a boolean prop on `Dialog` to toggle the open state rather than conditional rendering of the `Dialog`. Running storybook with `yarn storybook:react`, I'm able to reproduce the bug when closing the dialog with the close button, and after the refactoring, the bug goes away. So there's something here. Same thing happens when I do the same to the `Drawer` and drawer stories. The bug goes away. I don't really want to change the API, so how can I figure out what part of this is relevant and apply it to the old API?
Previously when I was prototyping this, I just replaced the scroll code with garbage so it'd compile. I don't love that not having the effect cleanup function closure means I have to track the `y` state separately, but whatever
document.documentElement.style.setProperty('scroll-behavior', 'auto'); | ||
return () => { | ||
document.body.classList.remove(bodyClass); | ||
window.scrollTo({ top: y, behavior: 'auto' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making the state dependent on the value of isOpen
instead of following when it's mounted and unmounted, it meant that it was no longer as simple as a use effect and cleanup. Because I no longer had direct access to the y
value created in the top of the use effect in a closure, I had to start tracking that state separately. I didn't love that, so you'll notice that I ended up moving this all into a separate module and used a reducer, which I ended up trying to model as a finite state machine. It might be a little more code than is needed, but I feel more confident in it being a finite state machine, especially since I've been able to leverage TypeScript to help make sure my logic is correct.
name: 'closed'; | ||
} | ||
|
||
const OPEN = (_state: ClosedState): OpenState => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This OPEN
action requires a starting point of ClosedState
and an ending point of OpenState
.
// Show or hide the dialog based on `isOpen` value. The `dialogNode.open` property is | ||
// a read-only value that will tell us if our dialog DOM element is actually in the | ||
// open state. | ||
if (isOpen) { | ||
if (!dialogNode.open) { | ||
showModal ? dialogNode.showModal() : dialogNode.show(); | ||
} | ||
} else { | ||
if (dialogNode.open) { | ||
dialogNode.close(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because some of this state comes from the DOM, it didn't lend itself as well to modeling as a finite state machine. I didn't want to duplicate that state and make it way more verbose. Don't @ me. Or do, because this is a review.
And I actually think we can release these now that this bug is fixed
When I ran this in the `DrawerManager` story (which has multiple dialogs), I found that when I opened a new drawer when another one was open, the dialogs were closing only in the cleanup functions. Then I realized that the cleanup functions were being called on dialogs that should be open, which was a silly mistake of mine. I'm always closing in the cleanup function when, in fact, it will execute the cleanup function for more reasons than just de-rendering the component. It can also execute the cleanup function just because isOpen changed!
…es bug) I originally thought that we needed to make sure it closes if the component ever completely de-renders and is removed from the DOM for good, but removing it from the DOM will also close the dialog, just without the callbacks for close, which we don't want anyway
It might not work just because of how jsdom handles dialogs. That's my theory anyway. I think it just doesn't have the focus-managing functionality
@zarahzachz, the tests are finally passing. This is ready for real review. |
Summary
What I think has been happening is that React removes the
Dialog
andDrawer
from the DOM before the browser has a chance to fully execute its close cycle and transfer focus to the element that had focus before the<dialog>
underlying was opened. By always having theDialog
orDrawer
(internallyNativeDialog
) rendered by its parent element and passing anisOpen
prop to control its state, the browser is able to successfully transfer focus back to the right place after the dialog is closed.For temporary backwards compatibility during a deprecation period, you can still conditionally render the dialog, and it will behave as it did before, but it will warn you that you need a
isOpen
prop. This is slated to go out in v9, so we'll fully deprecate the old behavior in v10.I was curious if other React libraries conditionally rendered or used a boolean prop for toggling state. What I found was that Carbon, Ant, Forma 36, Helsinki, Polaris, and Primer use a boolean prop instead of conditionally rendering the modal to show and hide it. Gestalt uses conditional rendering, but their focus doesn't automatically return to the triggering button. The
react-aria
library uses modal trigger components like we were originally thinking we'd have to do (kind of an ugly solution to all this that would require more work for teams). All that to say, we're not weird for adopting this pattern.isOpen
prop toDialog
andDrawer
How to test
Use
yarn storybook:react
to reproduce the original problem, which only happens in React. If you open a dialog or drawer and then close it, focus won't move back to the button that opened it.How to migrate code
Previously applications would need to conditionally render dialogs and drawers like this:
With the new API, you always render the dialog or drawer, but the condition (a boolean) goes in the
isOpen
prop like this:Please note that if you had code effects that ran when the dialog rendered, you'll need to address any possible assumptions that rendering the dialog is the same as the dialog being open. For instance, if you previously called an external API to fetch information whenever your dialog was first rendered, you might see that API call happening when the page loads instead of waiting until the dialog is open. In those cases, you probably want to start looking at the
isOpen
prop before deciding to call your API.Checklist
[WNMGDS-####] Title
or [NO-TICKET] if this is unticketed work.Type
(only one) label for this PR, if it is a breaking change, label should only beType: Breaking
Impacts
, multiple can be selected.If this is a change to code:
yarn test:unit:update
) and browser-test snapshots (yarn test:browser:update
)