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 · Sep 18, 2024 · 220416c · 220416c
1 parent 19151fe
commit 220416c
Show file tree

Hide file tree

Showing 7 changed files with 1,001 additions and 880 deletions.
diff --git a/test/integration/find_spec.mjs b/test/integration/find_spec.mjs
@@ -43,7 +43,7 @@ describe("find bar", () => {
           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

diff --git a/web/annotation_editor_layer_builder.css b/web/annotation_editor_layer_builder.css
@@ -1226,18 +1226,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: 12px;
 
   .colorPicker {
     display: flex;
@@ -1261,6 +1252,7 @@
         align-items: center;
         background: none;
         flex: 0 0 auto;
+        padding: 0;
 
         .swatch {
           width: 24px;
@@ -1290,7 +1282,6 @@
     align-self: stretch;
 
     .editorParamsLabel {
-      width: 100%;
       height: auto;
       align-self: stretch;
     }

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;
 
@@ -166,13 +164,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);
     }
@@ -184,7 +175,6 @@ class PDFFindBar {
     if (!this.opened) {
       return;
     }
-    this.#resizeObserver.disconnect();
 
     this.opened = false;
     toggleExpandedBtn(this.toggleButton, false, this.bar);
@@ -199,25 +189,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
@@ -21,7 +21,7 @@ import {
   DEFAULT_SCALE_VALUE,
   MAX_SCALE,
   MIN_SCALE,
-  toggleCheckedBtn,
+  toggleExpandedBtn,
 } from "./ui_utils.js";
 
 /**
@@ -180,6 +180,9 @@ class Toolbar {
     for (const { element, eventName, eventDetails, telemetry } of buttons) {
       element.addEventListener("click", evt => {
         if (eventName !== null) {
+          if (evt.target !== element) {
+            return;
+          }
           eventBus.dispatch(eventName, {
             source: this,
             ...eventDetails,
@@ -270,32 +273,32 @@ class Toolbar {
       editorStampParamsToolbar,
     } = this.#opts;
 
-    toggleCheckedBtn(
+    toggleExpandedBtn(
       editorFreeTextButton,
       mode === AnnotationEditorType.FREETEXT,
       editorFreeTextParamsToolbar
     );
-    toggleCheckedBtn(
+    toggleExpandedBtn(
       editorHighlightButton,
       mode === AnnotationEditorType.HIGHLIGHT,
       editorHighlightParamsToolbar
     );
-    toggleCheckedBtn(
+    toggleExpandedBtn(
       editorInkButton,
       mode === AnnotationEditorType.INK,
       editorInkParamsToolbar
     );
-    toggleCheckedBtn(
+    toggleExpandedBtn(
       editorStampButton,
       mode === AnnotationEditorType.STAMP,
       editorStampParamsToolbar
     );
 
     const isDisable = mode === AnnotationEditorType.DISABLE;
-    editorFreeTextButton.disabled = isDisable;
-    editorHighlightButton.disabled = isDisable;
-    editorInkButton.disabled = isDisable;
-    editorStampButton.disabled = isDisable;
+    editorFreeTextButton.toggleAttribute("disabled", isDisable);
+    editorHighlightButton.toggleAttribute("disabled", isDisable);
+    editorInkButton.toggleAttribute("disabled", isDisable);
+    editorStampButton.toggleAttribute("disabled", isDisable);
   }
 
   #updateUIState(resetNumPages = false) {

diff --git a/web/viewer-geckoview.html b/web/viewer-geckoview.html
@@ -93,13 +93,13 @@
 
   </head>
 
-  <body tabindex="1">
+  <body tabindex="0">
     <div id="outerContainer">
 
       <div id="mainContainer">
 
         <div id="floatingToolbar">
-          <button id="download" class="toolbarButton" type="button" title="Download" tabindex="31" data-l10n-id="pdfjs-download-button">
+          <button id="download" class="toolbarButton" type="button" title="Download" tabindex="0" data-l10n-id="pdfjs-download-button">
             <span data-l10n-id="pdfjs-download-button-label">Download</span>
           </button>
         </div>