Investigate whether long tap injector is needed on Windows with touch support

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: TYLin, Assigned: kats)

Tracking

(Blocks: 1 bug)

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
AccessibleCaret has a long tap injector pref [1] to simulate long tap events by timer on desktop platforms without native eMouseLongTap.  I was wondering whether Windows with touch support needs that.  Fennec has this pref disabled.

If |dom::TouchEvent::PrefEnabled()| returns true implies having native eMouseLongTap support by APZ, we should add a condition in [2] to use the long tap injector only if there's no touch event support.

[1] http://searchfox.org/mozilla-central/rev/dbbbafc979a8362a1c8e3e99dbea188fc832b1c5/modules/libpref/init/all.js#5235
[2] http://searchfox.org/mozilla-central/rev/dbbbafc979a8362a1c8e3e99dbea188fc832b1c5/layout/base/AccessibleCaretEventHub.cpp#436
Any platform that supports touch events with APZ should be getting eMouseLongTap events (assuming the contextmenu event wasn't prevent-defaulted). So yeah I don't think this injector is needed on Windows.

It's not clear to me why you would want to use the long tap injector if there's no touch event support though - what would trigger the long tap timer without touch events?
Flags: needinfo?(tlin)
Actually it looks like we'll need to keep this on for now. I tried turning it off and it results in text selection not really working on Windows. The reason is that pretty much every long-press will throw up a context menu and consume the contextmenu event, so the eMouseLongTap doesn't get fired.

I'm going to close this as WONTFIX but feel free to reopen if you feel there's still something that we should do here.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(tlin)
Resolution: --- → WONTFIX
(Reporter)

Comment 3

2 years ago
The log tap injector was designed for marionette test to run on all desktop platforms, and it's also useful for developers to test caret on desktop, so I turn it on by default in all.js.

> The reason is that pretty much every long-press will throw up a context menu and
> consume the contextmenu event, so the eMouseLongTap doesn't get fired.

So unlike fennec, we always show the context menu and caret together.  I wonder how other browsers behave.  Perhaps there's something we can improve.
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #3)
> So unlike fennec, we always show the context menu and caret together.  I
> wonder how other browsers behave.  Perhaps there's something we can improve.

This interaction at least was intentional, per bug 1195722 comment 7. But yeah there's probably something we can improve here.

In terms of implementation detail I was going to maybe look at moving the eMouseLongTap so that it fires earlier and doesn't depend on the contextmenu event the way it does now. I'll need to look at all who all uses eMouseLongTap though and if any consumers of that event can be simplified as a result. Will do that later in a follow-up since it doesn't really affect user-visible behaviour.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> In terms of implementation detail I was going to maybe look at moving the
> eMouseLongTap so that it fires earlier and doesn't depend on the contextmenu
> event the way it does now.

I discovered a bug that makes me want to do this even more. The bug is that if you have a page with an iframe and the focus is on the outer document, and you long-press inside the iframe, the selection ends up on the outer document. e.g. if you load http://people.mozilla.org/~kgupta/gridframe.html and tap outside the iframe, and then long-press inside the iframe, the selection shows up outside the iframe. However if you tap inside the iframe (on some blank space) and then long-press, the selection shows up correctly inside. I think we need to shift focus to the iframe once the long-press is detected.
I filed bug 1306634 for the issue I mentioned in the previous comment.
See Also: → bug 1306634
Created attachment 8796666 [details] [diff] [review]
WIP

This is what I was considering. It would need more testing and stuff if we want to land it.
The more I think about it, the more I want to land this. Having the eMouseLongTap event fired earlier will also make it easier to implement things like bug 1147335.
Assignee: nobody → bugmail
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Reporter)

Comment 9

2 years ago
I would be grateful to see the patch land.  The long tap injector should be used only as a hack on platforms without APZ eMouseLongTap support for testing purpose, and it would hide problems when eMouseLongTap events go wrong.  

And I just recall that I had done bug 1209841 so that marionette tests are not using the injector anymore, so I second the idea to disable the injector on all.js.
The try push I did [1] seems to show a pretty consistent failure in some robocop tests that seem related to long-press, so I need to investigate those first. I can't seem to reproduce locally which is gonna make it tricky.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=925cd58c9981&group_state=expanded
I tried the patch on a device and it actually changes Fennec behaviour to be like that on Windows - long-pressing will do both text selection and context menu. I should have realized that will happen. I still think it makes sense to fire the eMouseLongTap event first but maybe we need to use the contextmenu event for AccessibleCaretEventHub on Android, if it's not prevent-defaulted. Or maybe we should use the context-menu-not-shown notification [1] instead. I'll experiment and see what works best.

[1] http://searchfox.org/mozilla-central/rev/3411332d29d377bf86405fc3ea72461ea0cb0295/mobile/android/chrome/content/browser.js#2687
I gave this some more thought and I concluded that it's probably best to only fire the eMouseLongTap early on Windows. For one thing, this would encapsulates all the windows-ifdef behaviour into APZEventState so it's less likely to get out of sync. Also it allows AccessibleCaretEventHub to just listen for the eMouseLongTap event instead of also listening for other random notifications that are per-platform. I'll update the patch accordingly and retest.
Comment hidden (mozreview-request)
Attachment #8796666 - Attachment is obsolete: true
(Reporter)

Comment 15

2 years ago
mozreview-review
Comment on attachment 8797234 [details]
Bug 1304263 - On Windows, fire the eMouseLongTap event as soon as APZ detects the long-press.

https://reviewboard.mozilla.org/r/82834/#review81684

If I understand this correctly, this patch modifies only the behavior on Windows. Now eMouseLongTap will fire when the finger is still on the screen. After lifting the finger, contextmenu then fires. It matches the behavior like Fennec that the word will be selected before finger lifts. Sounds good to me.

While you're here, please help change the default value of `sUseLongTapInjector` to false in [1] to match the all.js changes. It'll be even better if you could help remove [2] and [3]. Thank you!

[1] http://searchfox.org/mozilla-central/rev/3411332d29d377bf86405fc3ea72461ea0cb0295/layout/base/AccessibleCaretEventHub.cpp#385
[2] http://searchfox.org/mozilla-central/rev/3411332d29d377bf86405fc3ea72461ea0cb0295/b2g/app/b2g.js#962-965
[3] http://searchfox.org/mozilla-central/rev/3411332d29d377bf86405fc3ea72461ea0cb0295/layout/base/tests/marionette/test_accessiblecaret_selection_mode.py#53
Attachment #8797234 - Flags: review?(tlin) → review+

Comment 16

2 years ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c1d5e994f6d
On Windows, fire the eMouseLongTap event as soon as APZ detects the long-press. r=TYLin

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7c1d5e994f6d
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.