From e400d6c6336a66d152dc9f52f42ae8f968f22ad9 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandro.celaya@hypothes.is>
Date: Wed, 31 Jan 2024 16:08:59 +0100
Subject: [PATCH] Use ModalDialog in the notebook

---
 package.json                                  |  2 +-
 src/annotator/components/NotebookModal.tsx    | 51 ++++++++++++++-----
 .../components/test/NotebookModal-test.js     | 23 ++++++++-
 yarn.lock                                     | 10 ++--
 4 files changed, 67 insertions(+), 19 deletions(-)

diff --git a/package.json b/package.json
index cf546e8a679..a3512aa425e 100644
--- a/package.json
+++ b/package.json
@@ -12,7 +12,7 @@
     "@babel/preset-react": "^7.0.0",
     "@babel/preset-typescript": "^7.16.7",
     "@hypothesis/frontend-build": "^3.0.0",
-    "@hypothesis/frontend-shared": "^7.2.0",
+    "@hypothesis/frontend-shared": "^7.3.0",
     "@hypothesis/frontend-testing": "^1.2.0",
     "@npmcli/arborist": "^7.0.0",
     "@octokit/rest": "^20.0.1",
diff --git a/src/annotator/components/NotebookModal.tsx b/src/annotator/components/NotebookModal.tsx
index d8997acadd9..23e6ec5091e 100644
--- a/src/annotator/components/NotebookModal.tsx
+++ b/src/annotator/components/NotebookModal.tsx
@@ -1,6 +1,11 @@
-import { IconButton, CancelIcon } from '@hypothesis/frontend-shared';
+import {
+  IconButton,
+  CancelIcon,
+  ModalDialog,
+} from '@hypothesis/frontend-shared';
 import classnames from 'classnames';
-import { useEffect, useRef, useState } from 'preact/hooks';
+import type { Ref } from 'preact';
+import { useEffect, useLayoutEffect, useRef, useState } from 'preact/hooks';
 
 import { addConfigFragment } from '../../shared/config-fragment';
 import { createAppConfig } from '../config/app';
@@ -19,15 +24,16 @@ export type NotebookConfig = {
 type NotebookIframeProps = {
   config: NotebookConfig;
   groupId: string;
+  iframeRef: Ref<HTMLIFrameElement>;
 };
 /**
  * Create the iframe that will load the notebook application.
  */
-function NotebookIframe({ config, groupId }: NotebookIframeProps) {
+function NotebookIframe({ config, groupId, iframeRef }: NotebookIframeProps) {
   const notebookAppSrc = addConfigFragment(config.notebookAppUrl, {
     ...createAppConfig(config.notebookAppUrl, config),
 
-    // Explicity set the "focused" group
+    // Explicitly set the "focused" group
     group: groupId,
   });
 
@@ -37,9 +43,11 @@ function NotebookIframe({ config, groupId }: NotebookIframeProps) {
       className="h-full w-full border-0"
       allow="fullscreen; clipboard-write"
       src={notebookAppSrc}
+      ref={iframeRef}
     />
   );
 }
+
 export type NotebookModalProps = {
   eventBus: EventBus;
   config: NotebookConfig;
@@ -61,6 +69,7 @@ export default function NotebookModal({
   const [groupId, setGroupId] = useState<string | null>(null);
   const originalDocumentOverflowStyle = useRef('');
   const emitterRef = useRef<Emitter | null>(null);
+  const iframeRef = useRef<HTMLIFrameElement | null>(null);
 
   // Stores the original overflow CSS property of document.body and reset it
   // when the component is destroyed
@@ -96,6 +105,17 @@ export default function NotebookModal({
     };
   }, [eventBus]);
 
+  useLayoutEffect(() => {
+    if (!isHidden) {
+      // When the notebook is shown, focus the iframe so that the next element
+      // in the tab sequence is an element inside of it.
+      // We can't use ModalDialog's initialFocus prop because it assumes the
+      // modal is destroyed when closed, and would cause the iframe to focus
+      // only the first time it's opened.
+      iframeRef.current?.focus();
+    }
+  }, [isHidden]);
+
   const onClose = () => {
     setIsHidden(true);
     emitterRef.current?.publish('closeNotebook');
@@ -107,14 +127,16 @@ export default function NotebookModal({
 
   return (
     <div
-      className={classnames(
-        'fixed z-max top-0 left-0 right-0 bottom-0 p-3 bg-black/50',
-        { hidden: isHidden },
-      )}
+      className={classnames({ hidden: isHidden })}
       data-testid="notebook-outer"
     >
-      <div className="relative w-full h-full" data-testid="notebook-inner">
-        <div className="absolute right-0 m-3">
+      <ModalDialog
+        variant="custom"
+        size="custom"
+        classes="p-3 relative w-full h-full"
+        initialFocus="manual"
+      >
+        <div className="absolute right-6 top-6">
           <IconButton
             title="Close the Notebook"
             onClick={onClose}
@@ -130,8 +152,13 @@ export default function NotebookModal({
             <CancelIcon className="w-4 h-4" />
           </IconButton>
         </div>
-        <NotebookIframe key={iframeKey} config={config} groupId={groupId} />
-      </div>
+        <NotebookIframe
+          key={iframeKey}
+          config={config}
+          groupId={groupId}
+          iframeRef={iframeRef}
+        />
+      </ModalDialog>
     </div>
   );
 }
diff --git a/src/annotator/components/test/NotebookModal-test.js b/src/annotator/components/test/NotebookModal-test.js
index 12c7a714099..6dca4204237 100644
--- a/src/annotator/components/test/NotebookModal-test.js
+++ b/src/annotator/components/test/NotebookModal-test.js
@@ -14,12 +14,13 @@ describe('NotebookModal', () => {
 
   const outerSelector = '[data-testid="notebook-outer"]';
 
-  const createComponent = config => {
+  const createComponent = ({ attachTo, ...config } = {}) => {
     const component = mount(
       <NotebookModal
         eventBus={eventBus}
         config={{ notebookAppUrl: notebookURL, ...config }}
       />,
+      { attachTo },
     );
     components.push(component);
     return component;
@@ -106,6 +107,26 @@ describe('NotebookModal', () => {
     assert.notEqual(iframe1.getDOMNode(), iframe3.getDOMNode());
   });
 
+  it('focuses iframe when the notebook is opened', () => {
+    const container = document.createElement('div');
+    document.body.appendChild(container);
+
+    try {
+      const wrapper = createComponent({ attachTo: container });
+
+      // The body is initially focused
+      assert.equal(document.activeElement, document.body);
+
+      emitter.publish('openNotebook', '1');
+      wrapper.update();
+
+      // Once notebook is opened, focus transitions to iframe
+      assert.equal(document.activeElement, wrapper.find('iframe').getDOMNode());
+    } finally {
+      container.remove();
+    }
+  });
+
   it('makes the document unscrollable on "openNotebook" event', () => {
     createComponent();
     act(() => {
diff --git a/yarn.lock b/yarn.lock
index 7b7366694f4..0345a806b9b 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -1995,15 +1995,15 @@ __metadata:
   languageName: node
   linkType: hard
 
-"@hypothesis/frontend-shared@npm:^7.2.0":
-  version: 7.2.0
-  resolution: "@hypothesis/frontend-shared@npm:7.2.0"
+"@hypothesis/frontend-shared@npm:^7.3.0":
+  version: 7.3.0
+  resolution: "@hypothesis/frontend-shared@npm:7.3.0"
   dependencies:
     highlight.js: ^11.6.0
     wouter-preact: ^2.10.0-alpha.1
   peerDependencies:
     preact: ^10.4.0
-  checksum: fe4b515e0f856dcb6c9876e52aba2a262d1bf6cebb13176b6ee2a676df719e2331106ee1595e2de057843fe7ebe8ac03f3a8f568fb03f50b3ea0622496da2ead
+  checksum: 5b295e0cb949c858d70c96ab24e7ec387d31e4a0e5bde6555ba70843cdceaae030a119e20841fa4924888db618c7fa47bdd4f83a3be65da889ad702506cc75f4
   languageName: node
   linkType: hard
 
@@ -7793,7 +7793,7 @@ __metadata:
     "@babel/preset-react": ^7.0.0
     "@babel/preset-typescript": ^7.16.7
     "@hypothesis/frontend-build": ^3.0.0
-    "@hypothesis/frontend-shared": ^7.2.0
+    "@hypothesis/frontend-shared": ^7.3.0
     "@hypothesis/frontend-testing": ^1.2.0
     "@npmcli/arborist": ^7.0.0
     "@octokit/rest": ^20.0.1