[Editor] Don't show the alt-text button when the alt-text dialog is v…

…isible This way, the button doens't cover the image.
mozilla · Oct 2, 2023 · eebd251 · eebd251
1 parent f5367f0
commit eebd251
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 13 deletions.
diff --git a/src/display/editor/editor.js b/src/display/editor/editor.js
@@ -45,6 +45,8 @@ class AnnotationEditor {
 
   #altTextTooltipTimeout = null;
 
+  #altTextWasFromKeyBoard = false;
+
   #keepAspectRatio = false;
 
   #resizersDiv = null;
@@ -840,18 +842,17 @@ class AnnotationEditor {
     altText.tabIndex = "0";
     altText.addEventListener("contextmenu", noContextMenu);
     altText.addEventListener("pointerdown", event => event.stopPropagation());
-    altText.addEventListener(
-      "click",
-      event => {
-        event.preventDefault();
-        this._uiManager.editAltText(this);
-      },
-      { capture: true }
-    );
+
+    const onClick = event => {
+      this.#altTextButton.hidden = true;
+      event.preventDefault();
+      this._uiManager.editAltText(this);
+    };
+    altText.addEventListener("click", onClick, { capture: true });
     altText.addEventListener("keydown", event => {
       if (event.target === altText && event.key === "Enter") {
-        event.preventDefault();
-        this._uiManager.editAltText(this);
+        this.#altTextWasFromKeyBoard = true;
+        onClick(event);
       }
     });
     this.#setAltTextButtonState();
@@ -877,12 +878,13 @@ class AnnotationEditor {
       this.#altTextTooltip?.remove();
       return;
     }
+    button.classList.add("done");
+
     AnnotationEditor._l10nPromise
       .get("editor_alt_text_edit_button_label")
       .then(msg => {
         button.setAttribute("aria-label", msg);
       });
-
     let tooltip = this.#altTextTooltip;
     if (!tooltip) {
       this.#altTextTooltip = tooltip = document.createElement("span");
@@ -916,7 +918,6 @@ class AnnotationEditor {
         this.#altTextTooltip?.classList.remove("show");
       });
     }
-    button.classList.add("done");
     tooltip.innerText = this.#altTextDecorative
       ? await AnnotationEditor._l10nPromise.get(
           "editor_alt_text_decorative_tooltip"
@@ -939,6 +940,15 @@ class AnnotationEditor {
     this.#altTextButton.disabled = !enabled;
   }
 
+  altTextFinish() {
+    if (!this.#altTextButton) {
+      return;
+    }
+    this.#altTextButton.hidden = false;
+    this.#altTextButton.focus({ focusVisible: this.#altTextWasFromKeyBoard });
+    this.#altTextWasFromKeyBoard = false;
+  }
+
   getClientDimensions() {
     return this.div.getBoundingClientRect();
   }

diff --git a/test/integration/stamp_editor_spec.js b/test/integration/stamp_editor_spec.js
@@ -262,6 +262,9 @@ describe("Stamp Editor", () => {
           // Click on the alt-text button.
           await page.click(buttonSelector);
 
+          // Check that the alt-text button has been hidden.
+          await page.waitForSelector(`${buttonSelector}[hidden]`);
+
           // Wait for the alt-text dialog to be visible.
           await page.waitForSelector("#altTextDialog", { visible: true });
 
@@ -275,7 +278,7 @@ describe("Stamp Editor", () => {
           await page.click(saveButtonSelector);
 
           // Wait for the alt-text button to have the correct icon.
-          await page.waitForSelector(`${buttonSelector}.done`);
+          await page.waitForSelector(`${buttonSelector}:not([hidden]).done`);
 
           // Hover the button.
           await page.hover(buttonSelector);
@@ -371,6 +374,30 @@ describe("Stamp Editor", () => {
             sel => document.querySelector(sel) === null,
             tooltipSelector
           );
+
+          // We check that the alt-text button works correctly with the
+          // keyboard.
+          await page.evaluate(sel => {
+            document.getElementById("viewerContainer").focus();
+            return new Promise(resolve => {
+              setTimeout(() => {
+                const el = document.querySelector(sel);
+                el.addEventListener("focus", resolve, { once: true });
+                el.focus({ focusVisible: true });
+              }, 0);
+            });
+          }, buttonSelector);
+          await (browserName === "chrome"
+            ? page.waitForSelector(`${buttonSelector}:focus`)
+            : page.waitForSelector(`${buttonSelector}:focus-visible`));
+          await page.keyboard.press("Enter");
+          await page.waitForSelector(`${buttonSelector}[hidden]`);
+          await page.waitForSelector("#altTextDialog", { visible: true });
+          await page.keyboard.press("Escape");
+          await page.waitForSelector(`${buttonSelector}:not([hidden])`);
+          await (browserName === "chrome"
+            ? page.waitForSelector(`${buttonSelector}:focus`)
+            : page.waitForSelector(`${buttonSelector}:focus-visible`));
         })
       );
     });

diff --git a/web/alt_text_manager.js b/web/alt_text_manager.js
@@ -264,6 +264,7 @@ class AltTextManager {
     this.#removeOnClickListeners();
     this.#uiManager?.addEditListeners();
     this.#eventBus._off("resize", this.#boundSetPosition);
+    this.#currentEditor.altTextFinish();
     this.#currentEditor = null;
     this.#uiManager = null;
   }