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

RESOLVED FIXED in mozilla50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

({regression})

48 Branch
mozilla50
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 unaffected, firefox48 unaffected, firefox49 unaffected, firefox50 disabled, firefox51 disabled)

Details

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

Attachments

(3 attachments)

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).
I can repro this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Whiteboard: [gfx-noted]
Oh, this might be fixed by bug 1231570, ni myself to check when I get a chance.
Flags: needinfo?(bugmail.mozilla)
This is still happening.
Flags: needinfo?(bugmail.mozilla)
Whiteboard: [gfx-noted] → [gfx-noted][nightly only]

Comment 4

3 years ago
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.
Version: Trunk → 48 Branch
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
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
Flags: needinfo?(dtapuska)

Comment 7

3 years ago
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
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.
Flags: needinfo?(dtapuska)
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.
Assignee: nobody → bugmail.mozilla

Comment 10

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

3 years ago
Yeah we still have some bugs here too - eg. https://bugs.chromium.org/p/chromium/issues/detail?id=240550
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/
Attachment #8766872 - Flags: review?(botond)
Attachment #8766873 - Flags: review?(botond)
Attachment #8766874 - Flags: review?(mconley)
Attachment #8766874 - Flags: review?(jmathies)
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/
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/
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.
Forgot to update a gtest in part 2. Updated patches coming with that small change.
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 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 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 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.
Attachment #8766872 - Flags: review?(botond) → review+
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()
Attachment #8766873 - Flags: review?(botond) → review+
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!).
Attachment #8766874 - Flags: review?(mconley) → review+
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?
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 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
Attachment #8766874 - Flags: review?(jmathies) → review+

Comment 27

3 years ago
Pushed by kgupta@mozilla.com:
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
Depends on: 1292572
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.
Bug 1298908 is the clone.
See Also: → 1298908
Marking 51 as disabled since the part 3 patch is backed out there.
No longer depends on: 1292572
You need to log in before you can comment on or make changes to this bug.