Jim Mathies [:jimm] Reporter
	Description • 8 years ago

STR:

1) Surface 4, no mouse, no keyboard, open nightly
2) press-hold in the gray chrome area to try and bring up the config popup menu

result: about 75% of the time, the menu will display briefly and then hide, making it impossible to toggle UI elements like the bookmarks bar or menu bar.

When the menu does display, it shows on just as the press-hold rectangle animation starts, which technically is too early. The press-hold animation should complete before the menu is displayed.

	Jim Mathies [:jimm] Reporter
	Comment 1 • 8 years ago

This behavior is also visible on bookmark bar shortcuts. 

Kats, any chance this is related to recent touch events work? Is there a pref I can flip to test if recent changes regressed this?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 2 • 8 years ago

If it's a regression in today's nightly it might be from bug 1195722, which you can check by setting layout.accessiblecaret.enabled_on_touch to false. If it's older it might be from bug 1256339. No pref for that one.

	:Gijs (he/him)
	Comment 3 • 8 years ago

This seems like it'd be either a widget or XUL or (touch) events issue, so moving to Core.

	Michelle Funches - QA
	Comment 4 • 8 years ago

QA Update:
I do not have "Surface 4" I did however attempt to reproduce with Acer Aspire Switch 10E touch enabled laptop/tablet.
tested with layout.accessiblecaret.enabled_on_touch to false and also with default true

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 5 • 8 years ago

I think this is happening because with touch events, the contextmenu event is fired fairly early, before the animation is complete. We had to add special handling for the content area at [1] and may need to do something similar for the XUL widgets that show the chrome context menus.

[1] http://searchfox.org/mozilla-central/rev/b35975ec17c8227e827800fa2d9642655cb103a8/browser/base/content/content.js#107

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 6 • 8 years ago

This is mostly a regression from bug 1256339. Before that (but after touch events were enabled back on 2016-01-31) long-pressing in the toolbar area would cause the context menu to appear, disappear, and reappear. That happened for the same reason as explained in bug 1256339 comment 5.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 7 • 8 years ago

Also, touch events are currently enabled only on nightly, so this is limited to that branch.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 8 • 8 years ago

I have patches that more or less fix this. I had to redo the fix for bug 1256339 a different way and then pile on top of that a bit. Top two patches at [1] have the fixes, but I need to test them more.

[1] https://github.com/staktrace/gecko-dev/commits/longtap

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 9 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6a166455436

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 10 • 8 years ago

Taskcluster meltdown, but the jobs that finished seem green enough. I'll do another try push before landing but in the meantime I'll put the patches up for review.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 11 • 8 years ago

Since the LongTapUp observer notification doesn't have useful things like an event target, it's not as good for driving these context menu things. I'm replacing it with a new "mouselongtapup" event (similar to the existing "mouselongtap" event) and using that instead. r?smaug that it's ok to make such an event and dispatch it to chrome only. r?jimm for the rest, which is mostly a backout/reimplementation of bug 1256339 part 3.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 12 • 8 years ago

This fixes the main part of this bug, by using the mouselongtapup event instead of the contextmenu event to trigger XUL popups (but only if the contextmenu event is touch-triggered and APZ is enabled). I also needed to touch some stuff in the code that dismisses popups to get sane behaviour. r?smaug for the XUL bits and r?jimm for the widget bits, but please feel free to redirect.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 13 • 8 years ago

Comment on attachment 8784482 [details] [diff] [review]
Part 1 - Add a mouselongtapup event and use it instead of APZ:LongTapUp

ok to add the event.
Since mIgnoreRootScrollFrame is unusual thing in general, setting it to non-default value could use a comment.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 14 • 8 years ago

Comment on attachment 8784483 [details] [diff] [review]
Part 2 - Use the mouselongtapup event to trigger XUL context menu popups

oh, now I don't understand this after all. Why do we want some other event than context to trigger context menu? Wouldn't it make more sense to have the default action for mouselongtapup to trigger context menu event or something.

How does the new event work with web pages? contextmenu is after all event exposed to web pages and they can call preventDefault() if needed.

Why should we have two ways to trigger context menu? Chrome code would need to add listeners for both to prevent the menu.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 15 • 8 years ago

Sorry for not giving more context before review.

(In reply to Olli Pettay [:smaug] (vacation Aug 25-28) from comment #14)
> Why do we want some other event
> than context to trigger context menu?

On Windows, the platform convention is that the context menu is supposed to appear when you lift your finger after a long-press. However, the web convention (and what is implemented by Chrome) is that the "contextmenu" event is fired to web content after the holds their finger down for ~500 milliseconds (while the finger is still down). i.e. the timeline looks like this:

t=0 finger goes down
t=500 contextmenu event fired to web content
t=later finger is lifted
t=later browser context menu is displayed

Web content can call preventDefault() on the contextmenu event, in which case the browser context menu is not supposed to be displayed. So the way I'm implementing it is I added this mouselongtapup event which is fired at t=later, but ONLY if the contextmenu event was not preventDefault()ed. That can then be used as a reliable way to know if the browser context menu should be displayed or not.

> Wouldn't it make more sense to have
> the default action for mouselongtapup to trigger context menu event or
> something.

If we did this, then the contextmenu event would get fired to web content at t=later. That might be a valid option, as it would make the web behaviour more compatible with Windows platform (but on Android we would still want contextmenu at t=500, so we would have different codepaths across platforms). It would also be different from what Chrome does, and it might potentially break sites, I'm not sure.

> How does the new event work with web pages? contextmenu is after all event
> exposed to web pages and they can call preventDefault() if needed.

Yeah, that should still work. If the contextmenu is preventDefault()ed by web content, the APZ cancels the rest of the gesture and so doesn't send the long-tap-up notification. So the mouselongtapup event won't happen at all in this case.

> Why should we have two ways to trigger context menu? Chrome code would need
> to add listeners for both to prevent the menu.

Yeah this is the downside of this approach. The only other viable approach I think is the one above where we fire the contextmenu at t=later. I could try doing that if you prefer. There is also some more background and discussion in bug 1256339 comment 5 - 11.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 16 • 8 years ago

Comment on attachment 8784483 [details] [diff] [review]
Part 2 - Use the mouselongtapup event to trigger XUL context menu popups

Dropping review flags for now. In bug 1298313 I plan on backing out some of bug 1256339, so I'll revisit this after that.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 17 • 8 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15)
> If we did this, then the contextmenu event would get fired to web content at
> t=later. That might be a valid option, as it would make the web behaviour
> more compatible with Windows platform (but on Android we would still want
> contextmenu at t=500, so we would have different codepaths across
> platforms). It would also be different from what Chrome does, and it might
> potentially break sites, I'm not sure.

I wrote patches that do this, and they seem to work reasonably well. It's different from Chrome's behaviour, but it still respects the various invariants I would expect (e.g. that preventDefault on the touchstart will prevent the contextmenu event from ever firing). Once I'm satisfied with the patches I'm gonna post the patches to bug 1298908 and dupe this bug over.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 18 • 8 years ago

The backout in bug 1298313 should have fixed this, and the patches in bug 1298908 should have improved things. Resolving.