1590582 - Tap events don't properly handle on position:fixed element on the main-thread in the case where the dynamic toolbar is visible

Status: VERIFIED FIXED

(GeckoView :: General, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

After bug 1552608, hit testing on the compositor works fine, but still event position passing back to the main-thread seems wrong. One thing I noticed is that we need to subtract the fixed margin instead of (adding it)[https://searchfox.org/mozilla-central/rev/8a63fc190b39ed6951abb4aef4a56487a43962bc/gfx/layers/apz/src/APZCTreeManager.cpp#1884], because on the main-thread position:fixed elements are not shifted the fixed margin, so they (some areas of them) are still under the dynamic toolbar.

Another thing I noticed after debugging is we still receive some mouse events with wrong position which comes from (ReceiveInputEvent call just before we tweak the position)[https://searchfox.org/mozilla-central/rev/8a63fc190b39ed6951abb4aef4a56487a43962bc/gfx/layers/apz/src/APZCTreeManager.cpp#1857-1860].

Moving the position tweak before the ReceiveInputEvent call fixes the problem locally, but I I am not sure it's reasonable or not.

Anyways, I just pushed a try with the change; https://hg.mozilla.org/try/rev/c786a6fb6cd04138605a895a63f24e2e54da24b8

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Botond, what is the proper way to fix this? ReceiveInputEvent sends events to the main-thread so that it seems to me that we need to tweak the position at least for position:fixed elements, but I am totally unsure about other cases.

Comment 2

[Botond Ballo [:botond]]

Ah, Christian was just running into this the other day. Thanks for filing!

Comment 3

[Botond Ballo [:botond]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)

One thing I noticed is that we need to subtract the fixed margin instead of adding it

Indeed, good catch. (I'm not sure how this worked in my testing. Possibly the method I was using to work around A-C not calling setVerticalClipping was not valid.)

Another thing I noticed after debugging is we still receive some mouse events with wrong position which comes from (ReceiveInputEvent call just before we tweak the position)[https://searchfox.org/mozilla-central/rev/8a63fc190b39ed6951abb4aef4a56487a43962bc/gfx/layers/apz/src/APZCTreeManager.cpp#1857-1860].

Moving the position tweak before the ReceiveInputEvent call fixes the problem locally, but I I am not sure it's reasonable or not.

Anyways, I just pushed a try with the change; https://hg.mozilla.org/try/rev/c786a6fb6cd04138605a895a63f24e2e54da24b8

We can't move the adjustment before ReceiveInputEvent, because ReceiveInputEvent expects the event's mScreenPoint to be in unmodified screen coordinates. (For example, it will call into MultiTouchInput::TransformToLocal which applies transforms to mScreenPoint).

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)

Botond, what is the proper way to fix this? ReceiveInputEvent sends events to the main-thread so that it seems to me that we need to tweak the position at least for position:fixed elements, but I am totally unsure about other cases.

I think the issue here is that the synthesized mouse events that trigger the button click, come from a tap event rather than the touch event. Tap events are dispatched using a different codepath (e.g. AsyncPanZoomController::GenerateSingleTap()), and we need to apply the fixed margins in that codepath as well.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)

Botond, what is the proper way to fix this? ReceiveInputEvent sends events to the main-thread so that it seems to me that we need to tweak the position at least for position:fixed elements, but I am totally unsure about other cases.

I think the issue here is that the synthesized mouse events that trigger the button click, come from a tap event rather than the touch event. Tap events are dispatched using a different codepath (e.g. AsyncPanZoomController::GenerateSingleTap()), and we need to apply the fixed margins in that codepath as well.

Yes! The events I saw came from GenerateSingleTap(), I couldn't tell the difference between touch and tap. :)

Comment 5

[Botond Ballo [:botond]]

Attachment: Bug 1590582 - Don't leave HitTestResult::mFixedPosSides uninitialized. r=tnikkel

Likewise for APZCTreeManager::mFixedPosSidesForInputBlock.

Comment 6

[Botond Ballo [:botond]]

Attachment: Bug 1590582 - Subtract the fixed margins offset when adjusting event coordinates during hit testing, rather than adding it. r=tnikkel

The offset is applied (added) during rendering; it needs to be un-applied
(subtracted) during hit testing.

Depends on D50360

Comment 7

[Botond Ballo [:botond]]

Attachment: Bug 1590582 - Un-apply the fixed margins offset in ConvertToGecko as well. r=tnikkel

This ensures the coordinates of tap events are adjusted as well.

Depends on D50361

Comment 8

[Botond Ballo [:botond]]

Attachment: Bug 1590582 - Have AsyncPanZoomController::ConvertToGecko return a Maybe rather than using a bool + outparam. r=tnikkel

Depends on D50362

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9094063323a3
Don't leave HitTestResult::mFixedPosSides uninitialized. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/aa1e247e0038
Subtract the fixed margins offset when adjusting event coordinates during hit testing, rather than adding it. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/fb87d73d3a49
Un-apply the fixed margins offset in ConvertToGecko as well. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/a0a5b420415c
Have AsyncPanZoomController::ConvertToGecko return a Maybe rather than using a bool + outparam. r=tnikkel

Comment 10

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9094063323a3
https://hg.mozilla.org/mozilla-central/rev/aa1e247e0038
https://hg.mozilla.org/mozilla-central/rev/fb87d73d3a49
https://hg.mozilla.org/mozilla-central/rev/a0a5b420415c

Comment 11

[Chris Peterson [:cpeterson]]

firefox71=wontfix. We won't need to uplift this change to GV Beta (71) because Fenix hasn't re-enabled the dynamic toolbar yet.

Comment 12

[Botond Ballo [:botond]]

Based on a discussion with Christian, I believe we do want this on 71 after all.

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 9103744 [details]
Bug 1590582 - Don't leave HitTestResult::mFixedPosSides uninitialized. r=tnikkel

Beta/Release Uplift Approval Request

• User impact if declined: With the dynamic toolbar enabled in Fenix or Reference Browser, user is not able to interact with content fixed to the bottom of the screen (e.g. a notification bar) via tap gestures (e.g. tapping on a button).
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. In Reference Browser, navigate to https://www.sozone.de/.

2. Tap the "OK" button on the green cookie notification bar at the bottom of the screen.

If the notification bar does not appear (e.g. because it has been dismissed before), you may need to test in a clean profile.

Expected result: the tap is registered and the notification bar disappears.
Actual results: the tap is not registered and the notification bar remains

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a small fix to the patch series that landed in bug 1552608 in the 71 cycle.
• String changes made/needed:

Comment 14

[Pascal Chevrel:pascalc]

Since there is no automated tests and we have STR I would like it to be verified by QA on nightly before taking an uplift to beta.

Comment 15

[Andrei Bodea[:andrei]]

Verified as fixed using:

• Fenix Nightly 191028 06:00 (Build #13010606)📦: 19.0.0, 6e6311ea4 🦎: 72.0a1-20191025095546 28/10 and
• Reference Browser 1.0.1943 (Build #13001206 GV: 72.0a1-20191025095546)📦: 19.0.0, 6e6311ea4 🚢: 0.42.0 27/10

Note that with the GV 72.0a1-20191025095546 everything works as expected and the tap is registered and the notification bar disappears(on - https://www.sozone.de/.)
Also, note that I tried to reproduce the issue using www.swiggy.com and everything worked fine.
In Fenix we have QAB enabled and you can see on the GIF that after ~1-2 seconds it disappears and the cookies notification bar disappears.
Current behavior Video.

Comment 16

[Pascal Chevrel:pascalc]

Comment on attachment 9103744 [details]
Bug 1590582 - Don't leave HitTestResult::mFixedPosSides uninitialized. r=tnikkel

Low risk, verified by QA on nightly, uplift approved for 71 beta 6, thanks.

Comment 17

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5f2b98bde759
https://hg.mozilla.org/releases/mozilla-beta/rev/59533843b4da
https://hg.mozilla.org/releases/mozilla-beta/rev/c59bc9863428
https://hg.mozilla.org/releases/mozilla-beta/rev/661f1e530689

Comment 18

[Andrei Bodea[:andrei]]

Still waiting for the fix to land in 71, and after that I will verify this issue and the one from Fenix repo Add EngineViewBottomBehavior to fragment_browser. #5946, the PR will be merge once the 71 will land.
NI myself until then.