597286 - Context popups are quirky

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Benjamin Stover (:stechz)]

Sometimes context popups will come up after you've moved on to panning or zooming, or even gone to the awesomebar. The wait time for context menus is not always predictable.

For instance:
1. Go to news.google.com
2. Long tap on upper left image
3. Click on awesome screen

Sometimes the context menu for the image will come up in the awesome screen.

Comment 1

[Benjamin Stover (:stechz)]

Attachment: part 1: fix Util.Timeout bugs

Comment 2

[Benjamin Stover (:stechz)]

Attachment: part 2: DragData no longer needs owner

Comment 3

[Benjamin Stover (:stechz)]

Attachment: part 3: Drag move and drag end no longer need to be passed parameters

Comment 4

[Benjamin Stover (:stechz)]

Attachment: Cancel pending events properly lets go of mouse movements for MouseModule

Comment 5

[Benjamin Stover (:stechz)]

Attachment: part 5: Simplify click timeouts

Comment 6

[Benjamin Stover (:stechz)]

Attachment: part 6: MouseModule generates tap events instead of using clunky clickers

Comment 7

[Benjamin Stover (:stechz)]

Attachment: part 7: Separate panning from tapping and MouseModule no longer cares about whether something is content

Comment 8

[Benjamin Stover (:stechz)]

Attachment: part 8: Create new LongTap event and use it to bring up content context menus

Comment 9

[Benjamin Stover (:stechz)]

Phew!

So, I split this up into several patches because it changes InputHandler rather significantly. I've found MouseModule to be way too Fennec specific. We could make it much more generic by sending our own custom tap and touch events, which could later be replaced by the platform if we so chose. These are also much easier to use for extensions, than our customclickers.

This was all helpful for ContextHelper because long taps are now a first-class member of MouseModule. No more finding clickers to call the magic function "panBegin" and no more context helpers popping up long after you've begun panning or released your finger from the screen.

Comment 10

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

Comment on attachment 476130 [details] [diff] [review]
part 3: Drag move and drag end no longer need to be passed parameters


>   /** Callback for kinetic scroller. */
>   _kineticStop: function _kineticStop() {
>-    let dragData = this._dragData;
>-    if (!dragData.dragging)
>-      this._doDragStop(0, 0, true);
>+    this._dragger.dragStop(0, 0, this._targetScrollInterface);
>+    let event = document.createEvent("Events");
>+    event.initEvent("PanFinished", true, false);
>+    document.dispatchEvent(event);

As we talked about, you probably want to keep the | if (!dragData.dragging) | check to keep from stopping kinetic while panning

r+ with nits

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Stechz if we want some kind of touch event why are we not using https://developer.mozilla.org/en/DOM/Touch_events?

Comment 12

[Matt Brubeck (:mbrubeck)]

Comment on attachment 476133 [details] [diff] [review]
part 6: MouseModule generates tap events instead of using clunky clickers

>+ * centered at the mousedown position). Custom touch events are dispatched

Minor nit: Let's call them "tap events", since as Vivien pointed out "touch
events" already have a slightly different meaning.

In repsonse to comment 11, I don't think we should both with the MozTouch
events in this patch - we should wait until we have a proper implementation
(including MozTouchMove) and can expose them to content.  Things like
SingleTap and DoubleTap don't fit in the low-level touch event model.

>+   * and initiating a drag if we have said scrollable element.  The * mousedown
>+   * event is entirely swallowed but is saved for later redispatching,

Left an extra * before mousedown.  Also, is this comment accurate?

>+      // Pan has begun
>+      if (!oldIsPan) {
>+        let event = document.createEvent("Events");
>+        event.initEvent("PanBegin", true, false);
>+        event.clientX = aEvent.clientX;
>+        event.clientY = aEvent.clientY;
>+      }

PanBegin doesn't need clientX/Y, right?

>+        // Let everyone know when mousemove begins a pan
>+        if (!oldIsPan && dragData.isPan()) {
>+          let event = document.createEvent("Events");
>+          event.initEvent("PanBegin", true, false);
>+          event.clientX = aEvent.clientX;
>+          event.clientY = aEvent.clientY;
>+          aEvent.target.dispatchEvent(event);
>+        }

Same here.

Comment 13

[Benjamin Stover (:stechz)]

We *could* replace our TapUp and TapDown with MozTouch* I think?I would like to see the other tap events (singletap, doubletap, long tap) to move into platform sometime. They seem like the kind of touch events that everyone could use.  We would need to take panning along with it so that a pan never ever results in a tap.

Comment 14

[Matt Brubeck (:mbrubeck)]

Comment on attachment 476136 [details] [diff] [review]
part 8: Create new LongTap event and use it to bring up content context menus

>+const kLongTapWait = 700;

I think we should change this to 500 now that the highlight delay is gone (also mentioned in another review in this series).

>+++ b/chrome/content/browser.js        Thu Sep 16 18:05:14 2010 -0700

>+    document.addEventListener("PopupChanged", this._popupChanged.bind(this), false);

Why not addEventListener("PopupChanged", this, false) ?

>   tapUp: function tapUp(aX, aY) {
>     TapHighlightHelper.hide(200);
>+    this._contextMenu = null;
>   },
> 
>   panBegin: function panBegin() {
>     TapHighlightHelper.hide(200);
>+    this._contextMenu = null;
> 
>     this._dispatchMouseEvent("Browser:MouseCancel");
>   },
> 
>   singleTap: function singleTap(aX, aY, aModifiers) {
>     TapHighlightHelper.hide(200);
>+    this._contextMenu = null;

I'm tempted to add something like this to handleEvent instead:

  if (type != "TapDown" && type != "LongTap")
    this._contextMenu = null;

What do you think?

>+      if (ContextHelper.showPopup(this._contextMenu)) {
>+        // Stop all input sequences
>+        ih.cancelPending();
>+      }

Braces aren't needed.

Looks good, although the new event/message flow took me a while to puzzle out.  Could you add a comment near the new addEventListener calls, explaining how it works?  For the record, the order of events is:

mousedown -> TapDown -> Browser:MouseDown -> contextmenu -> Browser:ContextMenu -> LongTap

Comment 16

[u367160]

ok, latest code seems have some regression, now context menu opens even with click not taps

Comment 17

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Issue occurs on the Maemo as well : 
Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b7pre)Gecko/20100921 Firefox/4.0b7pre Fennec/2.0b1pre

It appears that the short tap requires a .25 sec tap or so and anything longer will cause the context menu to appear.

Comment 19

[Benjamin Stover (:stechz)]

Attachment: part 4: Context popups are quirky - Use cancel pending where appropriate

Comment 20

[Benjamin Stover (:stechz)]

Comment on attachment 477319 [details] [diff] [review]
part 4: Context popups are quirky - Use cancel pending where appropriate

This no longer emulates mouseup on cancelpending, because we don't want to do a TapUp event for this case.

Comment 21

[Benjamin Stover (:stechz)]

> I'm tempted to add something like this to handleEvent instead:
> 
>   if (type != "TapDown" && type != "LongTap")
>     this._contextMenu = null;
> 
> What do you think?

Then I'd probably want to do something similar for TapHighlight too... I agree this code could be better, but this feels like the wrong solution.

Comment 22

[Matt Brubeck (:mbrubeck)]

Comment on attachment 477319 [details] [diff] [review]
part 4: Context popups are quirky - Use cancel pending where appropriate

According to interdiff, this is unchanged from attachment 476131 [details] [diff] [review].  Did you forget to refresh before re-exporting?

Comment 23

[Benjamin Stover (:stechz)]

Interdiff is wrong then. Before, there were changes to cancelPending in MouseModule that causes a "TapUp" event to occur. This may trigger a click, which is never what we want with a cancelPending.

Comment 24

[Matt Brubeck (:mbrubeck)]

Ah, okay.  The new version looks good, then.

Comment 25

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

Naming suggestions... shocking, I know. Since we are making "TapXxx" events, I suggest following through with it, based on "mousexxx" or "TabXxx:
"SingleTap" -> "TapSingle"
"DoubleTap" -> "TapDoble"
"LongTap" -> "TapLong"

I know we could agree that "SingleTap", "DoubleTap" are analogous to "click", "dblclick". However, we are making a family of "TapXxx" events and they fit together better, imo.

Comment 26

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

I am eager to see how these patches and the patches in bug 597547 combine to form a new system of input handling.

Comment 27

[Benjamin Stover (:stechz)]

Attachment: , part 9: Fix interaction between mousemodule and gesturemodule

Comment 28

[Benjamin Stover (:stechz)]

Comment on attachment 477629 [details] [diff] [review]
, part 9: Fix interaction between mousemodule and gesturemodule

There were strange things happening when you attempted to pinch. This fixes those problems.

1. don't grab until pan begins (irrelevant when input handler goes away)
2. watch for mozmagnifygesturestart in mousemodule so that when pinch begins we don't go anywhere (again irrelevant when inputhandler goes away)
3. use doDragStop in cancelpending, don't just stop dragging (in retrospect, this is maybe equivalent to before I think, so I guess we could remove it)

Comment 29

[Benjamin Stover (:stechz)]

Attachment: part 1: fix Util.Timeout bugs

Comment 30

[Benjamin Stover (:stechz)]

Attachment: part 2: DragData no longer needs owner

Comment 31

[Benjamin Stover (:stechz)]

Attachment: part 3: Drag move and drag end no longer need to be passed parameters

Comment 32

[Benjamin Stover (:stechz)]

Attachment: part 4: Use cancel pending where appropriate

Comment 33

[Benjamin Stover (:stechz)]

Attachment: part 5: Simplify click timeouts

Comment 34

[Benjamin Stover (:stechz)]

Attachment: part 6: MouseModule generates tap events instead of using clunky clickers

Comment 35

[Benjamin Stover (:stechz)]

Attachment: part 7: Separate panning from tapping and MouseModule no longer cares about whether something is content

Comment 36

[Benjamin Stover (:stechz)]

Attachment: part 8: Create new longtap event and use it to bring up context menus

Comment 37

[Benjamin Stover (:stechz)]

Attachment: part 9: Fix interaction between mousemodule and gesturemodule

Comment 38

[Benjamin Stover (:stechz)]

Pushed http://hg.mozilla.org/mobile-browser/rev/5ab8842823a7

Comment 39

[Aakash Desai [:aakashd]]

I've been playing around with the context menu and haven't been able to break it yet. verified FIXED on build:

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


I'll keep on hacking on it though.