Closed Bug 1528937 Opened 5 years ago Closed 4 years ago

Make HandleTap work in Fission

Categories

(Core :: DOM: UI Events & Focus Handling, enhancement, P3)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
84 Branch
Project Flags:
Fission Milestone M6c
Tracking Flags:
Tracking Status
firefox84 --- fixed

People

(Reporter: hsivonen, Assigned: hsivonen)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fission-event-backlog])

Attachments

(1 file)

Bug 1528937 - Test that taps are transformed correctly for out-of-process iframes.
47 bytes, text/x-phabricator-request
Details | Review

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)

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

Fission Milestone: --- → ?

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)

Fission Milestone: ? → M6

This bug is a Fission Nightly blocker (milestone M6c).

Fission Milestone: M6 → M6c

Henri, what are the next steps to close this?

Assignee: nobody → hsivonen
Status: NEW → ASSIGNED

Rebase WFM locally despite failing on try.

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
Flags: needinfo?(hsivonen) → needinfo?(kats)

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.

Flags: needinfo?(kats)

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

See Also: → 1672237

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

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

https://hg.mozilla.org/mozilla-central/rev/ddaab9f4a941

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox84: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: