w3c / gamepad

Gamepad
https://w3c.github.io/gamepad/
Other
139 stars 49 forks source link

gamepadconnected/disconnected events & non-"fully active" documents #149

Open rakina opened 3 years ago

rakina commented 3 years ago

Currently the gamepadconnected and gamepaddisconnected events fire on the (active document's?) window. It's a bit unclear how it interacts with non-"fully active" (bfcached) documents. I think it's better if the text specify that the window the event is fired to is the active document's window.

Also, it's probably a good chance to specify how bfcache restores are handled. Currently there's a text that says the gamepadconnected should fire "if a gamepad was already connected when the page was loaded". Does this include bfcache restore? What if a gamepad is connected then disconnected (possibly multiple times) while the document is in bfcache? Probably it makes sense to just send the last event (if it's different than the last dispatched event to the document) on bfcache restore (see our WIP guide).

cc @domenic @fergal

marcoscaceres commented 3 years ago

@rakina, should be ok to add, but I'm wondering if this doesn't apply generally to most (all?) events that get fired at the Window object? I'm wondering if maybe DOM should be providing some guidance here too? cc @annevk

rakina commented 3 years ago

Not sure if I get what you're suggesting correctly, but I think this applies to most events that come from "external sources" (e.g. devices), while "internal events" (changes to DOM?) are less of a problem because we won't run tasks/scripts within the non-fully active document itself. Providing general guidance on top of the TAG docs sounds good, how should we do that?

marcoscaceres commented 3 years ago

Not sure if I get what you're suggesting correctly, but I think this applies to most events that come from "external sources" (e.g. devices),

Right, yes.

while "internal events" (changes to DOM?) are less of a problem because we won't run tasks/scripts within the non-fully active document itself.

In the sentence above, who is "we" in "we won't run tasks"? Do you mean the browser or something specific that guards against that in a spec?

Providing general guidance on top of the TAG docs sounds good, how should we do that?

I guess we could have a go here (or in Geo) at preparing a pull request... then based on that, we can see if some general guidance is needed about how to write whatever prose is needed.

It's also really helpful that various specs have been identified as needing changes. We could maybe link to, or quote, the appropriate prose from those specs (e.g., Screen Wake Lock).

annevk commented 3 years ago

"We" is the processing model in HTML for how to implement a browser, I suppose. 😊

Looking at the prose there might be multiple problems here:

I suspect a lot of this follows from focus, perhaps? But none of that appears to be written down.

marcoscaceres commented 3 years ago

Ok, here is a fun test (code below): https://marcoscaceres.github.io/playground/gamepad_test.html

You can steal .getGamepads() and pull Gamepad objects from non-fully active documents.

Thankfully, "gamepadconnected" and "gamepadconnected" events won't fire on non-fully active documents.

So, I think we just need to add a check on .getGamepads() and we should be good.

However, it's not something we can test via WPT, because it requires an actual Gamepad... but this should be super easy to patch in each browser. I can take WebKit and Gecko.

<h2>Gamepad</h2>
<script>
  const bad = (ev) => console.log("Event on non-fully active frame!", ev);
  const good = (ev) => console.log("This is ok!", ev);
  async function attachIframe() {
    const iframe = document.createElement("iframe");
    iframe.src = "empty.html";
    document.body.appendChild(iframe);
    console.log("loading iframe");
    await new Promise((r) => {
      iframe.onload = r;
    });
    return iframe;
  }

  // Check events!
  attachIframe().then((iframe) => {
    document.body.appendChild(iframe);
    const win = iframe.contentWindow;
    win.addEventListener("gamepadconnected", bad);
    win.addEventListener("gamepaddisconnected", bad);
    iframe.remove();
  });

  // This is ok
  window.addEventListener("gamepadconnected", good);
  window.addEventListener("gamepaddisconnected", good);

  async function runTest() {
    const iframe = await attachIframe();
    // Prevent invalidation of Navigator
    const win = iframe.contentWindow;
    const gamepadGetter =
      iframe.contentWindow.navigator.getGamepads.bind(navigator);
    console.log("Still active, we get", gamepadGetter());
    // make doc no longer fully active
    iframe.remove();

    // try to use gamepad

    try {
      console.log("Inactive document", gamepadGetter());
    } catch (err) {
      console.log(err);
    }

    // re-attach
    document.body.appendChild(iframe);

    // and we are back!
    console.log("re-attached", iframe.contentWindow.navigator.getGamepads());

    //remove it again...
    iframe.contentWindow.addEventListener("gamepadconnected", console.log);

    iframe.contentWindow.addEventListener("gamepaddisconnected", console.log);
    iframe.remove();
    console.log("done");
  }
</script>

<button onclick="runTest()">Activate the gamepad - then press me</button>
rakina commented 3 years ago

Thanks @marcoscaceres for writing the test and spotting the getGamepads() bit! I guess that's a problem for iframes but less so for bfcache since the whole page will be non-fully active (and at least in chrome only pages with no openers can be bfcached), but yeah probably good to add a "fully active" check for the iframe case.

Similar to https://github.com/w3c/geolocation-api/issues/78, cc-ing @hiroshige-g and @fergald so that we can have a BFCache-version of the WPT. It's interesting that you mention is not testable via WPTs... how are various device-related APIs usually tested for interoperability then?

marcoscaceres commented 3 years ago

I'm embarrassed, I didn't know what "BFCache" was... I'm reading the docs.

how are various device-related APIs usually tested for interoperability then?

For WTP, manually 😢 sometimes we can get Web Driver to grant special permissions allowing us to bypass the prompts (e.g., we do this for geo).

But for things like Gamepad, we can only get WPT to poke around the edges of the API (i.e., throw random things at the IDL)... It's a very sad, I know. Why we have such a limited number of tests:

https://wpt.live/gamepad/

The rest we do collaboratively on phone calls and in person - like @sagoston sent a couple of Sony Controllers that I use (and cherish!) for testing at home/work 🎮.

reillyeon commented 3 years ago

We have some Chromium-specific test infrastructure which can attach mock gamepads and is useful for validating this type of behavior. For other device APIs we've done some work to make it plausible for tests to be shared between implementations as long as they implement a similar interface for controlling the mocks. So far that hasn't been done for the gamepad tests so all that is in WPT are basic IDL tests and some manual ones which assume the user can plug in a gamepad and press buttons. These are definitely better than nothing and I encourage adding such tests to WPT when fixing bugs like this.

marcoscaceres commented 3 years ago

About sharing testing infra, Gecko has "GamepadServiceTest.idl", which is pref-enabled (I'm actually in the process of rewriting it to be promise based). If Chrome has something similar, and if there is sufficient overlap (i.e., we just need to rename a few things), we could add something like that to the spec.

nondebug commented 3 years ago

Chrome has a test-only interface window.gamepadController with methods connect, dispatchConnected, disconnect, setId, setButtonCount, setButtonData, setAxisCount, setAxisData, and setDualRumbleVibrationActuator. I don't think there's any IDL for the test-only interface but you can see how the methods are called in the tests here:

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/gamepad/

connect(i) -> Update the gamepad buffer to mark the gamepad at index i connected, but don't fire a gamepadconnected event. dispatchConnected(i) -> If the gamepad at index i is connected, dispatch a connection event. disconnect(i) -> Update the gamepad buffer to mark the gamepad at index i disconnected, and fire a gamepaddisconnected event. setId(i, idString) -> Update the ID string for the gamepad at index i to idString. setButtonCount(i, buttonCount) -> Update the gamepad buffer to set the button count for the gamepad at index i to buttonCount. setButtonData(i, buttonIndex, value) -> Update the gamepad buffer for the gamepad at index i to set the value of the button at index buttonIndex to value. The button's pressed value is also updated. setAxisCount(i, axisCount) -> Update the gamepad buffer to set the axis count for the gamepad at index i to axisCount. setAxisData(i, axisIndex, value) -> Update the gamepad buffer for the gamepad at index i to set the value of the axis at index axisIndex to value. setDualRumbleVibrationActuator(i, enabled) -> If enabled is true, updates the gamepad at index i to add a vibrationActuator with type "dual-rumble". If false, updates the gamepad at index i to set the vibrationActuator to null.

I think the Gecko interface would work fine for our tests.

ArthurSonzogni commented 2 years ago

I am reviewing some blink-dev / w3ctag intent for a security meeting. About: https://togithub.com/w3ctag/design-reviews/issues/662 My question was about the interactions in between the Gamepad API input events with the BFCache. Happy to discover @rakina already opened a thread here.

Did you reach a conclusion?

One of my potential concerns was to fire when the page is re-activated, all the events that accumulated when the page was in the BFCache. @rakina suggestion about sending only the difference from the last activation sound like a good idea . Alternatively, disabling bfcache is also a possibility.

marcoscaceres commented 2 years ago

I don't think queueing up the gamepadconnected and gamepaddisconnected events would be a good idea (at least, it adds quite a bit of complexity). At the same time, the events are somewhat critical, so I'm leaning towards busting the BFCache if event handlers/listeners are registered for those.

Thoughts?

reillyeon commented 2 years ago

I think the desired goal is to move towards a world where features which disable BFCache are few and far between.

As a compromise when it comes to introducing additional complexity versus not introducing unexpected developer-visible behavior is that events should not be queued, but coalesced. That is, if a gamepad is connected or disconnected while the page is not fully active (but the page would otherwise have received the event) then that event should fire when the page becomes fully active. On the other hand, if a gamepad is connected and then disconnected or the other way around then the paired events should be dropped since the "end state" when the page becomes fully active again is no change from the state when it became not fully active.

For the input events currently being discussed by the TAG I think the same principle should apply and only events corresponding to the difference between the last observable state and the current observable state be fired.

marcoscaceres commented 2 years ago

Thanks @reillyeon, what you suggest makes sense for connected/disconnected events.

rakina commented 2 years ago

I agree with @reillyeon, and have similar opinion to other specification issues on whether we should drop vs send the latest update e.g. the Permissions and Geolocation APIs now.

I'm now going to revise my TAG Design Principles guidelines update to explicitly mention how existing APIs should be updated to align better with the Priority of Constituencies:

(And more specifically, changing the recommendation from "drop updates" to "save the last state and send it" instead, as that seems to be a common use. Glad that we're having all these spec discussions that makes us rethink our recommendation!)

marcoscaceres commented 2 years ago

The above sound great! Let's continue the general discussion over on https://github.com/w3ctag/design-principles/pull/317.

rakina commented 2 years ago

Revisiting this again... @nondebug, WDYT about specifying "save the last state of gamepad connectivity before navigation away from the document, then send a connected/disconnected event if the state doesn't match on BFCache restore"?

rakina commented 2 years ago

Looking at the current spec, it looks like the events won't be dispatched to BFCached (non-fully-active) documents which might cause confusion.

I think we can modify the spec like this

@ Gamepad people, WDYT? (There's also the TAG Design Principles doc for common patterns in case it's needed)