Closed Bug 1298908 Opened 3 years ago Closed 3 years ago

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

Categories

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

Unspecified
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- fixed

People

(Reporter: kats, Assigned: kats)

References

()

Details

(Keywords: regression, Whiteboard: [gfx-noted][nightly only])

Attachments

(2 files)

+++ 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.
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 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 on attachment 8786503 [details]
Bug 1298908 - Extract a helper method that does the dispatching of contextmenu events.

https://reviewboard.mozilla.org/r/75426/#review73652
Attachment #8786503 - Flags: review?(botond) → review+
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 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
Attachment #8786504 - Flags: review?(botond) → review+
(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 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
Attachment #8786504 - Flags: review?(jmathies) → review+
Pushed by kgupta@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/01b5da819a7c
https://hg.mozilla.org/mozilla-central/rev/fd96f3fee975
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Version: 48 Branch → Trunk
You need to log in before you can comment on or make changes to this bug.