Closed Bug 597286 Opened 9 years ago Closed 9 years ago

Context popups are quirky

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(fennec2.0b2+)

VERIFIED FIXED
Tracking Status
fennec 2.0b2+ ---

People

(Reporter: stechz, Assigned: stechz)

References

Details

(Whiteboard: [fennec-checkin-postb1])

Attachments

(9 files, 10 obsolete files)

1.22 KB, patch
stechz
: review+
Details | Diff | Splinter Review
1.60 KB, patch
stechz
: review+
Details | Diff | Splinter Review
7.95 KB, patch
stechz
: review+
Details | Diff | Splinter Review
2.46 KB, patch
stechz
: review+
Details | Diff | Splinter Review
5.72 KB, patch
stechz
: review+
Details | Diff | Splinter Review
19.45 KB, patch
stechz
: review+
Details | Diff | Splinter Review
15.96 KB, patch
stechz
: review+
Details | Diff | Splinter Review
11.68 KB, patch
stechz
: review+
Details | Diff | Splinter Review
3.22 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
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.
Attached patch part 1: fix Util.Timeout bugs (obsolete) — Splinter Review
Attached patch part 5: Simplify click timeouts (obsolete) — Splinter Review
Attachment #476128 - Flags: review?(mark.finkle)
Attachment #476129 - Flags: review?(mark.finkle)
Attachment #476130 - Flags: review?(mark.finkle)
Attachment #476131 - Flags: review?(mbrubeck)
Attachment #476132 - Flags: review?(mbrubeck)
Attachment #476133 - Flags: review?(mbrubeck)
Attachment #476134 - Flags: review?(mbrubeck)
Attachment #476136 - Flags: review?(21)
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.
Attachment #476128 - Flags: review?(mark.finkle) → review+
Attachment #476129 - Flags: review?(mark.finkle) → review+
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
Attachment #476130 - Flags: review?(mark.finkle) → review+
Attachment #476131 - Flags: review?(mbrubeck) → review+
Attachment #476132 - Flags: review?(mbrubeck) → review+
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.
Attachment #476133 - Flags: review?(mbrubeck) → review+
Attachment #476134 - Flags: review?(mbrubeck) → review+
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.
Blocks: 597547
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
Attachment #476136 - Flags: review?(21) → review+
OS: Linux → All
Hardware: x86 → All
Whiteboard: [fennec-checkin-postb1]
Assignee: nobody → webapps
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0b2+
Duplicate of this bug: 594071
ok, latest code seems have some regression, now context menu opens even with click not taps
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.
Duplicate of this bug: 598342
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.
Attachment #477319 - Flags: review?(mbrubeck)
Attachment #476131 - Attachment is obsolete: true
> 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.
Attachment #477319 - Attachment description: Context popups are quirky - Cancel pending events properly lets go of mouse movements for MouseModule [ → Context popups are quirky - Use cancel pending where appropriate
Attachment #476128 - Attachment description: fix Util.Timeout bugs → part 1: fix Util.Timeout bugs
Attachment #476129 - Attachment description: DragData no longer needs owner → part 2: DragData no longer needs owner
Attachment #476130 - Attachment description: Drag move and drag end no longer need to be passed parameters → part 3: Drag move and drag end no longer need to be passed parameters
Attachment #476132 - Attachment description: Simplify click timeouts → part 5: Simplify click timeouts
Attachment #477319 - Attachment description: Context popups are quirky - Use cancel pending where appropriate → part 4: Context popups are quirky - Use cancel pending where appropriate
Attachment #476133 - Attachment description: MouseModule generates tap events instead of using clunky clickers → part 6: MouseModule generates tap events instead of using clunky clickers
Attachment #476134 - Attachment description: Separate panning from tapping and MouseModule no longer cares about whether something is content → part 7: Separate panning from tapping and MouseModule no longer cares about whether something is content
Attachment #476136 - Attachment description: Create new LongTap event and use it to bring up content context menus → part 8: Create new LongTap event and use it to bring up content context menus
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?
Attachment #477319 - Flags: review?(mbrubeck) → review+
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.
Ah, okay.  The new version looks good, then.
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.
I am eager to see how these patches and the patches in bug 597547 combine to form a new system of input handling.
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)
Attachment #477629 - Flags: review?(mbrubeck)
Attachment #477319 - Attachment is obsolete: true
Attachment #477629 - Attachment is obsolete: true
Attachment #477629 - Flags: review?(mbrubeck)
Attachment #476128 - Attachment is obsolete: true
Attachment #476129 - Attachment is obsolete: true
Attachment #476130 - Attachment is obsolete: true
Attachment #476132 - Attachment is obsolete: true
Attachment #476133 - Attachment is obsolete: true
Attachment #476134 - Attachment is obsolete: true
Attachment #476136 - Attachment is obsolete: true
Attachment #477702 - Attachment description: , part 1: fix Util.Timeout bugs [ → part 1: fix Util.Timeout bugs
Attachment #477702 - Attachment is obsolete: true
Attachment #477702 - Attachment is obsolete: false
Attachment #477702 - Flags: review+
Attachment #477703 - Attachment description: , part 2: DragData no longer needs owner [ → part 2: DragData no longer needs owner
Attachment #477703 - Flags: review+
Attachment #477704 - Attachment description: , part 3: Drag move and drag end no longer need to be passed parameters [ → part 3: Drag move and drag end no longer need to be passed parameters
Attachment #477704 - Flags: review+
Attachment #477705 - Attachment description: , part 4: Use cancel pending where appropriate [ → part 4: Use cancel pending where appropriate
Attachment #477705 - Flags: review+
Attachment #477706 - Attachment description: , part 5: Simplify click timeouts [ → part 5: Simplify click timeouts
Attachment #477706 - Flags: review+
Attachment #477708 - Attachment description: , part 6: MouseModule generates tap events instead of using clunky clickers [ → part 6: MouseModule generates tap events instead of using clunky clickers
Attachment #477708 - Flags: review+
Attachment #477709 - Attachment description: , part 7: Separate panning from tapping and MouseModule no longer cares about whether something is content [ → part 7: Separate panning from tapping and MouseModule no longer cares about whether something is content
Attachment #477709 - Flags: review+
Attachment #477712 - Attachment description: , part 8: Create new longtap event and use it to bring up context menus [ → part 8: Create new longtap event and use it to bring up context menus
Attachment #477712 - Flags: review+
Attachment #477713 - Attachment description: , part 9: Fix interaction between mousemodule and gesturemodule → part 9: Fix interaction between mousemodule and gesturemodule
Attachment #477713 - Flags: review?(mbrubeck)
Attachment #477713 - Flags: review?(mbrubeck) → review+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.