Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 45

Status

()

Core
Panning and Zooming
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: capella, Assigned: kats)

Tracking

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

MozReview Requests

()

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

Attachments

(6 attachments)

40 bytes, text/x-review-board-request
rbarker
: review+
Details | Review
40 bytes, text/x-review-board-request
botond
: review+
Details | Review
40 bytes, text/x-review-board-request
botond
: review+
rbarker
: review+
Details | Review
40 bytes, text/x-review-board-request
rbarker
: review+
Details | Review
40 bytes, text/x-review-board-request
rbarker
: review+
Details | Review
40 bytes, text/x-review-board-request
botond
: review+
Details | Review
(Reporter)

Description

2 years ago
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
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.
(Reporter)

Comment 2

2 years ago
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.
(Reporter)

Comment 3

2 years ago
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?
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).
Argh! Seems like my build doesn't have MOZ_ANDROID_APZ defined :(
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.
I'm looking into the best way to fix this up.
Assignee: nobody → bugmail.mozilla
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.
(Reporter)

Comment 9

2 years ago
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.)
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.
Created attachment 8686754 [details]
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
Attachment #8686754 - Flags: review?(rbarker)
Created attachment 8686755 [details]
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
Attachment #8686755 - Flags: review?(botond)
Created attachment 8686756 [details]
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
Attachment #8686756 - Flags: review?(rbarker)
Attachment #8686756 - Flags: review?(botond)
Created attachment 8686757 [details]
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
Attachment #8686757 - Flags: review?(rbarker)
Created 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

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
Attachment #8686758 - Flags: review?(rbarker)
Created attachment 8686759 [details]
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
Attachment #8686759 - Flags: review?(botond)
Blocks: 1224325
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b99f5f03efe
Attachment #8686754 - Flags: review?(rbarker) → review+
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 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
Attachment #8686756 - Flags: review?(rbarker) → review+
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
Attachment #8686757 - Flags: review?(rbarker) → review+
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
Attachment #8686758 - Flags: review?(rbarker) → review+
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
Attachment #8686755 - Flags: review?(botond) → review+
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
Attachment #8686756 - Flags: review?(botond) → review+
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'?
Attachment #8686759 - Flags: review?(botond)
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/
Attachment #8686754 - Attachment description: MozReview Request: Bug 1223296 - Extract the function that dispatches a MozMouseHittest into a helper. r?rbarker → MozReview Request: Bug 1223296 - Extract the function that dispatches a MozMouseHittest into a helper. r=rbarker
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/
Attachment #8686755 - Attachment description: MozReview Request: Bug 1223296 - Turn HandlePanStart into a more generic function that can be called from other places. r?botond → MozReview Request: Bug 1223296 - Turn HandlePanStart into a more generic function that can be called from other places. r=botond
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/
Attachment #8686756 - Attachment description: MozReview Request: Bug 1223296 - Fire the MozMouseHittest event even if the C++ APZ is enabled. r?rbarker,botond → MozReview Request: Bug 1223296 - Fire the MozMouseHittest event even if the C++ APZ is enabled. r=rbarker,botond
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/
Attachment #8686757 - Attachment description: MozReview Request: Bug 1223296 - Fire a Gesture:SingleTap message to browser.js even on the C++ APZ codepath. r?rbarker → MozReview Request: Bug 1223296 - Fire a Gesture:SingleTap message to browser.js even on the C++ APZ codepath. r=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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25075/diff/1-2/
Attachment #8686758 - Attachment description: 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 → 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
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/
Attachment #8686759 - Attachment description: MozReview Request: Bug 1223296 - Clear element activation if a contextmenu is displayed. r?botond → MozReview Request: Bug 1223296 - Clear element activation if a contextmenu is displayed. r=
Attachment #8686759 - Flags: review?(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
Attachment #8686759 - Flags: review?(botond) → review+

Comment 32

2 years ago
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
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.