Reapply removal of findDOMNode from Modal and Popover, remove autofocus from Modal

[ravicious]

In order to remove findDOMNode usage from our project, I refactored the Modal and Popover components in #46123. But I had to quickly revert it in #46398 because certain modals would go into an infinite loop, which broke the enterprise tests.

What was wrong

The new code would go into an infinite loop calling focus on the child of <Modal>. Grzegorz and Bartosz helped me pin down the issue to a situation where at least two modals are shown on the screen. We attributed this to document.addEventListener('focus', this.enforceFocus, true) and modals fighting for focus.

But upon looking at the old Modal implementation, I came to the conclusion that in theory the logic was the same. The JSDocs for relevant props said this:

  /**
   * If `true`, the modal will not automatically shift focus to itself when it opens, and
   * replace it to the last focused element when it closes.
   * This also works correctly with any modal children that have the `disableAutoFocus` prop.
   *
   * Generally this should never be set to `true` as it makes the modal less
   * accessible to assistive technologies, like screen readers.
   */
  disableAutoFocus: PropTypes.bool,

  /**
   * If `true`, the modal will not prevent focus from leaving the modal while open.
   *
   * Generally this should never be set to `true` as it makes the modal less
   * accessible to assistive technologies, like screen readers.
   */
  disableEnforceFocus: PropTypes.bool,

If you consider those comments at face value, you'll realize that modals in our app cannot work this way. There are some places in the UI which show two modals at once. For example, the failing NotificationRoutingRules story shows a list of rules, which is a modal in itself. After you click "View" on a rule and then the trash icon in the top right, it shows another modal for confirmation. If both are shown at the same time, which modal is supposed to keep focus? How do you make sure that one modal doesn't steal the focus of another? FWIW, we've run into a similar problem in the past, see pauseUserInteraction.

The new code would go into an infinite loop whenever you clicked on the trash icon. This made me suspect that the old code never worked properly when it came to enforcing focus. I added some console logs and I could see that it does call .focus() on the modal child, but it actually didn't cause document.activeElement to change.

The actual broken code

It turns out that indeed the autofocus behavior and focus enforcement behavior never worked properly. Calling .focus() on an element doesn't do anything if the element itself isn't focusable. To work around this, Modal's autoFocus method would add the tabIndex attribute to the modal child if it didn't already have it. The problem is, this method would never get called.

autoFocus is called from handleOpened, which is called from handleOpen only if this.dialogRef is present. handleOpen is called from componentDidMount and componentDidUpdate if the open prop is true. After adding console logs to those methods, I realized that this.dialogRef is never ever present during the invocation of those lifecycle methods. See the following video where I interact with various modals and this.dialogRef is never present, hence handleOpened and thus autoFocus is never called.

handleOpen-early-return.mov

The old code used a hand-crafted callback ref through a hand rolled RootRef component which set dialogRef during RootRef's componentDidMount and componentDidUpdate methods.

teleport/web/packages/design/src/Modal/Modal.jsx

Lines 198 to 211 in db689c7

	<StyledModal
	modalCss={modalCss}
	data-testid="Modal"
	ref={this.handleModalRef}
	className={className}
	onClick={e => e.stopPropagation()}
	>
	{!hideBackdrop && (
	<Backdrop onClick={this.handleBackdropClick} {...BackdropProps} />
	)}
	<RootRef rootRef={this.onRootRef}>
	{React.cloneElement(children, childProps)}
	</RootRef>
	</StyledModal>

The new code changed it to a normal ref that's set on the div above the modal's child. This made it so that the ref was present during Modal's componentDidMount and componentDidUpdate methods, so autoFocus was executed and it set the tabIndex attribute on the modal's child. This later caused two modals to fight for focus because not this.dialogEl().focus() actually changed focus, this.dialogEl() became a focusable element.

The solution

I decided to completely remove the broken code. The general behavior it was supposed to establish was actually desirable. I remember that in Connect we often add autoFocus to fields in modals, because otherwise after opening the modal you'd have to click on the field to begin typing. It also indeed shouldn't be possible to let focus escape the modal.

However, if we want to be able to show multiple modals at the same time, as we already do, then the implementation must take that into account. Taking care of this would require more time than I have at the moment, which is why I'm deleting the broken implementation instead of replacing it.

The only situation where the autofocus implementation would actually work is if the child passed to <Modal> was a focusable element, either a div with tabindex or an inherently focusable element like a button for example. But I looked through the code place and there doesn't appear to be a single place where we do this.