If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Context popups are quirky

VERIFIED FIXED

Status

Fennec Graveyard
General
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: stechz, Assigned: stechz)

Tracking

Details

(Whiteboard: [fennec-checkin-postb1])

Attachments

(9 attachments, 10 obsolete attachments)

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
(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
Created attachment 476128 [details] [diff] [review]
part 1: fix Util.Timeout bugs
(Assignee)

Comment 2

7 years ago
Created attachment 476129 [details] [diff] [review]
part 2: DragData no longer needs owner
(Assignee)

Comment 3

7 years ago
Created attachment 476130 [details] [diff] [review]
part 3: Drag move and drag end no longer need to be passed parameters
(Assignee)

Comment 4

7 years ago
Created attachment 476131 [details] [diff] [review]
Cancel pending events properly lets go of mouse movements for MouseModule
(Assignee)

Comment 5

7 years ago
Created attachment 476132 [details] [diff] [review]
part 5: Simplify click timeouts
(Assignee)

Comment 6

7 years ago
Created attachment 476133 [details] [diff] [review]
part 6: MouseModule generates tap events instead of using clunky clickers
(Assignee)

Comment 7

7 years ago
Created attachment 476134 [details] [diff] [review]
part 7: Separate panning from tapping and MouseModule no longer cares about whether something is content
(Assignee)

Comment 8

7 years ago
Created attachment 476136 [details] [diff] [review]
part 8: Create new LongTap event and use it to bring up content context menus
(Assignee)

Updated

7 years ago
Attachment #476128 - Flags: review?(mark.finkle)
(Assignee)

Updated

7 years ago
Attachment #476129 - Flags: review?(mark.finkle)
(Assignee)

Updated

7 years ago
Attachment #476130 - Flags: review?(mark.finkle)
(Assignee)

Updated

7 years ago
Attachment #476131 - Flags: review?(mbrubeck)
(Assignee)

Updated

7 years ago
Attachment #476132 - Flags: review?(mbrubeck)
(Assignee)

Updated

7 years ago
Attachment #476133 - Flags: review?(mbrubeck)
(Assignee)

Updated

7 years ago
Attachment #476134 - Flags: review?(mbrubeck)
(Assignee)

Updated

7 years ago
Attachment #476136 - Flags: review?(21)
(Assignee)

Comment 9

7 years ago
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+
Stechz if we want some kind of touch event why are we not using https://developer.mozilla.org/en/DOM/Touch_events?
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+
(Assignee)

Comment 13

7 years ago
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.
(Assignee)

Updated

7 years ago
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

Comment 16

7 years ago
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.

Updated

7 years ago
Duplicate of this bug: 598342
(Assignee)

Comment 19

7 years ago
Created attachment 477319 [details] [diff] [review]
part 4: Context popups are quirky - Use cancel pending where appropriate
(Assignee)

Comment 20

7 years ago
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)
(Assignee)

Updated

7 years ago
Attachment #476131 - Attachment is obsolete: true
(Assignee)

Comment 21

7 years ago
> 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.
(Assignee)

Updated

7 years ago
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
(Assignee)

Updated

7 years ago
Attachment #476128 - Attachment description: fix Util.Timeout bugs → part 1: fix Util.Timeout bugs
(Assignee)

Updated

7 years ago
Attachment #476129 - Attachment description: DragData no longer needs owner → part 2: DragData no longer needs owner
(Assignee)

Updated

7 years ago
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
(Assignee)

Updated

7 years ago
Attachment #476132 - Attachment description: Simplify click timeouts → part 5: Simplify click timeouts
(Assignee)

Updated

7 years ago
Attachment #477319 - Attachment description: Context popups are quirky - Use cancel pending where appropriate → part 4: Context popups are quirky - Use cancel pending where appropriate
(Assignee)

Updated

7 years ago
Attachment #476133 - Attachment description: MouseModule generates tap events instead of using clunky clickers → part 6: MouseModule generates tap events instead of using clunky clickers
(Assignee)

Updated

7 years ago
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
(Assignee)

Updated

7 years ago
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+
(Assignee)

Comment 23

7 years ago
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.
(Assignee)

Comment 27

7 years ago
Created attachment 477629 [details] [diff] [review]
, part 9: Fix interaction between mousemodule and gesturemodule
(Assignee)

Comment 28

7 years ago
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)
(Assignee)

Comment 29

7 years ago
Created attachment 477702 [details] [diff] [review]
part 1: fix Util.Timeout bugs
(Assignee)

Comment 30

7 years ago
Created attachment 477703 [details] [diff] [review]
part 2: DragData no longer needs owner
(Assignee)

Comment 31

7 years ago
Created attachment 477704 [details] [diff] [review]
part 3: Drag move and drag end no longer need to be passed parameters
(Assignee)

Updated

7 years ago
Attachment #477319 - Attachment is obsolete: true
(Assignee)

Comment 32

7 years ago
Created attachment 477705 [details] [diff] [review]
part 4: Use cancel pending where appropriate
(Assignee)

Comment 33

7 years ago
Created attachment 477706 [details] [diff] [review]
part 5: Simplify click timeouts
(Assignee)

Comment 34

7 years ago
Created attachment 477708 [details] [diff] [review]
part 6: MouseModule generates tap events instead of using clunky clickers
(Assignee)

Comment 35

7 years ago
Created attachment 477709 [details] [diff] [review]
part 7: Separate panning from tapping and MouseModule no longer cares about whether something is content
(Assignee)

Comment 36

7 years ago
Created attachment 477712 [details] [diff] [review]
part 8: Create new longtap event and use it to bring up context menus
(Assignee)

Updated

7 years ago
Attachment #477629 - Attachment is obsolete: true
Attachment #477629 - Flags: review?(mbrubeck)
(Assignee)

Comment 37

7 years ago
Created attachment 477713 [details] [diff] [review]
part 9: Fix interaction between mousemodule and gesturemodule
(Assignee)

Updated

7 years ago
Attachment #476128 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #476129 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #476130 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #476132 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #476133 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #476134 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #476136 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #477702 - Attachment description: , part 1: fix Util.Timeout bugs [ → part 1: fix Util.Timeout bugs
Attachment #477702 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #477702 - Attachment is obsolete: false
Attachment #477702 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #477703 - Attachment description: , part 2: DragData no longer needs owner [ → part 2: DragData no longer needs owner
Attachment #477703 - Flags: review+
(Assignee)

Updated

7 years ago
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+
(Assignee)

Updated

7 years ago
Attachment #477705 - Attachment description: , part 4: Use cancel pending where appropriate [ → part 4: Use cancel pending where appropriate
Attachment #477705 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #477706 - Attachment description: , part 5: Simplify click timeouts [ → part 5: Simplify click timeouts
Attachment #477706 - Flags: review+
(Assignee)

Updated

7 years ago
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+
(Assignee)

Updated

7 years ago
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+
(Assignee)

Updated

7 years ago
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+
(Assignee)

Updated

7 years ago
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+
(Assignee)

Comment 38

7 years ago
Pushed http://hg.mozilla.org/mobile-browser/rev/5ab8842823a7
(Assignee)

Updated

7 years ago
Status: NEW → RESOLVED
Last Resolved: 7 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.