Bug 1625263 - Fix and polish attachment pane keyboard access. r=aleca
☠☠ backed out by 7fc15b13b7ee ☠ ☠
authorThomas Duellmann <bugzilla2007@duellmann24.net>
Thu, 14 May 2020 18:54:41 +0200
changeset 39249 d8ad92570434f84f790e5ce1f160c7c07f692006
parent 39248 02023407ac6144e37b70c88c64f908a95486fedb
child 39250 7fc15b13b7eef6b459ead9200ec8c228fe4f4925
push id402
push userclokep@gmail.com
push dateMon, 29 Jun 2020 20:48:04 +0000
reviewersaleca
bugs1625263
Bug 1625263 - Fix and polish attachment pane keyboard access. r=aleca
mail/base/content/mailWidgets.js
mail/components/compose/content/MsgComposeCommands.js
mail/components/compose/content/messengercompose.xhtml
mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
--- a/mail/base/content/mailWidgets.js
+++ b/mail/base/content/mailWidgets.js
@@ -1272,39 +1272,45 @@
     constructor() {
       super();
 
       this.messenger = Cc["@mozilla.org/messenger;1"].createInstance(
         Ci.nsIMessenger
       );
 
       this.addEventListener("keypress", event => {
-        // The spacebar should work just like the arrow keys, except that the
-        // focused element doesn't change, so use moveByOffset here.
-        if (event.keyCode == KeyEvent.DOM_VK_SPACE) {
-          this.moveByOffset(0, !event.ctrlKey, event.shiftKey);
-          event.preventDefault();
-        } else if (event.keyCode == KeyEvent.DOM_VK_RETURN) {
-          if (this.currentItem && !event.ctrlKey && !event.shiftKey) {
-            this.addItemToSelection(this.currentItem);
-            let evt = document.createEvent("XULCommandEvent");
-            evt.initCommandEvent(
-              "command",
-              true,
-              true,
-              window,
-              0,
-              event.ctrlKey,
-              event.altKey,
-              event.shiftKey,
-              event.metaKey,
-              null
-            );
-            this.currentItem.dispatchEvent(evt);
-          }
+        switch (event.key) {
+          case " ":
+            // Allow plain spacebar to select the focused item.
+            if (!event.shiftKey && !event.ctrlKey) {
+              this.addItemToSelection(this.currentItem);
+            }
+            // Prevent inbuilt scrolling.
+            event.preventDefault();
+            break;
+
+          case "Enter":
+            if (this.currentItem && !event.ctrlKey && !event.shiftKey) {
+              this.addItemToSelection(this.currentItem);
+              let evt = document.createEvent("XULCommandEvent");
+              evt.initCommandEvent(
+                "command",
+                true,
+                true,
+                window,
+                0,
+                event.ctrlKey,
+                event.altKey,
+                event.shiftKey,
+                event.metaKey,
+                null
+              );
+              this.currentItem.dispatchEvent(evt);
+            }
+            break;
         }
       });
 
       this.addEventListener("click", event => {
         if (
           event.button != 0 ||
           event.target.classList.contains("attachmentItem")
         ) {
--- a/mail/components/compose/content/MsgComposeCommands.js
+++ b/mail/components/compose/content/MsgComposeCommands.js
@@ -808,17 +808,20 @@ var defaultController = {
       },
     },
 
     cmd_toggleAttachmentPane: {
       isEnabled() {
         return !gWindowLocked;
       },
       doCommand() {
-        toggleAttachmentPane();
+        // Here we pick up the inbuilt command event, to check modifiers later.
+        // Note: We cannot pass event along the call chain: Bug 461578 / 959494.
+        // eslint-disable-next-line no-restricted-globals
+        toggleAttachmentPane("toggle", event);
       },
     },
 
     cmd_reorderAttachments: {
       isEnabled() {
         if (attachmentsCount() == 0) {
           let reorderAttachmentsPanel = document.getElementById(
             "reorderAttachmentsPanel"
@@ -5903,30 +5906,33 @@ function UpdateAttachmentBucket(aShowBuc
  * and tooltip of attachment pane, then (optionally) show or hide the pane.
  *
  * @param aShowPane {string} "show":  show the attachment pane
  *                           "hide":  hide the attachment pane
  *                           omitted: just update without changing pane visibility
  */
 function updateAttachmentPane(aShowPane) {
   let bucket = GetMsgAttachmentElement();
-  let bucketCountLabel = document.getElementById("attachmentBucketCount");
-  let words = getComposeBundle().getString("attachmentCount");
   let count = bucket.itemCount;
-  let countStr = PluralForm.get(count, words).replace("#1", count);
-
-  bucketCountLabel.value = countStr;
+
+  document.l10n.setAttributes(
+    document.getElementById("attachmentBucketCount"),
+    "attachment-bucket-count",
+    { count }
+  );
+
   document.getElementById("attachmentBucketSize").value =
     count > 0 ? gMessenger.formatFileSize(gAttachmentsSize) : "";
   document.getElementById("attachmentBucketCloseButton").collapsed = count > 0;
 
-  let placeholderTooltip = count > 0 ? countStr : "";
-  document
-    .getElementById("attachments-placeholder-box")
-    .setAttribute("tooltiptext", placeholderTooltip);
+  document.l10n.setAttributes(
+    document.getElementById("attachments-placeholder-box"),
+    "attachments-placeholder-tooltip",
+    { count }
+  );
 
   attachmentBucketUpdateTooltips();
 
   // If aShowPane argument is omitted, it's just updating, so we're done.
   if (aShowPane === undefined) {
     return;
   }
 
@@ -6040,23 +6046,16 @@ function RenameSelectedAttachment() {
 
     gContentChanged = true;
 
     let event = document.createEvent("CustomEvent");
     event.initCustomEvent("attachment-renamed", true, true, originalName);
     item.dispatchEvent(event);
   }
 
-  let reorderAttachmentsPanel = document.getElementById(
-    "reorderAttachmentsPanel"
-  );
-  if (reorderAttachmentsPanel.state == "open") {
-    // Hack to ensure that reorderAttachmentsPanel does not get closed as we exit.
-    bucket.setAttribute("data-ignorenextblur", "true");
-  }
   // Update cmd_sortAttachmentsToggle because renaming may change the current
   // sort order.
   goUpdateCommand("cmd_sortAttachmentsToggle");
 }
 
 /* eslint-disable complexity */
 /**
  * Move selected attachment(s) within the attachment list.
@@ -6306,54 +6305,78 @@ function moveSelectedAttachments(aDirect
   updateReorderAttachmentsItems();
 }
 /* eslint-enable complexity */
 
 /**
  * Toggle attachment pane view state: show or hide it.
  * If aAction parameter is omitted, toggle current view state.
  *
- * @param aAction {string} "show":   show attachment pane
- *                         "hide":   hide attachment pane
- *                         "toggle": toggle attachment pane visibility
+ * @param {string} [aAction = "toggle"] - "show":   show attachment pane
+ *                                        "hide":   hide attachment pane
+ *                                        "toggle": toggle attachment pane
+ * @param {Event} [event] - The command event (cmd_toggleAttachmentPane)
  */
-function toggleAttachmentPane(aAction = "toggle") {
+function toggleAttachmentPane(aAction = "toggle", event) {
   let bucket = GetMsgAttachmentElement();
   let attachmentsBox = document.getElementById("attachments-box");
   let attachmentBucketSizer = document.getElementById("attachmentbucket-sizer");
+  let bucketHasFocus = document.activeElement == bucket;
 
   if (aAction == "toggle") {
-    aAction = attachmentsBox.collapsed ? "show" : "hide";
+    let shown = !attachmentsBox.collapsed;
+
+    if (shown && !bucketHasFocus && event && event.altKey) {
+      // If attachment pane is shown but not focused, and we're here via
+      // key_toggleAttachmentPane, handle access key here: Focus bucket.
+      bucket.focus();
+      if (bucket.currentItem) {
+        bucket.ensureElementIsVisible(bucket.currentItem);
+      }
+      return;
+    }
+
+    // Toggle attachment pane.
+    aAction = shown ? "hide" : "show";
   }
 
   switch (aAction) {
     case "show": {
-      let shown = !attachmentsBox.collapsed;
       attachmentsBox.collapsed = false;
       attachmentBucketSizer.collapsed = false;
       attachmentBucketSizer.setAttribute("state", "");
-      if (shown) {
+      if (!bucketHasFocus) {
         bucket.focus();
       }
+      if (bucket.currentItem) {
+        bucket.ensureElementIsVisible(bucket.currentItem);
+      }
       break;
     }
 
     case "hide": {
-      if (document.activeElement == bucket) {
+      if (bucketHasFocus) {
         SetMsgBodyFrameFocus();
       }
       attachmentsBox.collapsed = true;
       attachmentBucketSizer.setAttribute("state", "collapsed");
       break;
     }
   }
+
+  // Update the checkmark on menuitems hooked up with cmd_toggleAttachmentPane.
+  // Menuitem does not have .checked property nor .toggleAttribute(), sigh.
   for (let menuitem of document.querySelectorAll(
     'menuitem[command="cmd_toggleAttachmentPane"]'
   )) {
-    menuitem.checked = aAction == "show";
+    if (aAction == "show") {
+      menuitem.setAttribute("checked", "true");
+    } else {
+      menuitem.removeAttribute("checked");
+    }
   }
 }
 
 function showReorderAttachmentsPanel() {
   // Ensure attachment pane visibility as it might be collapsed.
   toggleAttachmentPane("show");
   showPopupById(
     "reorderAttachmentsPanel",
@@ -6488,32 +6511,34 @@ function attachmentHeaderContextOnPopupS
 
 function toggleInitiallyShowAttachmentPane(aMenuItem) {
   Services.prefs.setBoolPref(
     "mail.compose.show_attachment_pane",
     aMenuItem.getAttribute("checked")
   );
 }
 
+/**
+ * Handle blur event on attachment pane. Deselect all attachments if appropriate
+ * and control visibility of reorderAttachmentsPanel.
+ */
 function attachmentBucketOnBlur() {
-  // Ensure that reorderAttachmentsPanel remains open while we're focused
-  // on attachmentBucket or the panel, otherwise hide it.
   let attachmentBucket = document.getElementById("attachmentBucket");
-  if (attachmentBucket.getAttribute("data-ignorenextblur") == "true") {
-    // Hack to prevent the panel from hiding after RenameSelectedAttachment()
-    attachmentBucket.setAttribute("data-ignorenextblur", "false");
-    return;
-  }
   let reorderAttachmentsPanel = document.getElementById(
     "reorderAttachmentsPanel"
   );
+  // If attachment pane has really lost focus, and if reorderAttachmentsPanel is
+  // not currently in the process of showing up, deselect attachments and hide
+  // reorderAttachmentsPanel. Otherwise, keep attachments selected and the
+  // reorderAttachmentsPanel open when reordering and after renaming via dialog.
   if (
-    document.activeElement.id != "attachmentBucket" ||
-    document.activeElement.id != "reorderAttachmentsPanel"
+    document.activeElement.id != "attachmentBucket" &&
+    reorderAttachmentsPanel.state != "showing"
   ) {
+    attachmentBucket.clearSelection();
     reorderAttachmentsPanel.hidePopup();
   }
 }
 
 function attachmentBucketOnKeyPress(aEvent) {
   let bucket = GetMsgAttachmentElement();
 
   // When ESC is pressed ...
--- a/mail/components/compose/content/messengercompose.xhtml
+++ b/mail/components/compose/content/messengercompose.xhtml
@@ -414,27 +414,24 @@
   <key id="pastequotationkb" key="&pasteAsQuotationCmd.key;"
        observes="cmd_pasteQuote" modifiers="accel, shift"/>
   <key id="pastenoformattingkb" key="&pasteNoFormattingCmd.key;"
        modifiers="accel, shift" observes="cmd_pasteNoFormatting"/>
   <key id="key_rewrap" key="&editRewrapCmd.key;" command="cmd_rewrap" modifiers="accel"/>
 #ifdef XP_MACOSX
   <key id="key_delete" keycode="VK_BACK" command="cmd_delete"/>
   <key id="key_delete2" keycode="VK_DELETE" command="cmd_delete"/>
-  <key id="key_toggleAttachmentPane"
-       key="&toggleAttachmentPaneCmd.key;" modifiers="control"
-       command="cmd_toggleAttachmentPane"/>
 #else
   <key id="key_delete" keycode="VK_DELETE" command="cmd_delete"/>
   <key id="key_renameAttachment" keycode="VK_F2"
        command="cmd_renameAttachment"/>
+#endif
   <key id="key_toggleAttachmentPane"
-       key="&toggleAttachmentPaneCmd.key;" modifiers="alt"
+       data-l10n-id="key-toggle-attachment-pane" modifiers="alt"
        command="cmd_toggleAttachmentPane"/>
-#endif
   <key id="key_reorderAttachments"
        key="&reorderAttachmentsCmd.key;" modifiers="accel,shift"
        command="cmd_reorderAttachments"/>
   <key id="key_selectAll" data-l10n-id="text-action-select-all-shortcut" modifiers="accel"/>
   <key id="key_find" key="&findBarCmd.key;" command="cmd_find" modifiers="accel"/>
 #ifndef XP_MACOSX
   <key id="key_findReplace" key="&findReplaceCmd.key;" command="cmd_findReplace" modifiers="accel"/>
 #endif
@@ -2453,21 +2450,16 @@
                        role="listbox"
                        context="msgComposeAttachmentListContext"
                        itemcontext="msgComposeAttachmentItemContext"
                        onclick="attachmentBucketOnClick(event);"
                        onkeypress="attachmentBucketOnKeyPress(event);"
                        onselect="attachmentBucketOnSelect();"
                        ondragstart="attachmentBucketDNDObserver.onDragStart(event);"
                        onblur="attachmentBucketOnBlur();"/>
-          <hbox pack="end">
-            <button label="&attachFile.label;"
-                    accesskey="&attachFile.accesskey;"
-                    command="cmd_attachFile"/>
-          </hbox>
         </vbox>
         <vbox id="attachments-placeholder-box"
               hidden="true"
               tooltiptext=""
               onclick="goDoCommand('cmd_toggleAttachmentPane')"/>
       </hbox>
     </toolbar>
 
--- a/mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
+++ b/mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ -180,21 +180,16 @@ attachPageDlogTitle=Please specify a loc
 attachPageDlogMessage=Web Page (URL):
 
 ## String used for attachment pretty name, when the attachment is a message
 messageAttachmentSafeName=Attached Message
 
 ## String used for attachment pretty name, when the attachment is a message part
 partAttachmentSafeName=Attached Message Part
 
-# LOCALIZATION NOTE (attachmentCount): Semi-colon list of plural forms.
-# See: https://developer.mozilla.org/en/Localization_and_Plurals
-# #1 number of attachments
-attachmentCount=#1 attachment;#1 attachments
-
 # LOCALIZATION NOTE (attachmentBucketAttachFilesTooltip):
 # This tooltip should be same as attachFile.label in messengercompose.dtd,
 # but without ellipsis (…).
 attachmentBucketAttachFilesTooltip=Attach File(s)
 attachmentBucketClearSelectionTooltip=Clear Selection
 attachmentBucketHeaderShowTooltip=Show attachment pane
 attachmentBucketHeaderMinimizeTooltip=Minimize attachment pane
 attachmentBucketHeaderRestoreTooltip=Restore attachment pane
--- a/mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
+++ b/mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
@@ -79,17 +79,16 @@
 <!ENTITY editRewrapCmd.key "R">
 <!ENTITY renameAttachmentCmd.label "Rename Attachment…">
 <!ENTITY renameAttachmentCmd.accesskey "e">
 <!ENTITY reorderAttachmentsCmd.label "Reorder Attachments…">
 <!ENTITY reorderAttachmentsCmd.accesskey "s">
 <!ENTITY reorderAttachmentsCmd.key "x">
 <!ENTITY toggleAttachmentPaneCmd.label "Attachment Pane">
 <!ENTITY toggleAttachmentPaneCmd.accesskey "m">
-<!ENTITY toggleAttachmentPaneCmd.key "M">
 <!ENTITY selectAllCmd.accesskey "a">
 <!ENTITY findBarCmd.label "Find…">
 <!ENTITY findBarCmd.accesskey "F">
 <!ENTITY findBarCmd.key "F">
 <!ENTITY findReplaceCmd.label "Find and Replace…">
 <!ENTITY findReplaceCmd.accesskey "l">
 <!ENTITY findReplaceCmd.key "H">
 <!ENTITY findAgainCmd.label "Find Again">
@@ -249,17 +248,16 @@
 <!ENTITY toAddr2.label "To">
 <!ENTITY ccAddr2.label "Cc">
 <!ENTITY bccAddr2.label "Bcc">
 <!ENTITY replyAddr2.label "Reply-To">
 <!ENTITY newsgroupsAddr2.label "Newsgroup">
 <!ENTITY followupAddr2.label "Followup-To">
 <!ENTITY subject2.label "Subject">
 <!ENTITY subject.accesskey "S">
-<!ENTITY attachmentBucketHeader.tooltip "Attachment Pane">
 <!ENTITY attachmentBucketCloseButton.tooltip "Hide the attachment pane">
 
 <!-- Format Toolbar, imported from editorAppShell.xhtml -->
 <!ENTITY SmileButton.tooltip "Insert a smiley face">
 <!ENTITY smiley1Cmd.label "Smile">
 <!ENTITY smiley2Cmd.label "Frown">
 <!ENTITY smiley3Cmd.label "Wink">
 <!ENTITY smiley4Cmd.label "Tongue-out">
--- a/mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
+++ b/mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
@@ -32,8 +32,27 @@ pill-action-move-to =
 
 pill-action-move-cc =
     .label = Move to Cc
     .accesskey = C
 
 pill-action-move-bcc =
     .label = Move to Bcc
     .accesskey = B
+
+#   $count (Number) - the number of attachments in the attachment bucket
+attachment-bucket-count =
+    .value = { $count ->
+        [1]      { $count } Attachment
+        *[other] { $count } Attachments
+    }
+    .accesskey = m
+
+#   $count (Number) - the number of attachments in the attachment bucket
+attachments-placeholder-tooltip =
+    .tooltiptext = { $count ->
+        [1]      { $count } Attachment
+        *[other] { $count } Attachments
+    }
+
+#   { attachment-bucket-count.accesskey } - Do not localize this message.
+key-toggle-attachment-pane =
+    .key = { attachment-bucket-count.accesskey }