Make HandleTap work in Fission
Categories
(Core :: DOM: UI Events & Focus Handling, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox84 | --- | fixed |
People
(Reporter: hsivonen, Assigned: hsivonen)
References
(Blocks 1 open bug)
Details
(Whiteboard: [fission-event-backlog])
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
Some touch-related events are dispatched via TabParent::SendHandleTap, which does not use a WidgetEvent.
Since the current Fission event delivery story is built around WidgetEvent, there's a need to investigate what alternative design is needed for this WidgetEvent-less API.
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
I expect the patch for bug 1529237 to fix this if bug 1529239 is feasible to fix.
Comment 2•6 years ago
|
||
Having an offline IRC discussion with Henri - If we disable touch events on desktop, is this bug still desktop-relevant?
Comment 3•6 years ago
|
||
Even if you disable dispatching touch events to web content, we probably still want to keep them internally for detecting taps and long-presses and other touch gestures. And this bug is still relevant in those scenarios.
Comment 4•6 years ago
|
||
Touch wouldn't be really disabled. As kats says, all touch handling would still run internally. It is just that web pages won't see the events.
Updated•6 years ago
|
Comment 5•5 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #1)
I expect the patch for bug 1529237 to fix this if bug 1529239 is feasible to fix.
Do we still need anything else in this bug, given bug 1529237 was resolved?
Assignee | ||
Comment 6•5 years ago
|
||
This might already be fixed, but I'm not sure how to test.
smaug, can I test this on Ubuntu without an actual touch device?
Comment 7•5 years ago
|
||
You could modify/clone this test to use fission. Getting the fission variant landed in-tree would be nice too :)
The test uses synthesized touch events, which works even on non-touch devices.
Assignee | ||
Comment 9•5 years ago
|
||
Thanks. Adding a dependency on bug 1550467 for testing infra.
Comment 10•5 years ago
|
||
Bug 1550467 has landed and stuck, so this bug should be actionable now.
Assignee | ||
Comment 12•5 years ago
|
||
(In reply to Neha Kochar [:neha] from comment #11)
Henri, is this something you can look into?
Looking into this now.
Assignee | ||
Comment 13•5 years ago
|
||
Looks like we already do the transforms if we get the right BrowserParent
:
https://searchfox.org/mozilla-central/rev/0671407b7b9e3ec1ba96676758b33316f26887a4/dom/ipc/BrowserParent.cpp#1824-1827
https://searchfox.org/mozilla-central/rev/0671407b7b9e3ec1ba96676758b33316f26887a4/gfx/layers/ipc/APZCTreeManagerChild.cpp#136 looks like we might already be getting the right BrowserParent
.
I'll look into writing a test case using the infra from bug 1550467.
Meanwhile, it would be great to know if this actually works in practice, but I don't have a touch-enabled Windows device.
smaug, with Fission enabled, can you tap the buttons that say "Button" in https://hsivonen.fi/fission-host.html using touch on Windows?
Assignee | ||
Comment 15•5 years ago
|
||
Assignee | ||
Comment 16•5 years ago
|
||
The click
event works for me with synthetic taps, and I verified that they actually go through HandleTap
. pointerup
didn't work for me, but it's quite possible that I was testing wrong, and my test failure doesn't indicate anything about Fission.
Assignee | ||
Comment 17•5 years ago
|
||
Comment 18•5 years ago
|
||
Comment 19•5 years ago
|
||
Backed out for bc failures on helper_fission_tap.html
Backout link: https://hg.mozilla.org/integration/autoland/rev/8ad5fbc5b9358fc84aa43d9a1b19c851056b1f39
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=254671824&repo=autoland&lineNumber=6180
Assignee | ||
Comment 20•5 years ago
|
||
Whoa! The TV results for the try run came in after I thought the try run was done. Sorry.
I assume that the outer harness imposes a timeout on browser_test_group_fission.js
collectively, so adding another item within the browser_test_group_fission.js
inner harness pushed the collective above the outer timeout. :-(
Comment 21•5 years ago
|
||
That's my fault, sorry. I triggered those TV jobs after you put the patch up for review but before r+'ing, because I wasn't sure if the test would pass without WR. Except then I apparently didn't wait for the windows job to finish. Whoops.
That being said, the failure that led to the backout is different from the TV failure on the try push. I don't think it's a timeout problem per se, if you look at the timestamps there's a big gap after the event is dispatched where nothing happens. I suspect the test might just intermittently lose the events because we're not setting things up properly for touch events. I'll take another look and see if I can spot any problems.
Comment 22•5 years ago
|
||
So from the log in comment 19 it looks like the tap was dispatched, but neither the OOPIF nor the hosting page received a click event. What might have happened is that more than 400ms elapsed between the touchstart event and touchend event, which would exceed the apz.max_tap_time
interval and turn the tap into a long-tap, which will not trigger the click event. If this is the case, then we should bump up the apz.max_tap_time
pref value to something high (e.g. 10000) for this subtest.
For the TV failure on the try push - I think what happened there is what I was worried about, which is that there's something in the non-WR codepath that makes the test fail when it would normally pass on the WR codepath. I have bug 1560312 on file for this, so maybe for now we should just make the test conditional on WR.
Also as a related footnote, there is no TV job for Windows-QR (see bug 1453478) so it'll be hard to verify the max_tap_time fix without manual retriggers on the try push.
Comment 23•5 years ago
|
||
With respect to setting a pref just on the subtest, it would be good to generalize the test_urls
array so that it's an array of objects similar to how the mochitest-plain test_group_*
tests work. e.g.:
var test_urls = [
{ "file": httpURL("helper_fission_basic.html") },
{ "file": httpURL("helper_fission_tap.html"), "prefs": [
["apz.max_tap_time", 10000],
]},
// add additional tests here
];
And then do a pushPrefEnv
/popPrefEnv
inside the test_urls
loop.
Comment 24•5 years ago
|
||
There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:hsivonen, could you have a look please?
For more information, please visit auto_nag documentation.
Comment 25•5 years ago
|
||
Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage milestone.
This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:
0ee3c76a-bc79-4eb2-8d12-05dc0b68e732
Comment 26•5 years ago
|
||
Henri, what needs to be done to resolve this bug? Your patch landed but bounced.
Also as a related footnote, there is no TV job for Windows-QR (see bug 1453478) so it'll be hard to verify the max_tap_time fix without manual retriggers on the try push.
Fission will require WebRender, so we don't care about non-WebRender issues, if that matters for this bug.
Tracking for Fission Nightly (M6)
Comment 27•4 years ago
|
||
This bug is a Fission Nightly blocker (milestone M6c).
Comment 28•4 years ago
|
||
Henri, what are the next steps to close this?
Assignee | ||
Comment 29•4 years ago
|
||
Assignee | ||
Comment 30•4 years ago
|
||
Rebase WFM locally despite failing on try.
Assignee | ||
Comment 31•4 years ago
|
||
Assignee | ||
Comment 32•4 years ago
|
||
Assignee | ||
Comment 33•4 years ago
|
||
Assignee | ||
Comment 34•4 years ago
|
||
Assignee | ||
Comment 35•4 years ago
|
||
This still works for me locally, but fails on try. kats, do you have ideas of how TypeError: eventTarget.addEventListener is not a function
happens? See the logging here: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=319027720&repo=try&lineNumber=15600 . Note how the dump
calls around addEventListener
end up logging:
[task 2020-10-19T17:14:52.014Z] 17:14:52 INFO - GECKO(5216) | About to call addEventListener in code_for_oopif_to_run: document: [object HTMLDocument] addEventListener: function addEventListener() {
[task 2020-10-19T17:14:52.014Z] 17:14:52 INFO - GECKO(5216) | [native code]
[task 2020-10-19T17:14:52.015Z] 17:14:52 INFO - GECKO(5216) | }
[task 2020-10-19T17:14:52.016Z] 17:14:52 INFO - GECKO(5216) | OOPIF registered click listener
Comment 36•4 years ago
|
||
I applied your patch locally and ran it (./mach mochitest --keep-open=false gfx/layers/apz/test/mochitest/browser_test_group_fission.js
) and although the mochitest harness reports everything passing, I do see the same TEST-UNEXPECTED-FAIL
and error message in the test output if I scroll back. I'm not sure why the mochitest harness doesn't pick up on the failure, maybe because it's some sort of exception caught by SimpleTest in the content process that doesn't get propagated to the parent process.
But the exception is coming from the promiseOneEvent("OOPIF:ClickData", ...)
call which is passing two arguments to a function that expects three arguments. And this line is the addEventListener call that's failing. Changing that call to promiseOneEvent(window, ...)
seems to fix it for me locally.
Comment 37•4 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #36)
I'm not sure why the mochitest harness doesn't pick up on the failure, maybe because it's some sort of exception caught by SimpleTest in the content process that doesn't get propagated to the parent process.
Patches for bug 1672237 will provide a solution to this (although your patch will need updating so that the .then(FissionTestHelper.subtestDone, FissionTestHelper.subtestDone)
is a .then(FissionTestHelper.subtestDone, FissionTestHelper.subtestFailed)
).
Assignee | ||
Comment 38•4 years ago
|
||
Thanks. This is now the second time in a relatively short while when the problem has been that I've trusted the test harness pass/fail output and the output has been wrong. I guess I will have to teach myself to assume that the test harness may say bogus things.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8369c1a29d1b8e3883ab177dc8777d5a0d8e8c66
Comment 39•4 years ago
|
||
Comment 40•4 years ago
|
||
bugherder |
Description
•