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

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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

Comment 1

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

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?

Comment 2

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

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.

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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.

Comment 4

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

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

Comment 5

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

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 6

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

I filed bug 1306634 for the issue I mentioned in the previous comment.

Comment 7

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

Attachment: WIP

This is what I was considering. It would need more testing and stuff if we want to land it.

Comment 8

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

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.

Comment 9

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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.

Comment 10

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

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

Comment 11

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

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

Comment 12

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

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 13

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

Try push with this change is also looking good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=feee2f7d482e&group_state=expanded

Comment 14

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

Attachment: Bug 1304263 - On Windows, fire the eMouseLongTap event as soon as APZ detects the long-press.

On Windows, the contextmenu event is fired when the finger is lifted after a
long-press. However, there are various bits of code, such as the AccessibleCaret
or potential fixes for bug 1147335, which would benefit from knowing when the
long-press gesture was detected. By moving eMouseLongTap event up we can satisfy
that need. An alternative approach considered was to fire the eMouseLongTap
before the contextmenu on all platforms unconditionally, but that makes it harder
to implement platform-specific text selection behaviour the way we want. In
particular we would have to add an extra message or notification for non-Windows
platforms that initiated text selection if the contextmenu event was not
consumed.

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

Comment 15

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

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

https://hg.mozilla.org/mozilla-central/rev/7c1d5e994f6d