Bug 991965 - Use event.target, inhibit item selection if context menu is open draft
authorGeorge Pîrlea <george@dranov.ro>
Sat, 21 Jan 2017 12:21:51 +0000
changeset 464635 6ffe8384c6fcc107f3ad35efa5fb861fdf02558e
parent 464601 487a4e43eb9d1f04a5d8e3dd183fe38dbe105e1f
child 464636 d4bf14aa54ccb4d376ccb05538e75e8632555e2f
child 464637 3c3d22b1647175da881ea92feaea9c4bc67e883b
push id42392
push userbmo:george@dranov.ro
push dateSat, 21 Jan 2017 17:45:48 +0000
bugs991965
milestone53.0a1
Bug 991965 - Use event.target, inhibit item selection if context menu is open 1) In the Downloads Panel, the item with the hover effect applied to it was not always the actual 'selectedItem'. This was due to an error in how 'onDownloadMouseOver()' events were processed – 'aEvent.orginalTarget' doesn't take retargets into account, so hovering over an item would sometimes not trigger an actual selection. This is what the reporter of bug 991965 describes: removing an item from the download history fails to work "just because you didn't click in some magical place". ('selectedItem' was -1) Fixed by using 'aEvent.target' instead of 'aEvent.originalTarget'. 2) In the Downloads Panel, right-clicking an item opens a context menu that lets you act on the selected item. However, if you unintentionally hover over a different item when the context menu is open, the selection changes - you may, for example, 'Remove From History' a different, unintended item. To fix this, we inhibit item selection when a context menu is open for a download item. This commit DOES NOT include the necessary style changes. At this point, the selection UI is completely broken! MozReview-Commit-ID: DLIAFNcs33N
browser/components/downloads/content/downloads.js
browser/components/downloads/content/downloadsOverlay.xul
--- a/browser/components/downloads/content/downloads.js
+++ b/browser/components/downloads/content/downloads.js
@@ -718,16 +718,21 @@ const DownloadsView = {
   //// Functions handling download items in the list
 
   /**
    * Maximum number of items shown by the list at any given time.
    */
   kItemCountLimit: 5,
 
   /**
+   * Indicates whether there is an open contextMenu for a download item.
+   */
+  contextMenuOpen: false,
+
+  /**
    * Indicates whether we are still loading downloads data asynchronously.
    */
   loading: false,
 
   /**
    * Ordered array of all Download objects.  We need to keep this array because
    * only a limited number of items are shown at once, and if an item that is
    * currently visible is removed from the list, we might need to take another
@@ -995,30 +1000,56 @@ const DownloadsView = {
     }
 
     if (aEvent.keyCode == KeyEvent.DOM_VK_RETURN) {
       goDoCommand("downloadsCmd_doDefault");
     }
   },
 
   /**
+   * Event handlers to keep track of context menu state (open/closed) for
+   * download items.
+   */
+  onContextPopupShown(aEvent) {
+    // Ignore events raised by nested popups.
+    if (aEvent.target != aEvent.currentTarget) {
+      return;
+    }
+
+    DownloadsCommon.log("Context menu has shown.");
+    this.contextMenuOpen = true;
+  },
+
+  onContextPopupHidden(aEvent) {
+    // Ignore events raised by nested popups.
+    if (aEvent.target != aEvent.currentTarget) {
+      return;
+    }
+
+    DownloadsCommon.log("Context menu has hidden.");
+    this.contextMenuOpen = false;
+  },
+
+  /**
    * Mouse listeners to handle selection on hover.
    */
   onDownloadMouseOver(aEvent) {
-    if (aEvent.originalTarget.parentNode == this.richListBox) {
-      this.richListBox.selectedItem = aEvent.originalTarget;
+    if (!this.contextMenuOpen &&
+        aEvent.target.parentNode == this.richListBox) {
+      this.richListBox.selectedItem = aEvent.target;
     }
   },
 
   onDownloadMouseOut(aEvent) {
-    if (aEvent.originalTarget.parentNode == this.richListBox) {
+    if (!this.contextMenuOpen &&
+        aEvent.target.parentNode == this.richListBox) {
       // If the destination element is outside of the richlistitem, clear the
       // selection.
       let element = aEvent.relatedTarget;
-      while (element && element != aEvent.originalTarget) {
+      while (element && element != aEvent.target) {
         element = element.parentNode;
       }
       if (!element) {
         this.richListBox.selectedIndex = -1;
       }
     }
   },
 
--- a/browser/components/downloads/content/downloadsOverlay.xul
+++ b/browser/components/downloads/content/downloadsOverlay.xul
@@ -55,16 +55,18 @@
            orient="vertical"
            level="top"
            onpopupshown="DownloadsPanel.onPopupShown(event);"
            onpopuphidden="DownloadsPanel.onPopupHidden(event);">
       <!-- The following popup menu should be a child of the panel element,
            otherwise flickering may occur when the cursor is moved over the area
            of a disabled menu item that overlaps the panel.  See bug 492960. -->
       <menupopup id="downloadsContextMenu"
+                 onpopupshown="DownloadsView.onContextPopupShown(event);"
+                 onpopuphidden="DownloadsView.onContextPopupHidden(event);"
                  class="download-state">
         <menuitem command="downloadsCmd_pauseResume"
                   class="downloadPauseMenuItem"
                   label="&cmd.pause.label;"
                   accesskey="&cmd.pause.accesskey;"/>
         <menuitem command="downloadsCmd_pauseResume"
                   class="downloadResumeMenuItem"
                   label="&cmd.resume.label;"