597600 - Ignore old context menu and tap highlight events

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Benjamin Stover (:stechz)]

Sometimes the event queue can go:
1. tap something
2. pan around
3. content sends back a tap highlight
4. old area is highlighted even though user has panned

Comment 1

[Benjamin Stover (:stechz)]

Attachment: use lightweight transaction system

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 476436 [details] [diff] [review]
use lightweight transaction system

I think this is a good idea. I don't think of it as "transactions" though:
* Change transactionId to contextId (or messageId)

I don't think we need this for beta 1. If you disagree, please comment.

Comment 3

[Benjamin Stover (:stechz)]

Attachment: Ignore old context menu and tap highlight events

Comment 4

[Benjamin Stover (:stechz)]

Comment on attachment 477730 [details] [diff] [review]
Ignore old context menu and tap highlight events

Review comments addressed, refactored ContentTouchHelper:handleEvent

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 477730 [details] [diff] [review]
Ignore old context menu and tap highlight events

># HG changeset patch
># User Benjamin Stover <[e-mail]>
># Date 1285194840 25200
># Node ID 1e872bb2a2d0117a72974e6e4febda64e592f314
># Parent  059acf0068940e2c6f8e12796e768e4b4b0e6d65
>Bug 597600 - Ignore old context menu and tap highlight events r=mfinkle
>
>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
>--- a/chrome/content/browser-ui.js
>+++ b/chrome/content/browser-ui.js
>@@ -410,17 +410,16 @@ var BrowserUI = {
>     tabs.addEventListener("TabOpen", this, true);
> 
>     // listen content messages
>     messageManager.addMessageListener("DOMLinkAdded", this);
>     messageManager.addMessageListener("DOMTitleChanged", this);
>     messageManager.addMessageListener("DOMWillOpenModalDialog", this);
>     messageManager.addMessageListener("DOMWindowClose", this);
> 
>-    messageManager.addMessageListener("Browser:Highlight", this);
>     messageManager.addMessageListener("Browser:OpenURI", this);
>     messageManager.addMessageListener("Browser:SaveAs:Return", this);
> 
>     // listening mousedown for automatically dismiss some popups (e.g. larry)
>     window.addEventListener("mousedown", this, true);
> 
>     // listening escape to dismiss dialog on VK_ESCAPE
>     window.addEventListener("keypress", this, true);
>@@ -850,27 +849,19 @@ var BrowserUI = {
>           element.setAttribute("referrer", json.referrer);
>           DownloadsView._updateTime(element);
>           DownloadsView._updateStatus(element);
>         }
>         catch(e) {}
>         Services.obs.notifyObservers(download, "dl-done", null);
>         break;
> 
>-      case "Browser:Highlight":
>-        let rects = [];
>-        for (let i = 0; i < json.rects.length; i++) {
>-          let rect = json.rects[i];
>-          rects.push(new Rect(rect.left, rect.top, rect.width, rect.height));
>-        }
>-        TapHighlightHelper.show(rects);
>-        break;
>-
>       case "Browser:OpenURI":
>         Browser.addTab(json.uri, json.bringFront, Browser.selectedTab);
>+        break;
>     }
>   },
> 
>   supportsCommand : function(cmd) {
>     var isSupported = false;
>     switch (cmd) {
>       case "cmd_back":
>       case "cmd_forward":
>diff --git a/chrome/content/browser.js b/chrome/content/browser.js
>--- a/chrome/content/browser.js
>+++ b/chrome/content/browser.js
>@@ -1258,78 +1258,109 @@ const BrowserSearch = {
>       return aEngine.title == item.name;
>     });
>   }
> };
> 
> 
> /** Watches for mouse events in chrome and sends them to content. */
> const ContentTouchHandler = {
>+  // Use lightweight transactions so that old context menus and tap
>+  // highlights don't ever see the light of day.
>+  _messageId: 0,
>+
>   init: function init() {
>     document.addEventListener("TapDown", this, false);
>     document.addEventListener("TapUp", this, false);
>     document.addEventListener("TapSingle", this, false);
>     document.addEventListener("TapDouble", this, false);
>     document.addEventListener("TapLong", this, false);
>+
>     document.addEventListener("PanBegin", this, false);
>-
>-    document.addEventListener("PopupChanged", this._popupChanged.bind(this), false);
>+    document.addEventListener("PopupChanged", this, false);
>+    document.addEventListener("CancelTouchSequence", this, false);
> 
>     // Context menus have the following flow:
>     //   [parent] mousedown -> TapDown -> Browser:MouseDown
>     //   [child]  Browser:MouseDown -> contextmenu -> Browser:ContextMenu
>     //   [parent] Browser:ContextMenu -> ...* -> TapLong
>     //
>     // * = Here some time will elapse. Although we get the context menu we need
>     //     ASAP, we do not act on the context menu until we receive a LongTap.
>     //     This is so we can show the context menu as soon as we know it is
>     //     a long tap, without waiting for child process.
>     //
>     messageManager.addMessageListener("Browser:ContextMenu", this);
>+
>+    messageManager.addMessageListener("Browser:Highlight", this);
>   },
> 
>-  handleEvent: function handleEvent(ev) {
>+  handleEvent: function handleEvent(aEvent) {
>     // ignore content events we generate
>-    if (ev.target.localName == "browser")
>+    if (aEvent.target.localName == "browser")
>       return;
> 
>-    if (!this._targetIsContent(ev)) {
>-      TapHighlightHelper.hide();
>-      this._dispatchMouseEvent("Browser:MouseCancel");
>-      return;
>-    }
>-
>-    switch (ev.type) {
>-      case "TapDown":
>-        this.tapDown(ev.clientX, ev.clientY);
>+    switch (aEvent.type) {
>+      case "PanBegin":
>+      case "PopupChanged":
>+      case "CancelTouchSequence":
>+        this._clearPendingMessages();
>         break;
>-      case "TapUp":
>-        thisTapSingletapUp(ev.clientX, ev.clientY);
>-        break;
>-      case "TapSingle":
>-        this.tapSingle(ev.clientX, ev.clientY, ev.modifiers);
>-        break;
>-      case "TapDouble":
>-        this.tapDouble(ev.clientX1, ev.clientY1, ev.clientX2, ev.clientY2);
>-        break;
>-      case "TapLong":
>-        this.tapLong();
>-        break;
>-      case "PanBegin":
>-        this.panBegin();
>-        break;
>+
>+      default: {
>+        if (!this._targetIsContent(aEvent)) {
>+          // Received tap event on something that isn't for content.
>+          this._clearPendingMessages();
>+          break;
>+        }
>+
>+        switch (aEvent.type) {
>+          case "TapDown":
>+            this.tapDown(aEvent.clientX, aEvent.clientY);
>+            break;
>+          case "TapUp":
>+            this.tapUp(aEvent.clientX, aEvent.clientY);
>+            break;
>+          case "TapSingle":
>+            this.tapSingle(aEvent.clientX, aEvent.clientY, aEvent.modifiers);
>+            break;
>+          case "TapDouble":
>+            this.tapDouble(aEvent.clientX1, aEvent.clientY1, aEvent.clientX2, aEvent.clientY2);
>+            break;
>+          case "TapLong":
>+            this.tapLong();
>+            break;
>+        }
>+      }
>     }
>   },
> 
>   receiveMessage: function receiveMessage(aMessage) {
>-    this._contextMenu = { name: aMessage.name, json: aMessage.json, target: aMessage.target };
>+    if (aMessage.json.messageId != this._messageId)
>+      return;
>+
>+    switch (aMessage.name) {
>+      case "Browser:ContextMenu":
>+        // Long tap 
>+        this._contextMenu = { name: aMessage.name, json: aMessage.json, target: aMessage.target };
>+        break;
>+
>+      case "Browser:Highlight": {
>+        let rects = aMessage.json.rects.map(
>+          function(r) new Rect(r.left, r.top, r.width, r.height));
>+        TapHighlightHelper.show(rects);
>+        break;
>+      }
>+    }
>   },
> 
>-  _popupChanged: function _popupChanged() {
>-    TapHighlightHelper.hide(200);
>+  /** Invalidates any messages received from content that are sensitive to time. */
>+  _clearPendingMessages: function _clearPendingMessages() {
>+    this._messageId++;
>+    TapHighlightHelper.hide(0);
>     this._contextMenu = null;
>   },
> 
>   /**
>    * Check if the event concern the browser content
>    */
>   _targetIsContent: function _targetIsContent(aEvent) {
>     let target = aEvent.target;
>@@ -1337,17 +1368,22 @@ const ContentTouchHandler = {
>   },
> 
>   _dispatchMouseEvent: function _dispatchMouseEvent(aName, aX, aY, aModifiers) {
>     let aX = aX || 0;
>     let aY = aY || 0;
>     let aModifiers = aModifiers || null;
>     let browser = getBrowser();
>     let [x, y] = Browser.transformClientToBrowser(aX, aY);
>-    browser.messageManager.sendAsyncMessage(aName, { x: x, y: y, modifiers: aModifiers });
>+    browser.messageManager.sendAsyncMessage(aName, {
>+      x: x,
>+      y: y,
>+      modifiers: aModifiers,
>+      messageId: this._messageId
>+    });
>   },
> 
>   tapDown: function tapDown(aX, aY) {
>     // Ensure that the content process has gets an activate event
>     let browser = getBrowser();
>     let fl = browser.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader;
>     browser.focus();
>     try {
>@@ -1357,39 +1393,27 @@ const ContentTouchHandler = {
>     this._dispatchMouseEvent("Browser:MouseDown", aX, aY);
>   },
> 
>   tapUp: function tapUp(aX, aY) {
>     TapHighlightHelper.hide(200);
>     this._contextMenu = null;
>   },
> 
>-  panBegin: function panBegin() {
>-    TapHighlightHelper.hide(0);
>-    this._contextMenu = null;
>-
>-    this._dispatchMouseEvent("Browser:MouseCancel");
>-  },
>-
>   tapSingle: function tapSingle(aX, aY, aModifiers) {
>     TapHighlightHelper.hide(200);
>     this._contextMenu = null;
> 
>     // Cancel the mouse click if we are showing a context menu
>     if (!ContextHelper.popupState)
>       this._dispatchMouseEvent("Browser:MouseUp", aX, aY, aModifiers);
>-    this._dispatchMouseEvent("Browser:MouseCancel");
>   },
> 
>   tapDouble: function tapDouble(aX1, aY1, aX2, aY2) {
>-    TapHighlightHelper.hide();
>-    this._contextMenu = null;
>-
>-
>-    this._dispatchMouseEvent("Browser:MouseCancel");
>+    this._clearPendingMessages();
> 
>     const kDoubleClickRadius = 100;
> 
>     let maxRadius = kDoubleClickRadius * getBrowser().scale;
>     let dx = aX2 - aX1;
>     let dy = aY1 - aY2;
> 
>     if (dx*dx + dy*dy < maxRadius*maxRadius)
>diff --git a/chrome/content/content.js b/chrome/content/content.js
>--- a/chrome/content/content.js
>+++ b/chrome/content/content.js
>@@ -376,19 +376,21 @@ Content.prototype = {
> 
>       case "Browser:MouseDown": {
>         let element = elementFromPoint(x, y);
>         if (!element)
>           return;
> 
>         if (element.mozMatchesSelector("*:link,*:visited,*:link *,*:visited *,*[role=button],button,input,option,select,textarea,label")) {
>           let rects = getContentClientRects(element);
>-          sendAsyncMessage("Browser:Highlight", { rects: rects });
>+          sendAsyncMessage("Browser:Highlight", { rects: rects, messageId: json.messageId });
>         }
> 
>+        ContextHandler.messageId = json.messageId;
>+
>         let event = content.document.createEvent("PopupEvents");
>         event.initEvent("contextmenu", true, true);
>         element.dispatchEvent(event);
>         break;
>       }
> 
>       case "Browser:MouseUp": {
>         let element = elementFromPoint(x, y);
>@@ -702,16 +704,18 @@ var ContextHandler = {
> 
>       elem = elem.parentNode;
>     }
> 
>     for (let i = 0; i < this._types.length; i++)
>       if (this._types[i].handler(state, popupNode))
>         state.types.push(this._types[i].name);
> 
>+    state.messageId = this.messageId;
>+
>     sendAsyncMessage("Browser:ContextMenu", state);
>   },
> 
>   /**
>    * For add-ons to add new types and data to the ContextMenu message.
>    *
>    * @param aName A string to identify the new type.
>    * @param aHandler A function that takes a state object and a target element.

Comment 6

[Benjamin Stover (:stechz)]

Attachment: Ignore old context menu and tap highlight events

Comment 7

[Benjamin Stover (:stechz)]

Fixed bug where exiting context menu causes tap

Comment 8

[Benjamin Stover (:stechz)]

Attachment: part 2 onMouseUp stops long tap timer

Comment 9

[Benjamin Stover (:stechz)]

Pushed http://hg.mozilla.org/mobile-browser/rev/57560165c740

Comment 11

[Aakash Desai [:aakashd]]

verified FIXED on build:
Mozilla/5.0 (Android; Linux armv71; rv:2.0b6pre) Gecko/20100923 Namoroka/4.0b7pre Fennec/2.0b1pre