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

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

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).

Comment 1

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

I can repro this.

Comment 2

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

Oh, this might be fixed by bug 1231570, ni myself to check when I get a chance.

Comment 3

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

This is still happening.

Comment 4

[jradmacher]

I have the same problem, but it seems the bug is a bit different in a "non e10s" window: There it looks like two right clicks. One when pressing and one when releasing.

Comment 5

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

So there's a bunch of problems here. One is that we're actually dispatching two different contextmenu events. One from the APZEventState code at [1] and the other from the windows widget code at [2]. Both of these go through the listener at [3] which shows the popup. That's why if you look closely you see the context menu blink - it's actually being shown twice. The listener doesn't do a preventDefault on the event, so the APZ version of the event then further triggers a click since it considers the event to be unhandled. The widget version just ignores it.

The widget version of the event I think is what we want to keep, because it happens on touch-up which is more consistent with the Windows native UI contextmenu. So we'll need to add a way to disable the APZ long-press detection, and ensure that the touch event sequence we fire to web content is consistent with Chrome (and check what happens on other platforms like Android, where the long-press is detected while the finger is still down).

[1] http://searchfox.org/mozilla-central/rev/970569ad57ac4436ff31aa2ac63f38ed3ee2932d/gfx/layers/apz/util/APZEventState.cpp#231
[2] http://searchfox.org/mozilla-central/rev/970569ad57ac4436ff31aa2ac63f38ed3ee2932d/widget/windows/nsWindow.cpp#5174
[3] http://searchfox.org/mozilla-central/rev/970569ad57ac4436ff31aa2ac63f38ed3ee2932d/browser/base/content/content.js#216

Comment 6

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

I tested on [1] (it logs events to web console).

Android Firefox does: touchdown, contextmenu, touchcancel (or touchend)
Android Chrome does: touchdown, mousemove, mousedown, contextmenu, touchcancel (or touchend)

So apart from the mouse events Chrome sends (which IMO doesn't seem right) we seem to match up.

On Windows Chrome does: touchstart, mousemove, mousedown, contextmenu, touchend (with the browser context popup showing on touchend, if web content didn't do preventDefault on the contextmenu).

This seems reasonable, minus (again) the mouse events. Dave, do you know why Chrome dispatches mousemove/mousedown for touch-based long-press user actions? I assume for web compat with IE, but it's odd that there's no mouseup then. Do you know if there's actual sites that depend on this behaviour?

[1] http://people.mozilla.org/~kgupta/touch.html

Comment 7

[dtapuska]

The relevant chromium code is here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/input/EventHandler.cpp?sq=package:chromium&rcl=1466752711&l=2363

We do have a TODO in this code with crbug.com/579564

The mouse move was added to fix this issue it seems: crbug.com/485290

Comment 8

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

Hm, thanks! https://bugs.chromium.org/p/chromium/issues/detail?id=485290#c1 has the explanation I was looking for - I think based on that we should be ok with not sending any mouse events at all, at least until we run into sites that break. Hopefully there won't be many since on mobile Safari and Firefox both don't send mouse events.

Comment 9

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

Also it's important that we do NOT fire the contextmenu event if the touchstart event is cancelled by content. That's what we do on other platforms, what Chrome does, and what is specced behaviour. So we can't just skip the APZ long-press detection and call it a day, there needs to be some extra goop to ensure that the widget contextmenu event isn't dispatched either if the touchstart is cancelled.

Comment 10

[Rick Byers]

We should probably say something in the TE spec about this.  I filed https://github.com/w3c/touch-events/issues/68, kats@ mind weighing in there?

Comment 11

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

I did some more testing and experimenting. It's possible for us to (1) disable our long-press detection, and (2) skip the contextmenu event if the touch block was cancelled. However, the APZ code still fires a click event on a long-press, and getting rid of that is nontrivial. Ideally we would abort the click when Windows detected a long-press event. However we don't get any notification when that happens, and there's no API that I can find that tells us the duration of the long-press delay that the user has configured. The contextmenu event itself arrives too late (after the touch-up) to be used for this purpose.

I was looking at what Chrome did and they actually seem to just implement their own timer, because if I set the windows long-press delay to very long, I can repro the case where Chrome generates a contextmenu event before Windows shows me the visual feedback "square" that indicates it detected the long-press gesture.

So I guess we should probably do the same and use our internal timer, but only trigger the actual contextmenu popup on touch-end, to better simulate what Windows would have done.

Comment 12

[Rick Byers]

Yeah we still have some bugs here too - eg. https://bugs.chromium.org/p/chromium/issues/detail?id=240550

Comment 13

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

Attachment: Bug 1256339 - Collapse the different Handle*Tap functions in GeckoContentController into a single API.

This is just a refactoring, no functional changes intended.

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

Comment 14

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

Attachment: Bug 1256339 - Add a eLongTapUp tap type, which fires an observer notification rather than doing a click.

For B2G we had this "special" behaviour where a long-press that didn't trigger
a contextmenu or whose contextmenu event was cancelled would still trigger a
click event. No other browser does this, and so I think it doesn't make sense
for us to keep doing it either. It also makes it much harder to implement the
Windows-style contextmenu, where the contextmenu pops up when you *lift* your
finger after doing a long-press.

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

Comment 15

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

Attachment: Bug 1256339 - Fix up handling for touch-derived contextmenu events on desktop.

This patch stops the widget code from passing along touch-derived contextmenu
events straight from Windows to Gecko, and instead lets the APZ gesture
detection code handle it. This allows the contextmenu event to be prevented
according to web standards, e.g. if the touchstart event is cancelled.

This changes to browser.js will affect both Linux and Windows, but the behaviour
implemented is in line with native Windows touch behaviour. We may want to
add an alternate codepath for Linux to better simulate "native" Linux behavior,
if there is such a thing for touch-derived contextmenu.

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

Comment 16

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

I tested the above patches on Windows and Android in all the configurations I could think of. I didn't test Linux, but I expect it to behave the same as on Windows (contextmenu event will be fired after 500ms of long-pressing, and if it's not cancelled and the user doesn't move their finger too much, the browser will show a context menu when the finger is lifted).

mconley, not sure if you're the right person to review the content.js changes, feel free to redirect if you know somebody else would be more appropriate. I don't know if that code is really owned by anybody, I mostly just need a JS review.

Comment 17

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

Forgot to update a gtest in part 2. Updated patches coming with that small change.

Comment 18

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

Comment on attachment 8766872 [details]
Bug 1256339 - Collapse the different Handle*Tap functions in GeckoContentController into a single API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61590/diff/1-2/

Comment 19

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

Comment on attachment 8766873 [details]
Bug 1256339 - Add a eLongTapUp tap type, which fires an observer notification rather than doing a click.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61592/diff/1-2/

Comment 20

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

Comment on attachment 8766874 [details]
Bug 1256339 - Fix up handling for touch-derived contextmenu events on desktop.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61594/diff/1-2/

Comment 21

[Botond Ballo [:botond]]

Comment on attachment 8766872 [details]
Bug 1256339 - Collapse the different Handle*Tap functions in GeckoContentController into a single API.

https://reviewboard.mozilla.org/r/61590/#review58876

::: dom/ipc/TabChild.h:627
(Diff revision 2)
>    void ContentReceivedInputBlock(const ScrollableLayerGuid& aGuid,
>                                   uint64_t aInputBlockId,
>                                   bool aPreventDefault) const;
>    void SetTargetAPZC(uint64_t aInputBlockId,
>                      const nsTArray<ScrollableLayerGuid>& aTargets) const;
> -  void HandleDoubleTap(const CSSPoint& aPoint,
> +  void HandleTap(const layers::GeckoContentController::TapType& aType,

We can take the TapType by value too.

::: dom/ipc/TabChild.h:632
(Diff revision 2)
> -                       bool aCallTakeFocusForClickFromTap);
> -  void HandleLongTap(const CSSPoint& aPoint,
> -                     const Modifiers& aModifiers,
> +                 const Modifiers& aModifiers,
> -                     const mozilla::layers::ScrollableLayerGuid& aGuid,
> +                 const mozilla::layers::ScrollableLayerGuid& aGuid,
> -                     const uint64_t& aInputBlockId);
> +                 const uint64_t& aInputBlockId,
> +                 const bool& aCallTakeFocusForClickFromTap);

As this is not a generated function signature, there's no need to take the bool by reference-to-const.

::: gfx/layers/apz/public/GeckoContentController.h:43
(Diff revision 2)
>    virtual void RequestContentRepaint(const FrameMetrics& aFrameMetrics) = 0;
>  
>    /**
> -   * Requests handling of a double tap. |aPoint| is in CSS pixels, relative to
> -   * the current scroll offset. This should eventually round-trip back to
> -   * AsyncPanZoomController::ZoomToRect with the dimensions that we want to zoom
> +   * Different types of tap-related events that can be sent in
> +   * the HandleTap function. The names should be relatively self-explanatory.
> +   * Note that the eLongTapUp will always be preceded by an eLongTap, but not

This comment talks about eLongTapUp, but there is no such tap type.

(Ah, I see now. It's added in the second patch.)

::: gfx/layers/apz/util/ChromeProcessController.cpp
(Diff revision 2)
>  void
>  ChromeProcessController::HandleDoubleTap(const mozilla::CSSPoint& aPoint,
>                                           Modifiers aModifiers,
>                                           const ScrollableLayerGuid& aGuid)
>  {
> -  if (MessageLoop::current() != mUILoop) {

Assert that we're on mUILoop?

::: gfx/layers/ipc/PAPZ.ipdl:94
(Diff revision 2)
>  child:
>    async UpdateFrame(FrameMetrics frame);
>  
>    // The following methods correspond to functions on the GeckoContentController
>    // interface in gfx/layers/apz/public/GeckoContentController.h. Refer to documentation
>    // in that file for these functions.

Not strictly related to this change, but can we take this opportunity to document what |aCallTakeFocusForClickFromTap| does? That's not in the GeckoCC API, so the comment above ("just look at the GeckoCC API") doesn't apply to it.

Comment 22

[Botond Ballo [:botond]]

Comment on attachment 8766873 [details]
Bug 1256339 - Add a eLongTapUp tap type, which fires an observer notification rather than doing a click.

https://reviewboard.mozilla.org/r/61592/#review58882

::: gfx/layers/apz/src/AsyncPanZoomController.h:626
(Diff revision 2)
>      STICKY,   /* Allow lock to be broken, with hysteresis */
>    };
>  
>    static AxisLockMode GetAxisLockMode();
>  
>    // Helper function for OnSingleTapUp() and OnSingleTapConfirmed().

and OnLongPressUp()

Comment 23

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

I updated my local patches with all the review comments above (and I also moved the eLongTapUp documentation to the second patch - thanks for catching that!).

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8766874 [details]
Bug 1256339 - Fix up handling for touch-derived contextmenu events on desktop.

https://reviewboard.mozilla.org/r/61594/#review58906

This observer should be removed when the window is unloaded, like this: http://searchfox.org/mozilla-central/source/browser/base/content/tab-content.js#581

::: browser/base/content/content.js:248
(Diff revision 2)
>  
>  Cc["@mozilla.org/eventlistenerservice;1"]
>    .getService(Ci.nsIEventListenerService)
>    .addSystemEventListener(global, "contextmenu", handleContentContextMenu, false);
>  
> +Services.obs.addObserver(showContentContextMenu, "APZ:LongTapUp", false);

This observer never gets removed, so I think we're going to leak, no?

Comment 25

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

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=65374abe1277&group_state=expanded is pretty clean, it includes the observer removal mconley pointed out in comment 24. My previous try pushes had a bunch of Linux debug failures which may or may not have been related to this, but this one is clean so I'm happy.

Comment 26

[Jim Mathies [:jimm]]

Comment on attachment 8766874 [details]
Bug 1256339 - Fix up handling for touch-derived contextmenu events on desktop.

https://reviewboard.mozilla.org/r/61594/#review59162

::: widget/windows/nsWindow.cpp:5148
(Diff revision 2)
>      break;
>  
>      case WM_CONTEXTMENU:
>      {
> +      // If the context menu is brought up by a touch long-press, then
> +      // the APZ code has responsble for dealing with this, so we don't

nit - s/has/is

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8f47976eb5a
Collapse the different Handle*Tap functions in GeckoContentController into a single API. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/fefd3e69b369
Add a eLongTapUp tap type, which fires an observer notification rather than doing a click. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/6172a55a6ae8
Fix up handling for touch-derived contextmenu events on desktop. r=mconley,jimm

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/b8f47976eb5a
https://hg.mozilla.org/mozilla-central/rev/fefd3e69b369
https://hg.mozilla.org/mozilla-central/rev/6172a55a6ae8

Comment 29

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

I backed out part 3 of this patch series in bug 1298313 to fix that issue. I figured it was the best way forward since I was going to do something similar to that anyway to fix bug 1292572.

That being said, the backout makes *this* bug happen again, and I'm going to clone this bug to track that issue. Because the first two patches here are going to stay in the tree I feel it will be less confusing to open a new bug than to reopen this one.

Comment 30

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

Bug 1298908 is the clone.

Comment 31

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

Marking 51 as disabled since the part 3 patch is backed out there.