Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
rakina opened this issue May 19, 2021 · 18 comments
Open

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

rakina opened this issue May 19, 2021 · 18 comments

Comments

@rakina
Copy link
Member

rakina commented May 19, 2021

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
Copy link
Member

@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
Copy link
Member Author

rakina commented May 24, 2021

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
Copy link
Member

marcoscaceres commented May 24, 2021

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
Copy link
Member

annevk commented May 25, 2021

"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:

  • In many cases there isn't a single Window object within a tab, how should that work?
  • What if the user changes focus to another tab or another application?

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

@marcoscaceres
Copy link
Member

marcoscaceres commented Jun 11, 2021

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
Copy link
Member Author

rakina commented Jun 11, 2021

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 w3c/geolocation#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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Member

ArthurSonzogni commented Aug 9, 2021

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

@rakina
Copy link
Member Author

rakina commented Aug 11, 2021

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:

  • If we leave the responsibility to the developer to proactively update their sites to handle bfcache, end users might end up potentially suffering inexplicable errors on sites that don't explicitly support BFCache.
  • Specifying ways to make existing usage of existing APIs to more or less seamlessly work with BFCache seems to be the right choice, as the burden should be on browsers to make sure things will work (either by expending effort to implement support for the features, or disabling BFCache when that's not possible).

(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
Copy link
Member

The above sound great! Let's continue the general discussion over on w3ctag/design-principles#317.

@rakina
Copy link
Member Author

rakina commented Apr 27, 2022

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
Copy link
Member Author

rakina commented Jun 16, 2022

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

  • Add state tracking that saves whether a gamepad is currently connected / disconnected as currentState.
  • Save the currentState just before navigating to stateBeforeNavigation (either by listening to changes to the "fully active" state adding to "unloading document cleanup steps")
  • When the document becomes "fully active" again, check if the currentState is different from stateBeforeNavigation, and dispatch the appropriate event.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants