Merge pull request #3466 from overleaf/ta-accessible-modal

Fix Aria-Hidden Modals

GitOrigin-RevId: bcce61104220ebcf04e9c348d9f3ab84bff8804a

services/web/frontend/js/features/file-tree/components/modals/file-tree-modal-create-folder.js

@@ -5,6 +5,8 @@ import { Button, Modal } from 'react-bootstrap'
 import { useTranslation } from 'react-i18next'
 import { useRefWithAutoFocus } from '../../../../infrastructure/auto-focus'
 
+import AccessibleModal from '../../../../shared/components/accessible-modal'
+
 import { useFileTreeActionable } from '../../contexts/file-tree-actionable'
 
 import { DuplicateFilenameError } from '../../errors'
@@ -44,7 +46,7 @@ function FileTreeModalCreateFolder() {
   }
 
   return (
-    <Modal show={isCreatingFolder} onHide={handleHide}>
+    <AccessibleModal show={isCreatingFolder} onHide={handleHide}>
       <Modal.Header>
         <Modal.Title>{t('new_folder')}</Modal.Title>
       </Modal.Header>
@@ -95,7 +97,7 @@ function FileTreeModalCreateFolder() {
           </>
         )}
       </Modal.Footer>
-    </Modal>
+    </AccessibleModal>
   )
 }
 

services/web/frontend/js/features/file-tree/components/modals/file-tree-modal-delete.js

@@ -2,6 +2,8 @@ import React from 'react'
 import { Button, Modal } from 'react-bootstrap'
 import { useTranslation } from 'react-i18next'
 
+import AccessibleModal from '../../../../shared/components/accessible-modal'
+
 import { useFileTreeActionable } from '../../contexts/file-tree-actionable'
 
 function FileTreeModalDelete() {
@@ -27,7 +29,7 @@ function FileTreeModalDelete() {
   }
 
   return (
-    <Modal show={isDeleting} onHide={handleHide}>
+    <AccessibleModal show={isDeleting} onHide={handleHide}>
       <Modal.Header>
         <Modal.Title>{t('delete')}</Modal.Title>
       </Modal.Header>
@@ -60,7 +62,7 @@ function FileTreeModalDelete() {
           </>
         )}
       </Modal.Footer>
-    </Modal>
+    </AccessibleModal>
   )
 }
 

services/web/frontend/js/shared/components/accessible-modal.js

@@ -0,0 +1,36 @@
+import React, { useCallback } from 'react'
+import PropTypes from 'prop-types'
+import { Modal } from 'react-bootstrap'
+
+// a bootstrap Modal with its `aria-hidden` attribute removed. Visisble modals
+// should not have their `aria-hidden` attribute set but that's a bug in our
+// version of react-bootstrap.
+function AccessibleModal({ show, ...otherProps }) {
+  // use a callback ref to track the modal. This will re-run the function
+  // when the element node or any of the dependencies are updated
+  const setModalRef = useCallback(
+    element => {
+      if (!element) return
+
+      const modalNode = element._modal && element._modal.modalNode
+      if (!modalNode) return
+
+      if (show) {
+        modalNode.removeAttribute('aria-hidden')
+      } else {
+        modalNode.setAttribute('aria-hidden', 'true')
+      }
+    },
+    // `show` is necessary as a dependency, but eslint thinks it is not
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+    [show]
+  )
+
+  return <Modal show={show} {...otherProps} ref={setModalRef} />
+}
+
+AccessibleModal.propTypes = {
+  show: PropTypes.bool
+}
+
+export default AccessibleModal

services/web/test/frontend/features/file-tree/flows/create-folder.test.js

@@ -1,7 +1,7 @@
 import { expect } from 'chai'
 import React from 'react'
 import sinon from 'sinon'
-import { screen, render, fireEvent } from '@testing-library/react'
+import { screen, render, fireEvent, waitFor } from '@testing-library/react'
 import fetchMock from 'fetch-mock'
 import MockedSocket from 'socket.io-mock'
 
@@ -56,7 +56,7 @@ describe('FileTree Create Folder Flow', function() {
     }
     fetchMock.post(matcher, response)
 
-    fireCreateFolder(newFolderName)
+    await fireCreateFolder(newFolderName)
 
     const lastCallBody = JSON.parse(fetchMock.lastCall(matcher)[1].body)
     expect(lastCallBody.name).to.equal(newFolderName)
@@ -114,7 +114,7 @@ describe('FileTree Create Folder Flow', function() {
     }
     fetchMock.post(matcher, response)
 
-    fireCreateFolder(newFolderName)
+    await fireCreateFolder(newFolderName)
 
     const lastCallBody = JSON.parse(fetchMock.lastCall(matcher)[1].body)
     expect(lastCallBody.name).to.equal(newFolderName)
@@ -178,7 +178,7 @@ describe('FileTree Create Folder Flow', function() {
     }
     fetchMock.post(matcher, response)
 
-    fireCreateFolder(newFolderName)
+    await fireCreateFolder(newFolderName)
 
     const lastCallBody = JSON.parse(fetchMock.lastCall(matcher)[1].body)
     expect(lastCallBody.name).to.equal(newFolderName)
@@ -239,7 +239,7 @@ describe('FileTree Create Folder Flow', function() {
     })
   })
 
-  function fireCreateFolder(name) {
+  async function fireCreateFolder(name) {
     const createFolderButton = screen.getByRole('button', {
       name: 'New Folder'
     })
@@ -247,14 +247,12 @@ describe('FileTree Create Folder Flow', function() {
 
     setFolderName(name)
 
-    const modalCreateButton = getModalCreateButton()
+    const modalCreateButton = await getModalCreateButton()
     fireEvent.click(modalCreateButton)
   }
 
   function setFolderName(name) {
-    const input = screen.getByRole('textbox', {
-      hidden: true // FIXME: modal should not be hidden but it has the aria-hidden label due to a react-bootstrap bug
-    })
+    const input = screen.getByRole('textbox')
     fireEvent.change(input, { target: { value: name } })
   }
 
@@ -264,10 +262,7 @@ describe('FileTree Create Folder Flow', function() {
       .replace(/0\./, 'random-test-id-')
   }
 
-  function getModalCreateButton() {
-    return screen.getAllByRole('button', {
-      name: 'Create',
-      hidden: true // FIXME: modal should not be hidden but it has the aria-hidden label due to a react-bootstrap bug
-    })[0] // the first matched button is the toolbar button
+  async function getModalCreateButton() {
+    return waitFor(() => screen.getByRole('button', { name: 'Create' }))
   }
 })

services/web/test/frontend/features/file-tree/flows/delete-entity.test.js

@@ -55,7 +55,7 @@ describe('FileTree Delete Entity Flow', function() {
       const fetchMatcher = /\/project\/\w+\/doc\/\w+/
       fetchMock.delete(fetchMatcher, 204)
 
-      const modalDeleteButton = getModalDeleteButton()
+      const modalDeleteButton = await getModalDeleteButton()
       fireEvent.click(modalDeleteButton)
 
       window._ide.socket.socketClient.emit('removeEntity', '456def')
@@ -82,7 +82,7 @@ describe('FileTree Delete Entity Flow', function() {
     it('continues delete on 404s', async function() {
       fetchMock.delete(/\/project\/\w+\/doc\/\w+/, 404)
 
-      const modalDeleteButton = getModalDeleteButton()
+      const modalDeleteButton = await getModalDeleteButton()
       fireEvent.click(modalDeleteButton)
 
       window._ide.socket.socketClient.emit('removeEntity', '456def')
@@ -107,7 +107,7 @@ describe('FileTree Delete Entity Flow', function() {
       const fetchMatcher = /\/project\/\w+\/doc\/\w+/
       fetchMock.delete(fetchMatcher, 500)
 
-      const modalDeleteButton = getModalDeleteButton()
+      const modalDeleteButton = await getModalDeleteButton()
       fireEvent.click(modalDeleteButton)
 
       // The modal should still be open, but the file should not be deleted
@@ -150,7 +150,7 @@ describe('FileTree Delete Entity Flow', function() {
       const fetchMatcher = /\/project\/\w+\/(doc|file)\/\w+/
       fetchMock.delete(fetchMatcher, 204)
 
-      const modalDeleteButton = getModalDeleteButton()
+      const modalDeleteButton = await getModalDeleteButton()
       fireEvent.click(modalDeleteButton)
 
       window._ide.socket.socketClient.emit('removeEntity', '456def')
@@ -181,10 +181,7 @@ describe('FileTree Delete Entity Flow', function() {
     })
   })
 
-  function getModalDeleteButton() {
-    return screen.getAllByRole('button', {
-      name: 'Delete',
-      hidden: true // FIXME: modal should not be hidden but it has the aria-hidden label due to a react-bootstrap bug
-    })[1] // the first matched button is the toolbar button
+  async function getModalDeleteButton() {
+    return waitFor(() => screen.getByRole('button', { name: 'Delete' }))
   }
 })