Re-enable touch event retargeting
Categories
(GeckoView :: General, enhancement)
Tracking
(firefox83 fixed)
| Tracking | Status | |
|---|---|---|
| firefox83 | --- | fixed |
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(2 files)
Touch event retargeting was enabled in bug 1637908 to fix a hard-to-grab vue.js slider. Unfortunately it broke the play button on the browser's built-in media playing controls, because the nsRangeFrame that is used in that scenario has a touch listener here. This was causing touch events directed at the play button to get retargeted to the range frame (media scrubber/slider thingy).
So the touch event retargeting was disabled in bug 1668112 to fix this problem.
This bug is to re-enable it after figuring out a good solution for the media control issue. I think we might be able to get rid of the explicit touch event listener and instead modify the CompositorHitTestInfo for the frame to have the eApzAwareListeners flag set. That should keep the existing behaviour of having APZ know the range frame handles touch events, and also not break the media controls because there won't be an explicit touch event listener anymore. We couldn't do this before because at the time we didn't have CompositorHitTestInfo at all, but now we can.
|
Assignee | |
Comment 1•5 years ago
|
||
So I wrote a patch to remove the DummyEventListener, but unfortunately it looks like there is another touchstart/touchend listener being registered on the range, coming from here. So it's still stealing touch events. I think there's a way forward here, perhaps by ignoring touch listeners that are in the system group. I'll play around with it a bit.
|
|
Assignee | |
Comment 2•5 years ago
|
||
So yes, ignoring touch listeners in the system group works. I think doing this is probably reasonable - if browser elements are harder to hit as a result, we should just make them bigger instead of relying on the event retargeting feature. Particularly since we only have event retargeting turned on for Android, and we can get touch inputs on other platforms.
|
|
Assignee | |
Comment 3•5 years ago
|
||
:esawin, is there a way to verify that these patches now pass the media tests that triggered bug 1668112?
|
|
Assignee | |
Comment 4•5 years ago
|
||
The audio/video controls element has touch listeners on the scrubber, which
steals events when we don't want it to. So let's ignore those listeners.
Ignoring system group listeners for the purposes of event retargeting seems
reasonable in the general case, because those listeners are coming from the
browser itself. If we're relying on the event retargeting to make those browser
elements easy to hit, then we should not be doing that and instead just make
them bigger.
Test coverage for this change is provided by the android-components tests that
failed in bug 1668112. The next patch re-enables touch event retargeting and
exercises this code in the context of those tests.
Depends on D92436
|
|
Assignee | |
Comment 5•5 years ago
|
||
Depends on D92437
|
||
Comment 6•5 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
:esawin, is there a way to verify that these patches now pass the media tests that triggered bug 1668112?
I don't think there is an automated way to run the Fenix UI tests, since that would involve updating A-C to the new GV and Fenix to the new A-C version.
However, since this issue is reproducing in GVE, we can at least (manually) verify it there, which is easier.
The issue is still reproducing in GVE with the patch from this bug applied.
|
|
Assignee | |
Comment 7•5 years ago
|
||
(In reply to Eugen Sawin [:esawin] from comment #6)
The issue is still reproducing in GVE with the patch from this bug applied.
Did you apply the patch from bug 1668905 as well as the two patches on this bug? When I used all three I couldn't reproduce in GVE using the STR from bug 1668112 comment 0.
|
|
||
Comment 8•5 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7)
(In reply to Eugen Sawin [:esawin] from comment #6)
The issue is still reproducing in GVE with the patch from this bug applied.
Did you apply the patch from bug 1668905 as well as the two patches on this bug? When I used all three I couldn't reproduce in GVE using the STR from bug 1668112 comment 0.
I've missed the bug 1668905 dependency, sorry. It wfm now locally.
|
||
Comment 10•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/4f7601289c22
https://hg.mozilla.org/mozilla-central/rev/3e5832b31812
Description
•