Refactor the toolbar html & css to improve its overall accessibility …

…(bug 1171799, bug 1855695) The first goal of this patch was to remove the tabindex because it helps to improve overall a11y. That led to move some html elements associated with the buttons which helped to position these elements relatively to their buttons. Consequently it was easy to change the toolbar height (configurable in Firefox with the pref browser.uidensity): it's the second goal of this patch. For a11y reasons we want to be able to change the height of the toolbar to make the buttons larger.
mozilla · Jul 4, 2024 · eefaf87 · eefaf87
1 parent 790470c
commit eefaf87
Show file tree

Hide file tree

Showing 14 changed files with 1,049 additions and 955 deletions.
diff --git a/src/display/editor/tools.js b/src/display/editor/tools.js
@@ -644,10 +644,15 @@ class AnnotationEditorUIManager {
      * If the focused element is an input, we don't want to handle the arrow.
      * For example, sliders can be controlled with the arrow keys.
      */
-    const arrowChecker = self =>
-      self.#container.contains(document.activeElement) &&
-      document.activeElement.tagName !== "BUTTON" &&
-      self.hasSomethingToControl();
+    const arrowChecker = self => {
+      const { activeElement } = document;
+      return (
+        self.#container.contains(activeElement) &&
+        activeElement.tagName !== "BUTTON" &&
+        !activeElement.closest(".toolbarButton") &&
+        self.hasSomethingToControl()
+      );
+    };
 
     const textInputChecker = (_self, { target: el }) => {
       if (el instanceof HTMLInputElement) {

diff --git a/test/integration/find_spec.mjs b/test/integration/find_spec.mjs
@@ -40,10 +40,10 @@ describe("find bar", () => {
       await Promise.all(
         pages.map(async ([browserName, page]) => {
           // Highlight all occurrences of the letter A (case insensitive).
-          await page.click("#viewFind");
-          await page.waitForSelector("#viewFind", { hidden: false });
+          await page.click("#viewFindButton");
+          await page.waitForSelector("#viewFindButton", { hidden: false });
           await page.type("#findInput", "a");
-          await page.click("#findHighlightAll");
+          await page.click("#findHighlightAll + label");
           await page.waitForSelector(".textLayer .highlight");
 
           // The PDF file contains the text 'AB BA' in a monospace font on a
@@ -100,8 +100,8 @@ describe("find bar", () => {
     it("must search xfa correctly", async () => {
       await Promise.all(
         pages.map(async ([browserName, page]) => {
-          await page.click("#viewFind");
-          await page.waitForSelector("#viewFind", { hidden: false });
+          await page.click("#viewFindButton");
+          await page.waitForSelector("#viewFindButton", { hidden: false });
           await page.type("#findInput", "preferences");
           await page.waitForSelector("#findInput[data-status='']");
           await page.waitForSelector(".xfaLayer .highlight");

diff --git a/test/integration/freetext_editor_spec.mjs b/test/integration/freetext_editor_spec.mjs
@@ -2753,7 +2753,7 @@ describe("FreeText Editor", () => {
     it("must create an editor from the toolbar", async () => {
       await Promise.all(
         pages.map(async ([browserName, page]) => {
-          await page.focus("#editorFreeText");
+          await page.focus("#editorFreeTextButton");
           await page.keyboard.press("Enter");
 
           let selectorEditor = getEditorSelector(0);
@@ -2784,7 +2784,7 @@ describe("FreeText Editor", () => {
           // Disable editing mode.
           await switchToFreeText(page, /* disable = */ true);
 
-          await page.focus("#editorFreeText");
+          await page.focus("#editorFreeTextButton");
           await page.keyboard.press(" ");
           selectorEditor = getEditorSelector(1);
           await page.waitForSelector(selectorEditor, {

diff --git a/test/integration/highlight_editor_spec.mjs b/test/integration/highlight_editor_spec.mjs
@@ -1073,11 +1073,12 @@ describe("Highlight Editor", () => {
         pages.map(async ([browserName, page]) => {
           await switchToHighlight(page);
 
+          await page.focus(`.page[data-page-number="1"] > .textLayer`);
           await page.evaluate(() => {
             const text =
               "Dynamic languages such as JavaScript are more difﬁcult to com-";
             for (const el of document.querySelectorAll(
-              `.page[data-page-number="${1}"] > .textLayer > span`
+              `.page[data-page-number="1"] > .textLayer > span`
             )) {
               if (el.textContent === text) {
                 window.getSelection().setPosition(el.firstChild, 15);

diff --git a/test/integration/scripting_spec.mjs b/test/integration/scripting_spec.mjs
@@ -439,7 +439,7 @@ describe("Interaction", () => {
           page,
           getSelector("47R"),
           async () => {
-            await page.click("#print");
+            await page.click("#printButton");
           }
         );
         expect(text).withContext(`In ${browserName}`).toEqual("WillPrint");
@@ -488,7 +488,7 @@ describe("Interaction", () => {
             page,
             getSelector("47R"),
             async () => {
-              await page.click("#download");
+              await page.click("#downloadButton");
             }
           );
           expect(text).withContext(`In ${browserName}`).toEqual("WillSave");

diff --git a/test/integration/test_utils.mjs b/test/integration/test_utils.mjs
@@ -660,7 +660,7 @@ async function switchToEditor(name, page, disable = false) {
       { once: true }
     );
   });
-  await page.click(`#editor${name}`);
+  await page.click(`#editor${name}Button`);
   name = name.toLowerCase();
   await page.waitForSelector(
     ".annotationEditorLayer" +

diff --git a/web/annotation_editor_layer_builder.css b/web/annotation_editor_layer_builder.css
@@ -942,18 +942,9 @@
 }
 
 #highlightParamsToolbarContainer {
-  height: auto;
-  padding-inline: 10px;
-  padding-block: 10px 16px;
   gap: 16px;
-  display: flex;
-  flex-direction: column;
-  box-sizing: border-box;
-
-  .editorParamsLabel {
-    width: fit-content;
-    inset-inline-start: 0;
-  }
+  padding-inline: 10px;
+  padding-block-end: 16px;
 
   .colorPicker {
     display: flex;
@@ -1006,7 +997,6 @@
     align-self: stretch;
 
     .editorParamsLabel {
-      width: 100%;
       height: auto;
       align-self: stretch;
     }

diff --git a/web/app.js b/web/app.js
@@ -2995,7 +2995,8 @@ function webViewerKeyDown(evt) {
     curElementTagName === "SELECT" ||
     (curElementTagName === "BUTTON" &&
       (evt.keyCode === /* Enter = */ 13 || evt.keyCode === /* Space = */ 32)) ||
-    curElement?.isContentEditable
+    curElement?.isContentEditable ||
+    curElement?.closest(".toolbarButton")
   ) {
     // Make sure that the secondary toolbar is closed when Escape is pressed.
     if (evt.keyCode !== /* Esc = */ 27) {

diff --git a/web/pdf_find_bar.js b/web/pdf_find_bar.js
@@ -25,8 +25,6 @@ const MATCHES_COUNT_LIMIT = 1000;
  * is done by PDFFindController.
  */
 class PDFFindBar {
-  #resizeObserver = new ResizeObserver(this.#resizeObserverCallback.bind(this));
-
   constructor(options, eventBus) {
     this.opened = false;
 
@@ -162,13 +160,6 @@ class PDFFindBar {
 
   open() {
     if (!this.opened) {
-      // Potentially update the findbar layout, row vs column, when:
-      //  - The width of the viewer itself changes.
-      //  - The width of the findbar changes, by toggling the visibility
-      //    (or localization) of find count/status messages.
-      this.#resizeObserver.observe(this.bar.parentNode);
-      this.#resizeObserver.observe(this.bar);
-
       this.opened = true;
       toggleExpandedBtn(this.toggleButton, true, this.bar);
     }
@@ -180,7 +171,6 @@ class PDFFindBar {
     if (!this.opened) {
       return;
     }
-    this.#resizeObserver.disconnect();
 
     this.opened = false;
     toggleExpandedBtn(this.toggleButton, false, this.bar);
@@ -195,25 +185,6 @@ class PDFFindBar {
       this.open();
     }
   }
-
-  #resizeObserverCallback(entries) {
-    const { bar } = this;
-    // The find bar has an absolute position and thus the browser extends
-    // its width to the maximum possible width once the find bar does not fit
-    // entirely within the window anymore (and its elements are automatically
-    // wrapped). Here we detect and fix that.
-    bar.classList.remove("wrapContainers");
-
-    const findbarHeight = bar.clientHeight;
-    const inputContainerHeight = bar.firstElementChild.clientHeight;
-
-    if (findbarHeight > inputContainerHeight) {
-      // The findbar is taller than the input container, which means that
-      // the browser wrapped some of the elements. For a consistent look,
-      // wrap all of them to adjust the width of the find bar.
-      bar.classList.add("wrapContainers");
-    }
-  }
 }
 
 export { PDFFindBar };
diff --git a/web/toolbar.js b/web/toolbar.js
@@ -186,6 +186,9 @@ class Toolbar {
     for (const { element, eventName, eventDetails } of buttons) {
       element.addEventListener("click", evt => {
         if (eventName !== null) {
+          if (evt.target !== element) {
+            return;
+          }
           eventBus.dispatch(eventName, {
             source: this,
             ...eventDetails,
@@ -270,10 +273,17 @@ class Toolbar {
     );
 
     const isDisable = mode === AnnotationEditorType.DISABLE;
-    editorFreeTextButton.disabled = isDisable;
-    editorHighlightButton.disabled = isDisable;
-    editorInkButton.disabled = isDisable;
-    editorStampButton.disabled = isDisable;
+    if (isDisable) {
+      editorFreeTextButton.disabled = "disabled";
+      editorHighlightButton.disabled = "disabled";
+      editorInkButton.disabled = "disabled";
+      editorStampButton.disabled = "disabled";
+    } else {
+      editorFreeTextButton.removeAttribute("disabled");
+      editorHighlightButton.removeAttribute("disabled");
+      editorInkButton.removeAttribute("disabled");
+      editorStampButton.removeAttribute("disabled");
+    }
   }
 
   #updateUIState(resetNumPages = false) {

diff --git a/web/ui_utils.js b/web/ui_utils.js
@@ -851,6 +851,9 @@ function apiPageModeToSidebarView(mode) {
 function toggleCheckedBtn(button, toggle, view = null) {
   button.classList.toggle("toggled", toggle);
   button.setAttribute("aria-checked", toggle);
+  if (button.parentElement.classList.contains("toolbarButtonWithContainer")) {
+    button.setAttribute("aria-expanded", toggle);
+  }
 
   view?.classList.toggle("hidden", !toggle);
 }