Bug 1337772 - Part 1 - Use mousedown instead of contextmenu to avoid showing the password autocomplete. r=MattN draft
authorJohann Hofmann <jhofmann@mozilla.com>
Fri, 07 Apr 2017 00:01:31 +0200
changeset 557475 e0b4f4f5d14407a831ca6efe4481f6c884682b39
parent 555310 b5d8b27a753725c1de41ffae2e338798f3b5cacd
child 557476 6133986c9e3c470f6e2fb3c5a172a82c0f07fd8a
push id52741
push userbmo:jhofmann@mozilla.com
push dateThu, 06 Apr 2017 22:02:16 +0000
reviewersMattN
bugs1337772
milestone55.0a1
Bug 1337772 - Part 1 - Use mousedown instead of contextmenu to avoid showing the password autocomplete. r=MattN MozReview-Commit-ID: EUZ1f6Qdm0c
toolkit/components/passwordmgr/LoginManagerContent.jsm
toolkit/components/passwordmgr/test/browser/browser.ini
toolkit/components/satchel/nsFormFillController.cpp
toolkit/components/satchel/nsFormFillController.h
--- a/toolkit/components/passwordmgr/LoginManagerContent.jsm
+++ b/toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ -5,17 +5,17 @@
 "use strict";
 
 this.EXPORTED_SYMBOLS = [ "LoginManagerContent",
                           "LoginFormFactory",
                           "UserAutoCompleteResult" ];
 
 const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
 const PASSWORD_INPUT_ADDED_COALESCING_THRESHOLD_MS = 1;
-const AUTOCOMPLETE_AFTER_CONTEXTMENU_THRESHOLD_MS = 400;
+const AUTOCOMPLETE_AFTER_RIGHT_CLICK_THRESHOLD_MS = 400;
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
 Cu.import("resource://gre/modules/InsecurePasswordUtils.jsm");
 Cu.import("resource://gre/modules/Promise.jsm");
 Cu.import("resource://gre/modules/Preferences.jsm");
 Cu.import("resource://gre/modules/Timer.jsm");
@@ -36,17 +36,17 @@ XPCOMUtils.defineLazyServiceGetter(this,
 
 XPCOMUtils.defineLazyGetter(this, "log", () => {
   let logger = LoginHelper.createLogger("LoginManagerContent");
   return logger.log.bind(logger);
 });
 
 // These mirror signon.* prefs.
 var gEnabled, gAutofillForms, gStoreWhenAutocompleteOff;
-var gLastContextMenuEventTimeStamp = Number.NEGATIVE_INFINITY;
+var gLastRightClickTimeStamp = Number.NEGATIVE_INFINITY;
 
 var observer = {
   QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
                                           Ci.nsIFormSubmitObserver,
                                           Ci.nsIWebProgressListener,
                                           Ci.nsIDOMEventListener,
                                           Ci.nsISupportsWeakReference]),
 
@@ -124,20 +124,23 @@ var observer = {
 
     switch (aEvent.type) {
       // Only used for username fields.
       case "focus": {
         LoginManagerContent._onUsernameFocus(aEvent);
         break;
       }
 
-      case "contextmenu": {
-        // Date.now() is used instead of event.timeStamp since
-        // dom.event.highrestimestamp.enabled isn't true on all channels yet.
-        gLastContextMenuEventTimeStamp = Date.now();
+      case "mousedown": {
+        if (aEvent.button == 2) {
+          // Date.now() is used instead of event.timeStamp since
+          // dom.event.highrestimestamp.enabled isn't true on all channels yet.
+          gLastRightClickTimeStamp = Date.now();
+        }
+
         break;
       }
 
       default: {
         throw new Error("Unexpected event");
       }
     }
   },
@@ -570,43 +573,34 @@ var LoginManagerContent = {
     }
 
     if (this._isLoginAlreadyFilled(focusedField)) {
       log("_onUsernameFocus: Already filled");
       return;
     }
 
     /*
-     * A `focus` event is fired before a `contextmenu` event if a user right-clicks into an
+     * A `mousedown` event is fired before the `focus` event if the user right clicks into an
      * unfocused field. In that case we don't want to show both autocomplete and a context menu
-     * overlapping so we spin the event loop to see if a `contextmenu` event is coming next. If no
-     * `contextmenu` event was seen and the focused field is still focused by the form fill
-     * controller then show the autocomplete popup.
+     * overlapping so we check against the timestamp that was set by the `mousedown` event if the
+     * button code indicated a right click.
+     * We use a timestamp instead of a bool to avoid complexity when dealing with multiple input
+     * forms and the fact that a mousedown into an already focused field does not trigger another focus.
      * Date.now() is used instead of event.timeStamp since dom.event.highrestimestamp.enabled isn't
      * true on all channels yet.
      */
-    let timestamp = Date.now();
-    setTimeout(function maybeOpenAutocompleteAfterFocus() {
-      // Even though the `focus` event happens first in testing, I don't want to
-      // rely on that since it was supposedly in the opposite order before. Use
-      // the absolute value to handle both orders.
-      let timeDiff = Math.abs(gLastContextMenuEventTimeStamp - timestamp);
-      if (timeDiff < AUTOCOMPLETE_AFTER_CONTEXTMENU_THRESHOLD_MS) {
-        log("Not opening autocomplete after focus since a context menu was opened within",
-            timeDiff, "ms");
-        return;
-      }
+    let timeDiff = Date.now() - gLastRightClickTimeStamp;
+    if (timeDiff < AUTOCOMPLETE_AFTER_RIGHT_CLICK_THRESHOLD_MS) {
+      log("Not opening autocomplete after focus since a context menu was opened within",
+          timeDiff, "ms");
+      return;
+    }
 
-      if (this._formFillService.focusedInput == focusedField) {
-        log("maybeOpenAutocompleteAfterFocus: Opening the autocomplete popup");
-        this._formFillService.showPopup();
-      } else {
-        log("maybeOpenAutocompleteAfterFocus: FormFillController has a different focused input");
-      }
-    }.bind(this), 0);
+    log("maybeOpenAutocompleteAfterFocus: Opening the autocomplete popup");
+    this._formFillService.showPopup();
   },
 
   /**
    * Listens for DOMAutoComplete and blur events on an input field.
    */
   onUsernameInput(event) {
     if (!event.isTrusted)
       return;
@@ -1257,17 +1251,17 @@ var LoginManagerContent = {
             this._formFillService.showPopup();
           }
         }
       }
 
       if (usernameField) {
         log("_fillForm: Attaching event listeners to usernameField");
         usernameField.addEventListener("focus", observer);
-        usernameField.addEventListener("contextmenu", observer);
+        usernameField.addEventListener("mousedown", observer);
       }
 
       Services.obs.notifyObservers(form.rootElement, "passwordmgr-processed-form", null);
     }
   },
 
   /**
    * Given a field, determine whether that field was last filled as a username
--- a/toolkit/components/passwordmgr/test/browser/browser.ini
+++ b/toolkit/components/passwordmgr/test/browser/browser.ini
@@ -35,17 +35,16 @@ support-files =
   subtst_notifications_1.html
   subtst_notifications_8.html
 [browser_capture_doorhanger_window_open.js]
 support-files =
   subtst_notifications_11.html
   subtst_notifications_11_popup.html
 skip-if = os == "linux" # Bug 1312981, bug 1313136
 [browser_context_menu_autocomplete_interaction.js]
-skip-if = asan || (os == 'linux') # disabled on asan and linux * (see bug 1337772)
 [browser_username_select_dialog.js]
 support-files =
   subtst_notifications_change_p.html
 [browser_DOMFormHasPassword.js]
 [browser_DOMInputPasswordAdded.js]
 [browser_exceptions_dialog.js]
 [browser_formless_submit_chrome.js]
 [browser_hasInsecureLoginForms.js]
--- a/toolkit/components/satchel/nsFormFillController.cpp
+++ b/toolkit/components/satchel/nsFormFillController.cpp
@@ -36,17 +36,16 @@
 #include "nsToolkitCompsCID.h"
 #include "nsEmbedCID.h"
 #include "nsIDOMNSEditableElement.h"
 #include "nsContentUtils.h"
 #include "nsILoadContext.h"
 #include "nsIFrame.h"
 #include "nsIScriptSecurityManager.h"
 #include "nsFocusManager.h"
-#include "nsThreadUtils.h"
 
 using namespace mozilla;
 using namespace mozilla::dom;
 using mozilla::ErrorResult;
 
 NS_IMPL_CYCLE_COLLECTION(nsFormFillController,
                          mController, mLoginManager, mFocusedPopup, mDocShells,
                          mPopups, mLastListener, mLastFormAutoComplete)
@@ -68,21 +67,21 @@ NS_IMPL_CYCLE_COLLECTING_RELEASE(nsFormF
 
 nsFormFillController::nsFormFillController() :
   mFocusedInput(nullptr),
   mFocusedInputNode(nullptr),
   mListNode(nullptr),
   // The amount of time a context menu event supresses showing a
   // popup from a focus event in ms. This matches the threshold in
   // toolkit/components/passwordmgr/LoginManagerContent.jsm.
-  mFocusAfterContextMenuThreshold(400),
+  mFocusAfterRightClickThreshold(400),
   mTimeout(50),
   mMinResultsForPopup(1),
   mMaxRows(0),
-  mLastContextMenuEventTimeStamp(TimeStamp()),
+  mLastRightClickTimeStamp(TimeStamp()),
   mDisableAutoComplete(false),
   mCompleteDefaultIndex(false),
   mCompleteSelectedIndex(false),
   mForceComplete(false),
   mSuppressOnInput(false)
 {
   mController = do_GetService("@mozilla.org/autocomplete/controller;1");
   MOZ_ASSERT(mController);
@@ -952,19 +951,16 @@ nsFormFillController::HandleEvent(nsIDOM
   }
   if (type.EqualsLiteral("compositionend")) {
     NS_ASSERTION(mController, "should have a controller!");
     if (mController && mFocusedInput)
       mController->HandleEndComposition();
     return NS_OK;
   }
   if (type.EqualsLiteral("contextmenu")) {
-    // Set timestamp to check for a recent contextmenu
-    // call in Focus(), to avoid showing the popup.
-    mLastContextMenuEventTimeStamp = TimeStamp::Now();
     if (mFocusedPopup)
       mFocusedPopup->ClosePopup();
     return NS_OK;
   }
   if (type.EqualsLiteral("pagehide")) {
 
     nsCOMPtr<nsIDocument> doc = do_QueryInterface(
       aEvent->InternalDOMEvent()->GetTarget());
@@ -1038,61 +1034,54 @@ nsFormFillController::MaybeStartControll
     isPwmgrInput = true;
   }
 
   if (isPwmgrInput || hasList || autocomplete) {
     StartControllingInput(aInput);
   }
 }
 
-void
-nsFormFillController::FocusEventDelayedCallback(nsIFormControl* aFormControl)
-{
-  nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(mFocusedInputNode);
-
-  if (!formControl || formControl != aFormControl ||
-      formControl->ControlType() != NS_FORM_INPUT_PASSWORD) {
-    return;
-  }
-
-  // If we have not seen a context menu call yet, just show the popup.
-  if (mLastContextMenuEventTimeStamp.IsNull()) {
-   ShowPopup();
-   return;
-  }
-
-  uint64_t timeDiff = fabs((TimeStamp::Now() - mLastContextMenuEventTimeStamp).ToMilliseconds());
-  // If this focus doesn't follow a contextmenu event within our specified
-  // threshold then show the autocomplete popup for all password fields.
-  // This is done to avoid showing both the context menu and the popup
-  // at the same time. The threshold should be a low amount of time that
-  // makes it impossible for the user to accidentally trigger this condition.
-  if (timeDiff > mFocusAfterContextMenuThreshold) {
-   ShowPopup();
-  }
-}
-
 nsresult
 nsFormFillController::Focus(nsIDOMEvent* aEvent)
 {
   nsCOMPtr<nsIDOMHTMLInputElement> input = do_QueryInterface(
     aEvent->InternalDOMEvent()->GetTarget());
   MaybeStartControllingInput(input);
 
   // Bail if we didn't start controlling the input.
   if (!mFocusedInputNode) {
     return NS_OK;
   }
 
 #ifndef ANDROID
   nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(mFocusedInputNode);
   MOZ_ASSERT(formControl);
 
-  NS_DispatchToMainThread(NewRunnableMethod<nsCOMPtr<nsIFormControl>>(
-      this, &nsFormFillController::FocusEventDelayedCallback, formControl));
+  // If this focus doesn't follow a right click within our specified
+  // threshold then show the autocomplete popup for all password fields.
+  // This is done to avoid showing both the context menu and the popup
+  // at the same time.
+  // We use a timestamp instead of a bool to avoid complexity when dealing with
+  // multiple input forms and the fact that a mousedown into an already focused
+  // field does not trigger another focus.
+
+  if (formControl->ControlType() != NS_FORM_INPUT_PASSWORD) {
+    return NS_OK;
+  }
+
+  // If we have not seen a right click yet, just show the popup.
+  if (mLastRightClickTimeStamp.IsNull()) {
+    ShowPopup();
+    return NS_OK;
+  }
+
+  uint64_t timeDiff = (TimeStamp::Now() - mLastRightClickTimeStamp).ToMilliseconds();
+  if (timeDiff > mFocusAfterRightClickThreshold) {
+    ShowPopup();
+  }
 #endif
 
   return NS_OK;
 }
 
 nsresult
 nsFormFillController::KeyPress(nsIDOMEvent* aEvent)
 {
@@ -1214,16 +1203,25 @@ nsFormFillController::MouseDown(nsIDOMEv
 
   nsCOMPtr<nsIDOMHTMLInputElement> targetInput = do_QueryInterface(
     aEvent->InternalDOMEvent()->GetTarget());
   if (!targetInput)
     return NS_OK;
 
   int16_t button;
   mouseEvent->GetButton(&button);
+
+  // In case of a right click we set a timestamp that
+  // will be checked in Focus() to avoid showing
+  // both contextmenu and popup at the same time.
+  if (button == 2) {
+    mLastRightClickTimeStamp = TimeStamp::Now();
+    return NS_OK;
+  }
+
   if (button != 0)
     return NS_OK;
 
   return ShowPopup();
 }
 
 NS_IMETHODIMP
 nsFormFillController::ShowPopup()
--- a/toolkit/components/satchel/nsFormFillController.h
+++ b/toolkit/components/satchel/nsFormFillController.h
@@ -25,17 +25,16 @@
 // X.h defines KeyPress
 #ifdef KeyPress
 #undef KeyPress
 #endif
 
 class nsFormHistory;
 class nsINode;
 class nsPIDOMWindowOuter;
-class nsIFormControl;
 
 class nsFormFillController final : public nsIFormFillController,
                                    public nsIAutoCompleteInput,
                                    public nsIAutoCompleteSearch,
                                    public nsIDOMEventListener,
                                    public nsIFormAutoCompleteObserver,
                                    public nsIMutationObserver
 {
@@ -83,18 +82,16 @@ protected:
   inline nsPIDOMWindowOuter *GetWindowForDocShell(nsIDocShell *aDocShell);
   inline int32_t GetIndexOfDocShell(nsIDocShell *aDocShell);
 
   void MaybeRemoveMutationObserver(nsINode* aNode);
 
   void RemoveForDocument(nsIDocument* aDoc);
   bool IsEventTrusted(nsIDOMEvent *aEvent);
 
-  void FocusEventDelayedCallback(nsIFormControl* formControl);
-
   // members //////////////////////////////////////////
 
   nsCOMPtr<nsIAutoCompleteController> mController;
   nsCOMPtr<nsILoginManager> mLoginManager;
   nsIDOMHTMLInputElement* mFocusedInput;
   nsINode* mFocusedInputNode;
 
   // mListNode is a <datalist> element which, is set, has the form fill controller
@@ -111,21 +108,21 @@ protected:
 
   // This is cleared by StopSearch().
   nsCOMPtr<nsIFormAutoComplete> mLastFormAutoComplete;
   nsString mLastSearchString;
 
   nsDataHashtable<nsPtrHashKey<const nsINode>, bool> mPwmgrInputs;
   nsDataHashtable<nsPtrHashKey<const nsINode>, bool> mAutofillInputs;
 
-  uint16_t mFocusAfterContextMenuThreshold;
+  uint16_t mFocusAfterRightClickThreshold;
   uint32_t mTimeout;
   uint32_t mMinResultsForPopup;
   uint32_t mMaxRows;
-  mozilla::TimeStamp mLastContextMenuEventTimeStamp;
+  mozilla::TimeStamp mLastRightClickTimeStamp;
   bool mDisableAutoComplete;
   bool mCompleteDefaultIndex;
   bool mCompleteSelectedIndex;
   bool mForceComplete;
   bool mSuppressOnInput;
 };
 
 #endif // __nsFormFillController__