1631754 - PanZoomController erroneously returns INPUT_RESULT_HANDLED

Status: RESOLVED FIXED

(GeckoView :: IME, defect, P1)

Description

[Petru-Mugurel Lingurar [:petru]]

For websites which have their own event handlers GV still returns INPUT_RESULT_HANDLED in between the first returns for how an event was handled.

I don't know if this is because of the async evaluation or how the events are queued but seems to be happening with all such websites (like Twitter, Google Maps, etc).

A video of a swipe down gesture on Twitter which shows Fenix's pull down to refresh erroneously appearing - https://drive.google.com/file/d/1TgecheYMDoZ-nGlgInhwnp6BDwXs-28E/view?usp=sharing
in response to GV which returns

2020-04-21 14:13:56.277 17990-17990/org.mozilla.fenix.debug E/InputResult: INPUT_RESULT_HANDLED_CONTENT
2020-04-21 14:13:56.324 17990-17990/org.mozilla.fenix.debug E/InputResult: INPUT_RESULT_HANDLED
2020-04-21 14:13:56.341 17990-17990/org.mozilla.fenix.debug E/InputResult: INPUT_RESULT_HANDLED
2020-04-21 14:13:56.357 17990-17990/org.mozilla.fenix.debug E/InputResult: INPUT_RESULT_HANDLED_CONTENT
2020-04-21 14:13:56.389 17990-17990/org.mozilla.fenix.debug E/InputResult: INPUT_RESULT_HANDLED_CONTENT
2020-04-21 14:13:56.406 17990-17990/org.mozilla.fenix.debug E/InputResult: INPUT_RESULT_HANDLED_CONTENT
2020-04-21 14:13:56.472 17990-17990/org.mozilla.fenix.debug E/InputResult: INPUT_RESULT_HANDLED_CONTENT
2020-04-21 14:13:56.488 17990-17990/org.mozilla.fenix.debug E/InputResult: INPUT_RESULT_HANDLED_CONTENT
2020-04-21 14:13:56.592 17990-17990/org.mozilla.fenix.debug E/InputResult: INPUT_RESULT_HANDLED_CONTENT

The above logs are the result of using

// Execute event handler from parent class in all cases
inputResult = handleEvent(event)
Log.e("InputResult", when(inputResult) {
    1 -> "INPUT_RESULT_HANDLED"
    2 -> "INPUT_RESULT_HANDLED_CONTENT"
    else -> "INPUT_RESULT_UNHANDLED"
})

here - https://github.com/mozilla-mobile/android-components/blob/cd134a34c94a4419167aa55b7c09babc9b6f60d8/components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/NestedGeckoView.kt#L67
and so logging the return of GV's onTouchEventForResult(event)

Comment 1

[Petru-Mugurel Lingurar [:petru]]

The same can be seen in geckoview-example when scrolling - https://treeherder.mozilla.org/#/jobs?repo=try&revision=669015980b228bcf3d0603bf55710d3eba6231d0

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Petru, for the dynamic toolbar they're making the decision to move based only on the result of the first ACTION_DOWN event. I think you'll probably have better results if you try the same.

Still, this kind of behavior seems wrong. Botond, can you take a look?

Comment 3

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

Based on the log pasted above I would imagine that we're hitting this exit condition and that is triggering this code which is missing the result.mTargetIsRoot check that we do later. The eConsumeNoDefault codepath can be coming from either content or chrome, so we should be checking mTargetIsRoot there.

You can verify if you run with MOZ_LOG=apz.inputqueue:4 and see if you get the log about slop from here turning up for those two events.

Comment 4

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

Might also be this exit condition which is equivalent to the other one I linked to.

Comment 5

[Sawyer Blatz [:sblatz]]

If you quickly scroll the embedded playlist on this site, you'll get what I believe is the the same issue: https://newsroom.spotify.com/2018-09-04/how-to-embed-spotifys-play-button/

The same happens on github projects like: https://github.com/mozilla-mobile/fenix/projects

Comment 6

[Sawyer Blatz [:sblatz]]

Here's a demo & some logs of this happening on the Spotify embedded playlist. Note this is all on ACTION_DOWN press. Seems like fast movement seems to make it more reproducible.

https://drive.google.com/file/d/1yjgY9vZK26i2lpw0VIjlAMVeY0x5CZFQ/view?usp=sharing

Comment 7

[Petru-Mugurel Lingurar [:petru]]

apz.inputqueue:4 logs show my issue does indeed follow the steps @kats described in comment 3!

But for the scenario Sawyer described
(quick scroll in an iframe like this) I saw that we are actually returning nsEventStatus_eConsumeDoDefault from here but then we get to here where based on result.mTargetIsRoot we erroneously return INPUT_RESULT_HANDLED.

Comment 8

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

@petru: the links in your previous comment are kinda broken, the markdown has nested brackets.

I'm also not totally clear on what you said - it sounds like there are two problems? The first being what I described earlier, and the second being a different issue producing the scenario Sawyer described. The second one seems like maybe mTargetIsRoot is true when you're expecting it to be false. Can you confirm my interpretation is correct?

Comment 9

[Chenxia Liu [:liuche]]

We're looking at doing a major Fennec/Fenix comparison experiment in mid-May - would this change get in? I'm sure we'll see lots of complaints from users that bottom toolbar isn't auto-hiding, taking up browsing space, etc. but we can't ship with this if the toolbar isn't going to show, bc that would be a security risk.

At the same time, I don't have a good view of what other priorities or tradeoffs are, so happy to let Emily/Jeff chime in here.

Comment 10

[vzare]

+1 prioritizing this bug. Assuming this will address the major pain points with the bottom toolbar/scroll behaviour, this is a high priority issue for Fenix.

Comment 11

[Emily Toop (:fluffyemily)]

Consult with snorp about whether to move this to graphics

Comment 13

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)

I'm also not totally clear on what you said - it sounds like there are two problems? The first being what I described earlier, and the second being a different issue producing the scenario Sawyer described. The second one seems like maybe mTargetIsRoot is true when you're expecting it to be false. Can you confirm my interpretation is correct?

Assuming this is the case, one potential issue here may be that we are setting mTargetIsRoot based on the tentative target, before any main-thread confirmation. If we're hitting a DTC region, we may report mTargetIsRoot=true but end up scrolling a subframe that gets activated.

If GV would like us to only report mTargetIsRoot=true if we know for sure we're going to be scrolling the root element, we should adjust our computation to be "(target apzc is the root) AND (did not hit a DTC region)".

Comment 14

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #13)

If GV would like us to only report mTargetIsRoot=true if we know for sure we're going to be scrolling the root element, we should adjust our computation to be "(target apzc is the root) AND (did not hit a DTC region)".

(Technically, only two of the DTC reasons ("irregular area" and "inactive scrollframe") mean the target is ambiguous. However, observing that GV already checks for the third reason, "APZ-aware listeners", via the sister flag mHitRegionWithApzAwareListeners, and uses it for the same purpose (returning INPUT_RESULT_HANDLED_CONTENT), we may as well just combine the two flags and check for any DTC reason.)

Comment 15

[Petru-Mugurel Lingurar [:petru]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)

@petru: the links in your previous comment are kinda broken, the markdown has nested brackets.

I'm also not totally clear on what you said - it sounds like there are two problems? The first being what I described earlier, and the second being a different issue producing the scenario Sawyer described. The second one seems like maybe mTargetIsRoot is true when you're expecting it to be false. Can you confirm my interpretation is correct?

Indeed, to me it seems like there are two situations in which PZC returns INPUT_RESULT_HANDLED instead of INPUT_RESULT_HANDLED_CONTENT:

• first, the one for which I opened this ticket which was explained by you in https://bugzilla.mozilla.org/show_bug.cgi?id=1631754#c3
• second, based on Sawyers report, which I tried to follow in https://bugzilla.mozilla.org/show_bug.cgi?id=1631754#c7 shows mTargetIsRoot being true when it actually should be false. If needed I can open a new ticket for this.

And, importantly there is another issue which shows the opposite, INPUT_RESULT_HANDLED_CONTENT being returned instead of the expected INPUT_RESULT_HANDLED on some specific sites - bug 1633322

Comment 16

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

Ok, thanks for explaining. The patch for the first issue here is pretty simple, as I described already in comment 3. The second one (based on Sawyers report) might be explained by what Botond said, although we should verify that. If it's not explained by that we would need to investigate a bit more. It seems suspicious to me that the speed of the panning action affects the output; the only reason I can think that might be the case would be the fast fling detection code and that would also be handled by the first patch.

At any rate, the last issue (bug 1633322) seems like a different problem and needs investigation.

Comment 17

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

Attachment: Bug 1631754 - Improve code to distinguish between root and content handling of events. r?snorp

In particular, the eConsumeNoDefault case also needs to check for mTargetIsRoot
and the apz aware listeners flag.

Comment 18

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

Patch seems to fix both the issue in comment 0 (tested on twitter.com) and Sawyer's issue (tested on the spotify playlist). I tested using GVE.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f049c6635dc2
Improve code to distinguish between root and content handling of events. r=snorp,geckoview-reviewers

Comment 20

[Dorel Luca [:dluca]]

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

Comment 21

[Chris Peterson [:cpeterson]]

Moving some input bugs to the new GeckoView::IME component.