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)
Tracking
(firefox70 disabled, firefox71+ fixed, firefox72 verified)
People
(Reporter: hiro, Assigned: botond, NeedInfo)
Details
Attachments
(4 files)
47 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
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
Reporter | ||
Comment 1•1 year ago
|
||
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.
Assignee | ||
Comment 2•1 year ago
|
||
Ah, Christian was just running into this the other day. Thanks for filing!
Assignee | ||
Comment 3•1 year ago
|
||
(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.
Assignee | ||
Updated•1 year ago
|
Reporter | ||
Comment 4•1 year ago
|
||
(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. :)
Assignee | ||
Comment 5•1 year ago
|
||
Likewise for APZCTreeManager::mFixedPosSidesForInputBlock.
Assignee | ||
Comment 6•1 year ago
|
||
The offset is applied (added) during rendering; it needs to be un-applied
(subtracted) during hit testing.
Depends on D50360
Assignee | ||
Comment 7•1 year ago
|
||
This ensures the coordinates of tap events are adjusted as well.
Depends on D50361
Assignee | ||
Comment 8•1 year ago
|
||
Depends on D50362
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
![]() |
||
Comment 10•1 year ago
|
||
bugherder |
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•1 year ago
•
|
||
firefox71=wontfix. We won't need to uplift this change to GV Beta (71) because Fenix hasn't re-enabled the dynamic toolbar yet.
Assignee | ||
Comment 12•1 year ago
|
||
Based on a discussion with Christian, I believe we do want this on 71 after all.
Assignee | ||
Comment 13•1 year ago
|
||
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/.
- 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:
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Updated•1 year ago
|
Comment 14•1 year ago
|
||
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.
Updated•1 year ago
|
Comment 15•1 year ago
|
||
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.
Updated•1 year ago
|
Updated•1 year ago
|
Comment 16•1 year ago
|
||
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.
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Comment 17•1 year ago
|
||
bugherderuplift |
Comment 18•1 year ago
|
||
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.
Description
•