Open Bug 1528937 Opened 8 months ago Updated 3 months ago

Make HandleTap work in Fission

Categories

(Core :: User events and focus handling, enhancement, P3)

enhancement

Tracking

()

People

(Reporter: hsivonen, Unassigned, NeedInfo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fission-event-backlog])

Attachments

(1 file)

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.

Priority: -- → P3
Whiteboard: [fission-event-backlog]

I expect the patch for bug 1529237 to fix this if bug 1529239 is feasible to fix.

Depends on: 1529237
Depends on: 1530656

Having an offline IRC discussion with Henri - If we disable touch events on desktop, is this bug still desktop-relevant?

Flags: needinfo?(bugs)

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.

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.

Flags: needinfo?(bugs)
Depends on: 1530661
No longer depends on: 1530656
Component: Event Handling → User events and focus handling

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

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?

Flags: needinfo?(bugs)

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.

yeah, tests do work on non-touch devices too.

Flags: needinfo?(bugs)

Thanks. Adding a dependency on bug 1550467 for testing infra.

Depends on: 1550467

Bug 1550467 has landed and stuck, so this bug should be actionable now.

Henri, is this something you can look into?

Flags: needinfo?(hsivonen)

(In reply to Neha Kochar [:neha] from comment #11)

Henri, is this something you can look into?

Looking into this now.

Flags: needinfo?(hsivonen)

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?

Flags: needinfo?(bugs)

I don't have a Windows machine at hand right now.

Flags: needinfo?(bugs)

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.

Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9554ac45236f
Test that taps are transformed correctly for out-of-process iframes. r=kats

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

Flags: needinfo?(hsivonen)

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.

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.

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.

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.

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