Closed Bug 1590582 Opened 2 months ago Closed 2 months ago

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

Categories

(GeckoView :: General, defect)

Unspecified
All
defect
Not set

Tracking

(firefox70 disabled, firefox71+ fixed, firefox72 verified)

VERIFIED FIXED
mozilla72
Tracking Status
firefox70 --- disabled
firefox71 + fixed
firefox72 --- verified

People

(Reporter: hiro, Assigned: botond, NeedInfo)

Details

Attachments

(4 files)

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

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.

Flags: needinfo?(botond)

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

(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.

Flags: needinfo?(botond)
Assignee: nobody → botond

(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. :)

Likewise for APZCTreeManager::mFixedPosSidesForInputBlock.

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

Depends on D50360

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

Depends on D50361

Pushed by bballo@mozilla.com:
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

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

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

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/.
  1. 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:
Attachment #9103744 - Flags: approval-mozilla-beta?
Attachment #9103745 - Flags: approval-mozilla-beta?
Attachment #9103746 - Flags: approval-mozilla-beta?
Attachment #9103747 - Flags: approval-mozilla-beta?
Flags: qe-verify+

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.

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.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]

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.

Attachment #9103744 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9103745 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9103746 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9103747 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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.

Flags: needinfo?(andrei.bodea)
You need to log in before you can comment on or make changes to this bug.