1298908 - Long-press on Windows touch activates link and also puts up context menu

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[Kartikaya Gupta (email:[e-mail])]

+++ This bug was initially created as a clone of Bug #1256339 +++

From https://bugzilla.mozilla.org/show_bug.cgi?id=1231570#c3:

I get this problem on all links on all sites on Windows nightly touch. (Just in case, verified just now on http://www.abc.net.au/ ) I long press one of the links and get both a context menu *and* a click (i.e. it navigates to the page, with the context menu still hovering over the top).

=====
This is a clone of bug 1256339, because I backed out part three of that bug to fix a separate issue, but I didn't want to reopen that bug.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

I wrote some patches that move the contextmenu event from being fired on long-press detection to long-press-up, but only for Windows (since that's the platform convention on Windows). That seems to fix this bug, as well as bug 1292572 and bug 1298982. They're also a lot simpler and less error-prone than the previous way I was trying to fix this bug.

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b719249842f

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1298908 - Extract a helper method that does the dispatching of contextmenu events.

Review commit: https://reviewboard.mozilla.org/r/75426/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75426/

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1298908 - On Windows, fire the contextmenu and long-tap events on the long-tap-up user action.

This patch prevents the Windows widget code from dispatching the contextmenu
event if APZ is handling touch input. Instead, the APZ code processes the
raw touch input, and will fire a contextmenu event when the user lifts their
finger after a long-press action, in keeping with the Windows platform
convention. Doing it this way also allows us to respect web conventions where
the web content can prevent the contextmenu event from firing by calling
preventDefault on the touchstart event; this was not possible when dispatching
the contextmenu event directly from the widget code.

This also makes long-pressing on browser chrome components work properly, as
it just shifts the point in time that the contextmenu event is fired without
changing any of the code that triggers the XUL popup. However, some changes
were needed to have the widget code ignore the synthetic mouse events that
the Windows platform sends us, because those would otherwise immediately
dismiss the contextmenu popup after it appeared.

Review commit: https://reviewboard.mozilla.org/r/75428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75428/

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8786503 [details]
Bug 1298908 - Extract a helper method that does the dispatching of contextmenu events.

https://reviewboard.mozilla.org/r/75426/#review73650

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8786503 [details]
Bug 1298908 - Extract a helper method that does the dispatching of contextmenu events.

https://reviewboard.mozilla.org/r/75426/#review73652

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 8786504 [details]
Bug 1298908 - On Windows, fire the contextmenu and long-tap events on the long-tap-up user action.

https://reviewboard.mozilla.org/r/75428/#review73664

::: gfx/layers/apz/util/APZEventState.cpp:293
(Diff revision 1)
> +                                Modifiers aModifiers)
>  {
> -  nsCOMPtr<nsIObserverService> observerService = mozilla::services::GetObserverService();
> -  observerService->NotifyObservers(nullptr, "APZ:LongTapUp", nullptr);
> +#ifdef XP_WIN
> +  nsCOMPtr<nsIWidget> widget = GetWidget();
> +  if (widget) {
> +    FireContextmenuEvents(aPresShell, aPoint, aScale, aModifiers, widget);

Do we need to send a touchcancel if the contextmenu event is handled, as in the long-tap case?

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8786504 [details]
Bug 1298908 - On Windows, fire the contextmenu and long-tap events on the long-tap-up user action.

https://reviewboard.mozilla.org/r/75428/#review73688

Comment 8

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #6)
> Do we need to send a touchcancel if the contextmenu event is handled, as in
> the long-tap case?

No, because at this point the finger has already been lifted, so there's no touch events ongoing to cancel. Sending a touchcancel after the touchend would provide no value, and might confuse content JS.

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 8786504 [details]
Bug 1298908 - On Windows, fire the contextmenu and long-tap events on the long-tap-up user action.

https://reviewboard.mozilla.org/r/75428/#review74192

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01b5da819a7c
Extract a helper method that does the dispatching of contextmenu events. r=botond
https://hg.mozilla.org/integration/autoland/rev/fd96f3fee975
On Windows, fire the contextmenu and long-tap events on the long-tap-up user action. r=botond,jimm

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/01b5da819a7c
https://hg.mozilla.org/mozilla-central/rev/fd96f3fee975