Mark Capella [:capella] Assignee
	Description • 9 years ago

The current direction of bug 988143 (Enable text selection and touch caret in Gecko on Fennec) works fairly well with the SelectionCarets code as provided originally for FF/OS.

However, it seems the method used to ::StartWord() and make the SelectionCarets visible, will need adjusting for Android. We run into an issue when the user performs a long press on a link in content for example.

In this case, the new SelectionCaret code performs the word selection of the text in the link, and makes the SelectionCarets visible. Then, the java LongPress event sent to widget/android/nsWindow.cpp triggers our contextmenu. If the user selects an option, ("Open in new Tab") or cancels the dialog by back-press, we still wind up a selected word, and a visible ActionBar [0] as we have no way to .preventDefault along that path.

aiui, the main SelectWord() is initiated at the tail end of a timer triggered by its HandleEvent() in response to NS_TOUCH_START or NS_MOUSE_BUTTON_DOWN at [1]. Note, for APZ, there is a second path below triggered by NS_MOUSE_MOZLONGTAP [2] that I don't believe is involved for us, or planned for future support (?)

I have a naive WIP for this that I'm poking at on the side and will post for feedback as I clean it up.

I've discussed this with kats already (briefly) and am cc:ing anyone here who might be interested, especially margaret for priority/blocking decisions.


[0] https://www.dropbox.com/s/qlopi3o14pdffxe/bug988143_linkLongPress.png?dl=0
[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/SelectionCarets.cpp?rev=584132a97872&mark=229-229#204
[2] http://mxr.mozilla.org/mozilla-central/source/layout/base/SelectionCarets.cpp?rev=584132a97872&mark=229,273-273#226

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 1 • 9 years ago

I think the SelectionCarets code should not be reimplementing a long-tap gesture detector, we already have too many of those in the code. Do you under what circumstances/platforms that code is needed? AFAIK the SelectionCarets code is only enabled on B2G so far and so you should always get the MOZ_LONGTAP event from APZ.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 2 • 9 years ago

(And we can add MOZ_LONGTAP support in Fennec pretty easily if we need it before APZ)

	Mark Capella [:capella] Assignee
	Comment 3 • 9 years ago

There's probably nothing else using that code then, other than my initial version of a patch implementing Gecko Carets, and coming up to speed on the existing code, and future plans in that area.

Implementing that LONGTAP gesture for Fennec sounds like the right path then.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 4 • 9 years ago

To implement the longtap for fennec you basically need to dispatch the longtap event at around [1] after the contextmenu event is dispatched, and assuming the contextmenu event is not prevent-defaulted. There is similar code in the C++ APZ handler that you can crib from at [2] (don't need to worry about the ApplyCallbackTransform stuff though).

[1] http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp?rev=da2d02fa2472#869
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/util/APZEventState.cpp?rev=584132a97872#221

	Mark Capella [:capella] Assignee
	Comment 5 • 9 years ago

Got it! We'll extend from the previous work we did in Bug 1021804 - Long press on news story links invoke context menu while at the same time load the page.

The patch looks something like this ...

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 6 • 9 years ago

Comment on attachment 8591320 [details] [diff] [review]
bug1153076.diff

Review of attachment 8591320 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the widget bits but somebody else should review the caret changes.

	Mark Capella [:capella] Assignee
	Comment 7 • 9 years ago

Comment on attachment 8591320 [details] [diff] [review]
bug1153076.diff

Agreed. Asking tylin for r? as he's also involved with bug 988143, then we'll see who else should be consulted :)

I'll point out that the clause I added in ::HandleEvent() ...
   if (!sSelectionCaretDetectsLongTap) { ... }

is basically the same as the subsequent clause
   if (!mVisible) { ... }

I didn't know why the existing code seems to pass mDownPoint to ::SelectWord() based on unrelated earlier NS_TOUCH_START or NS_MOUSE_BUTTON_DOWN events, rather than from the NS_MOUSE_MOZLONGTAP event itself, as provided by APZEventState::ProcessLongTap().

Is there something here I miss?

	Ting-Yu Lin [:TYLin] (UTC-8)
	Comment 8 • 9 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)
> I think the SelectionCarets code should not be reimplementing a long-tap
> gesture detector, we already have too many of those in the code. Do you
> under what circumstances/platforms that code is needed? AFAIK the
> SelectionCarets code is only enabled on B2G so far and so you should always
> get the MOZ_LONGTAP event from APZ.

Although B2G could get MOZ_LONGTAP event from APZ now. I personally still use the long-tap detector on desktop browser or B2G desktop which has no APZ for developing. Gaia integration tests and marionette tests for carets [1] runs on all available platform will still need the detector if the platform has no APZ as well.

Kats, does enabling APZ implies having MOZ_LONGTAP events? If so, we can remove the long-tap detector once all the platforms have APZ support.

[1] https://hg.mozilla.org/mozilla-central/annotate/dd32e3ff3717/layout/base/tests/marionette/test_selectioncarets.py#l77

	Ting-Yu Lin [:TYLin] (UTC-8)
	Comment 9 • 9 years ago

Comment on attachment 8591320 [details] [diff] [review]
bug1153076.diff

Review of attachment 8591320 [details] [diff] [review]:
-----------------------------------------------------------------

The caret changes looks good to me.

> I didn't know why the existing code seems to pass mDownPoint to
> ::SelectWord() based on unrelated earlier NS_TOUCH_START or
> NS_MOUSE_BUTTON_DOWN events, rather than from the NS_MOUSE_MOZLONGTAP event
> itself, as provided by APZEventState::ProcessLongTap().

If long-tap detector is triggered instead of receiving NS_MOUSE_MOZLONGTAP event, we only have mDownPoint from the previous event for SelectWord().

	Mark Capella [:capella] Assignee
	Comment 10 • 9 years ago

THanks :tylin  !

I see that when the long-tap detector is triggered, we only have mDownPoint from previous touch/mouse events.

I wonder if this version of the patch, with code shared by APZ and Android, and using the mDownPoint/ptInRoot from the NS_MOUSE_MOZLONGTAP event itself would regress B2G?

I suspect this is more accurate and concise, and wonder if you'd care to approve this version instead?

	Ting-Yu Lin [:TYLin] (UTC-8)
	Comment 11 • 9 years ago

Comment on attachment 8591475 [details] [diff] [review]
bug1153076.diff

Review of attachment 8591475 [details] [diff] [review]:
-----------------------------------------------------------------

If the LONGTAP event is available, it will better to use the point from the event. Thanks for improving this.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 12 • 9 years ago

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #8)
> Although B2G could get MOZ_LONGTAP event from APZ now. I personally still
> use the long-tap detector on desktop browser or B2G desktop which has no APZ
> for developing. Gaia integration tests and marionette tests for carets [1]
> runs on all available platform will still need the detector if the platform
> has no APZ as well.

Ah, I see. Thanks for the explanation!

> Kats, does enabling APZ implies having MOZ_LONGTAP events? If so, we can
> remove the long-tap detector once all the platforms have APZ support.

Yes, once APZ is enabled you should be getting MOZ_LONGTAP events everywhere.

	Mark Capella [:capella] Assignee
	Comment 13 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=709683eb1e71

	Mark Capella [:capella] Assignee
	Comment 14 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/b4d43073330d

	Carsten Book [:Tomcat]
	Comment 15 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/b4d43073330d