Bug 1602431 - Fix and handle focus and selection when removing recipient pills with DEL, BACKSPACE, context delete, or cut. r=mkmelin, ui-r=aleca
authorThomas Duellmann <bugzilla2007@duellmann24.net>
Thu, 16 Jan 2020 22:30:30 +0100
changeset 37152 cbfeac2dfd57d092d8e1498ad806e63dad213661
parent 37151 76f81e0c7e0be8fd2551b40c854731a022c4b90d
child 37153 71a037b9e9cd27ad72740b45d5bf1f4d8c6e6d8d
push id2552
push userclokep@gmail.com
push dateMon, 10 Feb 2020 21:24:16 +0000
treeherdercomm-beta@f95a6f4408a3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmkmelin, aleca
bugs1602431
Bug 1602431 - Fix and handle focus and selection when removing recipient pills with DEL, BACKSPACE, context delete, or cut. r=mkmelin, ui-r=aleca
mail/base/content/mailWidgets.js
mail/components/compose/content/addressingWidgetOverlay.js
mail/test/browser/composition/browser_sendButton.js
mail/test/browser/shared-modules/ComposeHelpers.jsm
--- a/mail/base/content/mailWidgets.js
+++ b/mail/base/content/mailWidgets.js
@@ -1798,17 +1798,17 @@
         `input[is="autocomplete-input"][recipienttype]`
       );
     }
 
     /**
      * Check if the pill is currently in "Edit Mode", meaning the label is
      * hidden and the html:input field is visible.
      *
-     * @returns {boolean} True if the pill is currently being edited.
+     * @return {boolean} true if the pill is currently being edited.
      */
     get isEditing() {
       return !this.emailInput.hasAttribute("hidden");
     }
 
     get fragment() {
       if (!this.constructor.hasOwnProperty("_fragment")) {
         this.constructor._fragment = MozXULElement.parseXULToFragment(`
@@ -1956,16 +1956,49 @@
       this.style.removeProperty("max-width");
       this.style.removeProperty("min-width");
       this.classList.remove("editing");
       this.pillLabel.removeAttribute("hidden");
       this.emailInput.setAttribute("hidden", "hidden");
       this.rowInput.focus();
     }
 
+    /**
+     * Get the nearest sibling pill which is not selected.
+     *
+     * @param {("next"|"previous")} [siblingsType="next"] - Iterate next or
+     *   previous siblings.
+     * @return {HTMLElement} - The nearest unselected sibling element, or null.
+     */
+    getUnselectedSiblingPill(siblingsType = "next") {
+      if (siblingsType == "next") {
+        // Check for next siblings.
+        let element = this.nextElementSibling;
+        while (element) {
+          if (!element.hasAttribute("selected")) {
+            return element;
+          }
+          element = element.nextElementSibling;
+        }
+
+        return null;
+      }
+
+      // Check for previous siblings.
+      let element = this.previousElementSibling;
+      while (element) {
+        if (!element.hasAttribute("selected")) {
+          return element;
+        }
+        element = element.previousElementSibling;
+      }
+
+      return null;
+    }
+
     removeObserver() {
       Services.obs.removeObserver(
         this.inputObserver,
         "autocomplete-did-enter-text"
       );
     }
   }
 
@@ -1992,17 +2025,17 @@
         setupAutocompleteInput(input, this.highlightNonMatches);
       }
     }
 
     /**
      * Create a new recipient row container with the input autocomplete.
      *
      * @param {Array} recipient - The unique identifier of the email header.
-     * @returns {XULElement} - The newly created recipient row.
+     * @return {Element} - The newly created recipient row.
      */
     buildRecipientRows(recipient) {
       let row = document.createXULElement("hbox");
       row.setAttribute("id", recipient.row);
       row.classList.add("addressingWidgetItem", "address-row");
 
       let firstCol = document.createXULElement("hbox");
       firstCol.classList.add("aw-firstColBox");
@@ -2172,35 +2205,46 @@
       params.type = element.getAttribute("recipienttype");
       pill.emailInput.setAttribute(
         "autocompletesearchparam",
         JSON.stringify(params)
       );
     }
 
     /**
-     * Move the focus on the first pill from the same .address-container.
+     * Handle keypress event on a pill in the mail-recipients-area.
      *
-     * @param {XULElement} pill - The mail-address-pill element.
+     * @param {Element} pill - The mail-address-pill element where Event fired.
      * @param {Event} event - The DOM Event.
      */
     handleKeyPress(pill, event) {
       if (pill.isEditing) {
         return;
       }
 
       switch (event.key) {
         case "Enter":
         case "F2": // For Windows users
           this.startEditing(pill, event);
           break;
 
         case "Delete":
         case "Backspace":
-          this.removePills(pill);
+          // We must never delete a focused pill which is not selected.
+          // If no pills selected, just select the focused pill.
+          // For rapid repeated deletions (esp. from holding BACKSPACE),
+          // stop before selecting another focused pill for deletion.
+          if (!this.hasSelectedPills() && !event.repeat) {
+            pill.setAttribute("selected", "selected");
+            break;
+          }
+          // Delete selected pills, handle focus and select another pill
+          // where applicable.
+          let focusType = event.key == "Delete" ? "next" : "previous";
+          this.removeSelectedPills(pill, focusType, true);
           break;
 
         case "ArrowLeft":
           if (pill.previousElementSibling) {
             pill.previousElementSibling.focus();
             this.checkKeyboardSelected(event, pill.previousElementSibling);
           }
           break;
@@ -2244,17 +2288,17 @@
           if (event.ctrlKey || event.metaKey) {
             copyEmailNewsAddress(pill);
           }
           break;
 
         case "x":
           if (event.ctrlKey || event.metaKey) {
             copyEmailNewsAddress(pill);
-            deleteAddressPill(pill);
+            this.removeSelectedPills(pill);
           }
           break;
       }
     }
 
     /**
      * Handle the selection and focus of the pill elements on mouse events.
      *
@@ -2293,17 +2337,20 @@
         if (this.hasAttribute("selected") && element.hasAttribute("selected")) {
           this.removeAttribute("selected");
           return;
         }
 
         this.setAttribute("selected", "selected");
         element.setAttribute("selected", "selected");
       } else if (!event.ctrlKey) {
+        // Non-modified navigation keys must select the target pill and deselect
+        // all others. Also some other keys like Backspace from rowInput.
         this.clearSelected();
+        element.setAttribute("selected", "selected");
       }
     }
 
     clearSelected() {
       for (let pill of this.getAllPills()) {
         pill.removeAttribute("selected");
       }
     }
@@ -2319,32 +2366,72 @@
         event.stopPropagation();
         return;
       }
 
       pill.startEditing();
     }
 
     /**
-     * When a "Delete" action is triggered, we need to check if other pills are
-     * currently selected and delete them all.
+     * Delete all selected pills and handle focus and selection smartly as needed.
      *
-     * @param {XULElement} pill - The mail-address-pill element.
+     * @param {Element} pill - The mail-address-pill element where Event fired.
+     * @param {("next"|"previous")} [focusType="next"] - How to move focus after
+     *   removing pills: try to focus one of the next siblings (for DEL etc.)
+     *   or one of the previous siblings (for BACKSPACE).
+     * @param {boolean} [select=false] - After deletion, whether to select the
+     *   focused pill where applicable.
      */
-    removePills(pill) {
-      // Store the addressing input that needs to receive focus before deleting
-      // the pill.
+    removeSelectedPills(pill, focusType = "next", select = false) {
+      // We'll look hard for an appropriate element to focus after the removal.
+      let focusElement = null;
+      // Get addressContainer and rowInput now as pill might be deleted later.
+      let addressContainer = pill.closest(".address-container");
       let rowInput = pill.rowInput;
-      for (let item of this.getAllSelectedPills()) {
-        item.remove();
+      let unselectedSourcePill = false;
+
+      if (pill.hasAttribute("selected")) {
+        // Find focus (1): Focused pill is selected and will be deleted;
+        // try nearest sibling, observing focusType direction.
+        focusElement = pill.getUnselectedSiblingPill(focusType);
+      } else {
+        // The source pill isn't selected; keep it focused ("satellite focus").
+        unselectedSourcePill = true;
+        focusElement = pill;
       }
 
-      rowInput.focus();
-      // Manually remove the pill in case it wasn't part of the selected array.
-      pill.remove();
+      // Remove selected pills.
+      let selectedPills = this.getAllSelectedPills();
+      for (let sPill of selectedPills) {
+        sPill.remove();
+      }
+
+      // Find focus (2): When deleting backwards, if no previous sibling found,
+      // this means that the first pill was deleted. Try the first remaining pill,
+      // but don't auto-select it because it's in the opposite direction.
+      if (!focusElement && focusType == "previous") {
+        focusElement = addressContainer.querySelector("mail-address-pill");
+      } else if (
+        select &&
+        focusElement &&
+        selectedPills.length == 1 &&
+        !unselectedSourcePill
+      ) {
+        // If select = true (DEL or BACKSPACE), and we found a pill to focus in
+        // round (1), and we have removed a single pill only, and it's not a
+        // case of "satellite focus" (see above):
+        // Conveniently select the nearest pill for rapid consecutive deletions.
+        focusElement.setAttribute("selected", "selected");
+      }
+      // Find focus (3): If all else fails (no pills left in addressContainer,
+      // or last pill deleted forwards): Focus rowInput.
+      if (!focusElement) {
+        focusElement = rowInput;
+      }
+      focusElement.focus();
 
       onRecipientsChanged();
       calculateHeaderHeight();
     }
 
     /**
      * Move the focus on the first pill from the same .address-container.
      *
@@ -2389,12 +2476,21 @@
     /**
      * Return all the selected pills currently available in the address area.
      *
      * @return {Array} Array of selected mail-address-pill elements.
      */
     getAllSelectedPills() {
       return this.querySelectorAll("mail-address-pill[selected]");
     }
+
+    /**
+     * Check if any pill in the addressing area is selected.
+     *
+     * @return {boolean} true if any pill is selected.
+     */
+    hasSelectedPills() {
+      return Boolean(this.querySelector("mail-address-pill[selected]"));
+    }
   }
 
   customElements.define("mail-recipients-area", MailRecipientsArea);
 }
--- a/mail/components/compose/content/addressingWidgetOverlay.js
+++ b/mail/components/compose/content/addressingWidgetOverlay.js
@@ -683,35 +683,25 @@ function copyEmailNewsAddress(element) {
  *   opened.
  */
 function cutEmailNewsAddress(element) {
   copyEmailNewsAddress(element);
   deleteAddressPill(element);
 }
 
 /**
- * Delete the selected pill/pills.
+ * Delete the selected pill(s).
  *
  * @param {XULElement} element - The element from which the context menu was
  *   opened.
  */
 function deleteAddressPill(element) {
-  // We need to store the input location before removing the pills.
-  let input = element
-    .closest(".address-container")
-    .querySelector(`input[is="autocomplete-input"][recipienttype]`);
-
-  for (let pill of document
-    .getElementById("recipientsContainer")
-    .getAllSelectedPills()) {
-    pill.remove();
-  }
-
-  input.focus();
-  onRecipientsChanged();
+  // element is the pill's <label>, get the pill.
+  let pill = element.closest("mail-address-pill");
+  document.getElementById("recipientsContainer").removeSelectedPills(pill);
 }
 
 /**
  * Handle the keypress event on the labels to show the container row
  * of an hidden recipient (Cc, Bcc, etc.).
  *
  * @param {Event} event - The DOM keypress event.
  * @param {XULelement} label - The clicked label to hide.
--- a/mail/test/browser/composition/browser_sendButton.js
+++ b/mail/test/browser/composition/browser_sendButton.js
@@ -25,17 +25,17 @@ var {
   click_tree_row,
   FAKE_SERVER_HOSTNAME,
   get_special_folder,
   wait_for_popup_to_open,
 } = ChromeUtils.import(
   "resource://testing-common/mozmill/FolderDisplayHelpers.jsm"
 );
 var {
-  clear_recipient,
+  clear_recipients,
   get_first_pill,
   close_compose_window,
   open_compose_new_mail,
   setup_msg_contents,
   toggle_recipient_type,
 } = ChromeUtils.import("resource://testing-common/mozmill/ComposeHelpers.jsm");
 var { wait_for_frame_load } = ChromeUtils.import(
   "resource://testing-common/mozmill/WindowHelpers.jsm"
@@ -97,24 +97,24 @@ add_task(function test_send_enabled_manu
   // On an empty window, Send must be disabled.
   check_send_commands_state(cwc, false);
 
   // On valid "To:" addressee input, Send must be enabled.
   setup_msg_contents(cwc, " recipient@fake.invalid ", "", "");
   check_send_commands_state(cwc, true);
 
   // When the addressee is not in To, Cc, Bcc or Newsgroup, disable Send again.
-  clear_recipient(cwc);
+  clear_recipients(cwc);
   cwc.click(cwc.eid("extraRecipientsLabel"));
   wait_for_popup_to_open(panel);
   cwc.click(cwc.eid("addr_reply"));
   setup_msg_contents(cwc, " recipient@fake.invalid ", "", "", "replyAddrInput");
   check_send_commands_state(cwc, false);
 
-  clear_recipient(cwc);
+  clear_recipients(cwc);
   check_send_commands_state(cwc, false);
 
   // Bug 1296535
   // Try some other invalid and valid recipient strings:
   // - random string that is no email.
   setup_msg_contents(cwc, " recipient@", "", "");
   check_send_commands_state(cwc, false);
 
@@ -133,35 +133,35 @@ add_task(function test_send_enabled_manu
   setup_msg_contents(
     cwc,
     "recipient@domain.invalid, info@somedomain.extension, name@incomplete",
     "",
     ""
   );
   check_send_commands_state(cwc, true);
 
-  clear_recipient(cwc);
+  clear_recipients(cwc);
   check_send_commands_state(cwc, false);
 
   // - a mailinglist in addressbook
   // Button is enabled without checking whether it contains valid addresses.
   let defaultAB = MailServices.ab.getDirectory("jsaddrbook://abook.sqlite");
   let ml = create_mailing_list("emptyList");
   defaultAB.addMailList(ml);
 
   setup_msg_contents(cwc, " emptyList", "", "");
   check_send_commands_state(cwc, true);
 
-  clear_recipient(cwc);
+  clear_recipients(cwc);
   check_send_commands_state(cwc, false);
 
   setup_msg_contents(cwc, "emptyList <list> ", "", "");
   check_send_commands_state(cwc, true);
 
-  clear_recipient(cwc);
+  clear_recipients(cwc);
   check_send_commands_state(cwc, false);
 
   // Show the extraRecipientsLabel in order to trigger the opening og the
   // extraRecipientsPanel.
   cwc.e("extraRecipientsLabel").removeAttribute("collapsed");
   cwc.click(cwc.eid("extraRecipientsLabel"));
   wait_for_popup_to_open(panel);
 
@@ -185,17 +185,17 @@ add_task(function test_send_enabled_pref
   identity.doCc = true;
   identity.doCcList = "Auto@recipient.invalid";
 
   // In that case the recipient is input, enabled Send.
   let cwc = open_compose_new_mail(); // compose controller
   check_send_commands_state(cwc, true);
 
   // Clear the CC list.
-  clear_recipient(cwc);
+  clear_recipients(cwc);
   // No other pill is there. Send should become disabled.
   check_send_commands_state(cwc, false);
 
   close_compose_window(cwc);
   identity.doCcList = "";
   identity.doCc = false;
 });
 
--- a/mail/test/browser/shared-modules/ComposeHelpers.jsm
+++ b/mail/test/browser/shared-modules/ComposeHelpers.jsm
@@ -12,17 +12,17 @@ this.EXPORTED_SYMBOLS = [
   "open_compose_with_forward",
   "open_compose_with_forward_as_attachments",
   "open_compose_with_edit_as_new",
   "open_compose_with_element_click",
   "open_compose_from_draft",
   "close_compose_window",
   "wait_for_compose_window",
   "setup_msg_contents",
-  "clear_recipient",
+  "clear_recipients",
   "get_first_pill",
   "toggle_recipient_type",
   "create_msg_attachment",
   "add_attachments",
   "add_cloud_attachments",
   "delete_attachment",
   "get_compose_body",
   "type_in_composer",
@@ -332,25 +332,26 @@ function setup_msg_contents(
   aCwc.type(aCwc.eid("msgSubject"), aSubj);
   aCwc.type(aCwc.eid("content-frame"), aBody);
 
   // Wait 1 second for the pill to be created.
   aCwc.sleep(1000);
 }
 
 /**
- * Remove the recipient by typing backspaces.
+ * Remove all recipients.
  *
  * @param aController    Compose window controller.
  */
-function clear_recipient(aController) {
+function clear_recipients(aController) {
   for (let pill of aController.window.document.querySelectorAll(
     "mail-address-pill"
   )) {
-    aController.keypress(new elib.Elem(pill), "VK_BACK_SPACE", {});
+    pill.toggleAttribute("selected", true);
+    aController.e("recipientsContainer").removeSelectedPills(pill);
   }
 }
 
 /**
  * Return the first available recipient pill.
  *
  * @param aController - Compose window controller.
  */