Closed Bug 1153076 Opened 5 years ago Closed 5 years ago

Modify SelectionCarets StartWord() logic to support Android

Categories

(Core :: DOM: Selection, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: capella, Assigned: capella)

Details

Attachments

(2 files)

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
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.
(And we can add MOZ_LONGTAP support in Fennec pretty easily if we need it before APZ)
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.
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
Attached patch bug1153076.diffSplinter Review
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 ...
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #8591320 - Flags: review?(bugmail.mozilla)
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.
Attachment #8591320 - Flags: review?(bugmail.mozilla) → review+
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?
Attachment #8591320 - Flags: review?(tlin)
(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
Flags: needinfo?(bugmail.mozilla)
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().
Attachment #8591320 - Flags: review?(tlin) → review+
Attached patch bug1153076.diffSplinter Review
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?
Attachment #8591475 - Flags: review?(tlin)
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.
Attachment #8591475 - Flags: review?(tlin) → review+
(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.
Flags: needinfo?(bugmail.mozilla)
https://hg.mozilla.org/mozilla-central/rev/b4d43073330d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.