fix(Modal): Focus primary actionable button on modal opening

[emyarod]

Closes carbon-design-system/carbon-components-react#827

This PR fixes the issue where the primary actionable element is not focused when a Modal component is opened. To accomplish this, refs are attached to the Modal and Button components so that the primary actionable element can be programmatically referenced and focused.

Changelog

Changed

• Add support for refs in both the Modal and Button components. This will allow for autofocusing of input elements on render.
• Convert Button from a stateless functional component to a class component in order to support refs

There are 2 options to solve this issue, and I would like to gather some more opinions about this implementation:

One method would be to added the autoFocus attribute to the primary button or the modalButton in Modal.js depending on whether or not the modal is passive. This may be detrimental to a11y and usability though, since the linter will show a warning if autoFocus is used (as mentioned in the ticket).

Another method (currently implemented in this PR) is to convert the Button component from a stateless functional component to a class component, so that we can attach a ref and call the focus method when the Modal component is mounted.

[asudoh]

Thank you for jumping in @emyarod! A couple of things I see are:

• I guess we don't focus on the button in closed state. So cDU (detecting closed to open prop change) instead of cDM?
• Instead of backing in the focus feature to button, I think we can add a DOM node ref callback there

[emyarod]

@asudoh I had some misunderstandings on how to instantiate the modal. I thought the modals were created and destroyed on open and close, and I was unaware of the ModalWrapper component. After taking a look at the ModalWrapper I understand why you are suggesting componentDidUpdate over componentDidMount.

One thing I ran into was the timing of CSS transitions affecting whether or not elements could be focused (or affecting the order in which the elements are focused, I'm not sure). To get around this I naively threw a setTimeout around the focusButton method in the Modal, but I'm not sure if there is a more elegant solution for this.

[emyarod]

The diff for this PR should now be greatly reduced. I've refactored my changes so that I am no longer converting Button.js from a stateless function to a class component. I am keeping the Button component as a stateless functional component while still maintaining a ref to the DOM to focus the button when it is visible

[asudoh]

Looks much better! - Wrt your question wrt animation, I’d suggest you use the same logic as our vanilla counterpart. Thanks @emyarod!

[emyarod]

Thank you for the tip @asudoh. Since I am now listening to the onTransitionEnd event, I no longer need to use component lifecycle methods or timeouts to call focus() on a button. I check the name of the property that is transitioning to avoid calling focus() an unnecessary number of times after the onTransitionEnd event fires. If the property name is opacity or visibility, then I know the modal is visible and the elements within it can be focused.

Please let me know what you think about my latest changes. If this is an acceptable solution, we may be able to use the same approach to solve #991 actually this may require a more substantial refactoring

[asudoh]

Hi @emyarod thank you for trying to deal with the animation stuff! Fiddling with the code revealed that the code is capturing some events bubbled from inner elements. Also, using event details for determining if the modal is visible (and if the button is focusable), especially not looking at what the value is transitioned to, sounds a bit brittle to our possible future changes to modal animation. So I'd recommend more direct check if the modal is actually visible or not, like what our vanilla code does. Thanks!

[emyarod]

@asudoh thanks for your help and explanations. Please let me know what you think about the latest changes!

[asudoh]

Looks great - Thanks @emyarod for the update! Just a nitpick (and the last comment) - buttonIsFocusable name sounds more of offsetHeight thing - Is it possible to change it to something else, like beingOpen as we talked, or beingFocused? Thanks a lot!

[emyarod]

understood @asudoh, I've gone ahead and made that change!

[asudoh]

Thanks for making the update @emyarod! 👍

[marijohannessen]

👍 ✅

[carbon-bot]

🎉 This PR is included in version 6.5.3 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀