1223296 - "Gesture:SingleTap" fails on Fennec when C++ APZ is enabled

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Mark Capella [:capella]]

Testing nightly built with |--enable-android-apz| breaks TextSelection

STR:
   See [0] built w/o |--enable-android-apz|
   See [1] build w   |--enable-android-apz|

In both cases, first tap to end-of-field in Google search:

   Focuses the editable, sets nsCaret visible, and initiates autoSuggest composition highlighting.

The second tap to end-of-field:

   w/o APZ build, clears the autoSuggest composition, leaves nsCaret visible, and opens the ActionBar, enabling a visible TextSelection (orange) caret.
   w/  APZ build, clears the autoSuggest composition and leaves nsCaret visible, but fails to trigger the TextSelection ActionBar.

I tried building with the patch for |Bug 1195553 - Double tap gesture fails on Fennec when C++ APZ is enabled| on the off-chance the two |Gesture| bugs are related, but I get the same results.

[0] https://www.dropbox.com/s/3zhvqgwowireje2/nonAPZ_Google.mp4?dl=0
[1] https://www.dropbox.com/s/54h5nblwquae2ir/APZ_Google.mp4?dl=0

Comment 1

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

This is fixed with one or more of Randall's patches. I have a build with his stuff and it doesn't have this problem, whereas it does happen without his stuff.

Comment 2

[Mark Capella [:capella]]

Hmm, k ... I have the following in my build, and still see this, so I've missed something :/

bug1223614.diff: Bug 1223614 - Enabling native selection handles in Fennec triggers assert when non-visible handles are moved.
bug1223612.diff: Bug 1223612 - Enabling native selection handles in Fennec triggers assert.
bug1223609.diff: Bug 1223609 - Update MotionEventHelper.java to work with C++APZ
bug1223486.diff: Fix WrapTexCoord so that it returns 0 instead of 1 for negative whole numbers
bug1223436.diff: Fix AsyncCompositionManager so that mLayersUpdated is only set to false after SyncFrameMetrics has been called
bug1223434.diff: Fix Java Selection handles to work with C++APZ
bug1223433.diff: Enable axis locking
bug1220925.diff: Bug 1220925 - Event::GetScreenCoords should return CSSIntPoint instead of LayoutDevicePoint
bug1122804_p1.diff: Bug 1122804 - Have nsDisplayResolution items adjust event coordinates for hit testing and dispatching to content
bug1122804_p2.diff: Fix base widget so that wheel events from nsDOMWindowUtil that are dispatched where controller thread != main thread are handled correctly
bug1122804_p4.diff: Promote setIsLongpressEnabled to PAnZoomController interface so that casting to JavaPanZoomController is no longer needed
bug1195553.diff: Bug 1195553 - Double tap gesture fails on Fennec when C++ APZ is enabled.

Comment 3

[Mark Capella [:capella]]

With (--enable-android-apz / MOZ_ANDROID_APZ) builds, we use NativePanZoomController vs the original JavaPanZoomController.

With the original JavaPanZoomController seeming to be the only source of Gesture:SingleTap messages (grabbed by browser.js and used to trigger TextSelection orange-caret), this issue then makes sense.

LongPress actions work in both (Native APZ/JAVA APZ) builds, because they're triggered differently.

With original JavaPanZoomController.onLongPress(), we generate AndroidGeckoEvent::LONG_PRESS that nsWindow uses to trigger an eContextMenu event which |browser.contextmenus.show()| uses to initiate contextmenu or failing that TextSelection.

With the new NativePanZoomController, nsContentUtils::SendMouseEvent() is responsible for generating the eContextMenu event, but it gets to browser.contextmenus.show() in the same place.


So I see, initiation of mobile TextSelection (LongPress), works in both Native/Java builds, but via different code paths. Initiation of TextSelection (singleTap) into an editor fails in the new Native builds.

Have I missed a patch somewhere?

Comment 4

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

I haven't gone through your last two comments in detail yet but as of yesterday at least not all of Randall's patches were on bugs. He emailed me a patch queue which I had applied locally, and wasn't seeing this problem. I landed some of his patches yesterday and he put some of the other ones up on bugs since then. I have randall's patch queue rebased to latest master at https://github.com/staktrace/gecko-dev/commits/apz-fennec (I just rebased it and am building it now).

Comment 5

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

Argh! Seems like my build doesn't have MOZ_ANDROID_APZ defined :(

Comment 6

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

Ok, so with a proper android-apz build, I can reproduce this bug. Your logic in comment 3 seems correct; we'll need to find some way to trigger the TextSelection with the native APZ.

Comment 7

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

I'm looking into the best way to fix this up.

Comment 8

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

The top four patches at https://github.com/staktrace/gecko-dev/commits/9542f56 fix this, although I need to test them more since they might break other stuff.

Comment 9

[Mark Capella [:capella]]

Thanks! I see the message being generated on the JavaUI thread, but not being received by the observer in browser.js ... then I remembered this one of the four patches applied oddly for me:
https://github.com/staktrace/gecko-dev/commit/9542f56b223bbed037e160ff6c0d32d39b68590f

and as I work off m-c, backtracking showed I was being blocked here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=75bcd8adb694&mark=423-424#395

After fixing / accounting for an apparent patch there that I don't yet, solves this for me  \o/.

In any case, I'm starting to feel like the dog wagging the tail in this area  ;-)

I think I'll drop back a bit and let y'all tighten things up a bit more / migrate more bits, before I drag you further off in the weeds with my bug 1215959 / AccessibleCarets issues-that-might-not-be-issues.

Thanks for earlier pointing out that this should all be going live in nightly sometime soon. Is there a public target completion date you're looking at for bug 1206872? Or more like "asap-ish" ?

(I've been targeting 12/4 completion for my stuff, but may realistically push that back.)

Comment 10

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

Yeah, sorry I should have mentioned it applies on top of the patch in bug 1224015. As for completion date, we don't have an official one but some of us will be getting together in the Toronto office next week with the hopes of getting everything landed and turned on. So "asap-ish" sounds about right.

Comment 11

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

Attachment: MozReview Request: Bug 1223296 - Extract the function that dispatches a MozMouseHittest into a helper. r=rbarker

Bug 1223296 - Extract the function that dispatches a MozMouseHittest into a helper. r?rbarker

Comment 12

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

Attachment: MozReview Request: Bug 1223296 - Turn HandlePanStart into a more generic function that can be called from other places. r=botond

Bug 1223296 - Turn HandlePanStart into a more generic function that can be called from other places. r?botond

Comment 13

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

Attachment: MozReview Request: Bug 1223296 - Fire the MozMouseHittest event even if the C++ APZ is enabled. r=rbarker,botond

Bug 1223296 - Fire the MozMouseHittest event even if the C++ APZ is enabled. r?rbarker,botond

Comment 14

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

Attachment: MozReview Request: Bug 1223296 - Fire a Gesture:SingleTap message to browser.js even on the C++ APZ codepath. r=rbarker

Bug 1223296 - Fire a Gesture:SingleTap message to browser.js even on the C++ APZ codepath. r?rbarker

Comment 15

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

Attachment: MozReview Request: Bug 1223296 - Be more selective in disabling code in browser.js when the C++ APZ is enabled; in particular allow caret and zoomedview code to run. r=rbarker

Bug 1223296 - Be more selective in disabling code in browser.js when the C++ APZ is enabled; in particular allow caret and zoomedview code to run. r?rbarker

Comment 16

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

Attachment: MozReview Request: Bug 1223296 - Clear element activation if a contextmenu is displayed. r=

Bug 1223296 - Clear element activation if a contextmenu is displayed. r?botond

Comment 17

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

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b99f5f03efe

Comment 18

[Randall Barker [:rbarker]]

Comment on attachment 8686754 [details]
MozReview Request: Bug 1223296 - Extract the function that dispatches a MozMouseHittest into a helper. r=rbarker

https://reviewboard.mozilla.org/r/25067/#review22575

Comment 19

[Randall Barker [:rbarker]]

Comment on attachment 8686756 [details]
MozReview Request: Bug 1223296 - Fire the MozMouseHittest event even if the C++ APZ is enabled. r=rbarker,botond

https://reviewboard.mozilla.org/r/25071/#review22577

Comment 20

[Randall Barker [:rbarker]]

Comment on attachment 8686757 [details]
MozReview Request: Bug 1223296 - Fire a Gesture:SingleTap message to browser.js even on the C++ APZ codepath. r=rbarker

https://reviewboard.mozilla.org/r/25073/#review22579

Comment 21

[Randall Barker [:rbarker]]

Comment on attachment 8686758 [details]
MozReview Request: Bug 1223296 - Be more selective in disabling code in browser.js when the C++ APZ is enabled; in particular allow caret and zoomedview code to run. r=rbarker

https://reviewboard.mozilla.org/r/25075/#review22581

Comment 22

[Botond Ballo [:botond]]

Comment on attachment 8686755 [details]
MozReview Request: Bug 1223296 - Turn HandlePanStart into a more generic function that can be called from other places. r=botond

https://reviewboard.mozilla.org/r/25069/#review22675

Comment 23

[Botond Ballo [:botond]]

Comment on attachment 8686756 [details]
MozReview Request: Bug 1223296 - Fire the MozMouseHittest event even if the C++ APZ is enabled. r=rbarker,botond

https://reviewboard.mozilla.org/r/25071/#review22681

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8686759 [details]
MozReview Request: Bug 1223296 - Clear element activation if a contextmenu is displayed. r=

https://reviewboard.mozilla.org/r/25077/#review22683

::: gfx/layers/apz/util/APZEventState.cpp:241
(Diff revision 1)
>    if (!eventHandled) {

Maybe make this an 'else'?

Comment 25

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

Comment on attachment 8686754 [details]
MozReview Request: Bug 1223296 - Extract the function that dispatches a MozMouseHittest into a helper. r=rbarker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25067/diff/1-2/

Comment 26

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

Comment on attachment 8686755 [details]
MozReview Request: Bug 1223296 - Turn HandlePanStart into a more generic function that can be called from other places. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25069/diff/1-2/

Comment 27

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

Comment on attachment 8686756 [details]
MozReview Request: Bug 1223296 - Fire the MozMouseHittest event even if the C++ APZ is enabled. r=rbarker,botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25071/diff/1-2/

Comment 28

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

Comment on attachment 8686757 [details]
MozReview Request: Bug 1223296 - Fire a Gesture:SingleTap message to browser.js even on the C++ APZ codepath. r=rbarker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25073/diff/1-2/

Comment 29

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

Comment on attachment 8686758 [details]
MozReview Request: Bug 1223296 - Be more selective in disabling code in browser.js when the C++ APZ is enabled; in particular allow caret and zoomedview code to run. r=rbarker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25075/diff/1-2/

Comment 30

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

Comment on attachment 8686759 [details]
MozReview Request: Bug 1223296 - Clear element activation if a contextmenu is displayed. r=

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25077/diff/1-2/

Comment 31

[Botond Ballo [:botond]]

Comment on attachment 8686759 [details]
MozReview Request: Bug 1223296 - Clear element activation if a contextmenu is displayed. r=

https://reviewboard.mozilla.org/r/25077/#review22691

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f9e17c976a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9be262bb940
https://hg.mozilla.org/integration/mozilla-inbound/rev/3de8fd99080d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7b7aac00fd8
https://hg.mozilla.org/integration/mozilla-inbound/rev/3545c9188b5b
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb4eb1def874

Comment 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2f9e17c976a8
https://hg.mozilla.org/mozilla-central/rev/c9be262bb940
https://hg.mozilla.org/mozilla-central/rev/3de8fd99080d
https://hg.mozilla.org/mozilla-central/rev/b7b7aac00fd8
https://hg.mozilla.org/mozilla-central/rev/3545c9188b5b
https://hg.mozilla.org/mozilla-central/rev/bb4eb1def874