fix(ui-modal,ui-dom-utils): fix Modal focus trap broken when it has a scrollbar

The issue was that its not recognized by findFocusable as a focusable element. Recently browsers
have made scrollable elements focusable, but there is no sure way to know that a scrollbar is
displayed, an easy workaround is to add the tabIndex=0 prop. Note that this breaks VoiceOver a bit,
its not able to navigate the inside of a scrollable container with SR focus

INSTUI-4625

Author: matyasf

packages/ui-dom-utils/src/findFocusable.ts

@@ -33,7 +33,10 @@
  *
  * http://api.jqueryui.com/category/ui-core/
  **/
-
+// TODO replace this with https://github.com/focus-trap/tabbable
+// or even better use the native <dialog> component.
+// tabbable has issues with scrollable containers, e.g.
+// https://github.com/focus-trap/tabbable/issues/167
 import { getComputedStyle, findDOMNode } from './'
 import type { UIElement } from '@instructure/shared-types'
 
@@ -50,24 +53,27 @@ const focusableSelector = [
   '[contenteditable="true"]'
 ].join(',')
 
+function hasQuerySelectorAll(el?: UIElement): el is Element | Document {
+  if (
+    !el ||
+    typeof (el as Element | Document).querySelectorAll !== 'function'
+  ) {
+    return false
+  }
+  return true
+}
+
 function findFocusable(
   el?: UIElement,
   filter?: (el: Element) => boolean,
   shouldSearchRootNode?: boolean
 ) {
   const element = el && findDOMNode(el)
 
-  if (
-    !element ||
-    typeof (element as Element | Document).querySelectorAll !== 'function'
-  ) {
+  if (!hasQuerySelectorAll(element)) {
     return []
   }
-
-  let matches = Array.from(
-    (element as Element | Document).querySelectorAll(focusableSelector)
-  )
-
+  let matches = Array.from(element.querySelectorAll(focusableSelector))
   if (shouldSearchRootNode && (element as Element).matches(focusableSelector)) {
     matches = [...matches, element as Element]
   }

packages/ui-modal/src/Modal/ModalBody/index.tsx

@@ -34,7 +34,7 @@ import generateStyle from './styles'
 import generateComponentTheme from './theme'
 
 import { propTypes, allowedProps } from './props'
-import type { ModalBodyProps, ModalBodyState } from './props'
+import type { ModalBodyProps } from './props'
 
 /**
 ---
@@ -44,7 +44,7 @@ id: Modal.Body
 **/
 @withStyle(generateStyle, generateComponentTheme)
 @testable()
-class ModalBody extends Component<ModalBodyProps, ModalBodyState> {
+class ModalBody extends Component<ModalBodyProps> {
   static readonly componentId = 'Modal.Body'
 
   static propTypes = propTypes
@@ -69,10 +69,6 @@ class ModalBody extends Component<ModalBodyProps, ModalBodyState> {
 
   constructor(props: ModalBodyProps) {
     super(props)
-
-    this.state = {
-      isFirefox: false
-    }
   }
 
   componentDidMount() {
@@ -117,11 +113,14 @@ class ModalBody extends Component<ModalBodyProps, ModalBodyState> {
         as={as}
         css={this.props.styles?.modalBody}
         padding={padding}
-        // We have to make an exception in Firefox, because it makes
-        //  the container focusable when it is scrollable.
-        //  This is a feature, not a bug, but it prevents VoiceOver
-        //  to correctly focus inside the body in other browsers.
-        {...(this.state.isFirefox && { tabIndex: -1 })}
+        // Setting this to 0 is necessary for findFocusable to find this DOM
+        // element.
+        // Note that VoiceOver does not read the contents properly
+        // if there is a scrollbar, and it has no kb just SR focus.
+        // To prevent this we could set tabIndex to -1
+        // but this would make the scrollbar non-focusable, so the modal cannot
+        // be scrolled with keyboard.
+        tabIndex={0}
       >
         {children}
       </View>

packages/ui-modal/src/Modal/ModalBody/props.ts

@@ -56,10 +56,6 @@ type ModalBodyProps = ModalBodyOwnProps &
 
 type ModalBodyStyle = ComponentStyle<'modalBody'>
 
-type ModalBodyState = {
-  isFirefox: boolean
-}
-
 const propTypes: PropValidators<PropKeys> = {
   children: PropTypes.node,
   padding: PropTypes.string,
@@ -81,5 +77,5 @@ const allowedProps: AllowedPropKeys = [
   'overflow'
 ]
 
-export type { ModalBodyProps, ModalBodyState, ModalBodyStyle }
+export type { ModalBodyProps, ModalBodyStyle }
 export { propTypes, allowedProps }