893785 - Gamepad API - Combat demo shows wrong number of elements

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Ada [:adalucinet]]

Attachment: Image showing this issue

Reproducible on latest Aurora 24.0a2 (BuildID: 20130715004002): Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130715 Firefox/24.0; 
Reproducible on latest Nightly 25.0a1 (BuildID: 20130715030202): Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:25.0) Gecko/20130715 Firefox/25.0

STR:
1. Plug in a controller.
2. Launch Firefox.
3. Navigate to http://people.mozilla.com/~tmielczarek/combat/.
4. Open a new window (Ctrl+N) or a private browsing window (Ctrl+Shift+P) and repeat step 3.
5. Repeat step 4 a couple of times.

Expected results:
The number of tanks is the same in all windows. 

Actual results: 
The number of tanks is increased.

Notes:
1. Tested with a Xbox 360 controller.
2. If I open a new tab and repeat step 3, this issue is not reproducible; if the tab is dragged out and refreshed, this issue is reproduced.

Comment 1

[(not currently active) Ted Mielczarek]

There's clearly some bug here with firing gamepadconnected events. I can reproduce this on Linux as well.

The reason this doesn't work if you use a new tab instead of a new window is because we don't send events to non-visible pages, so background tabs don't get them.

I can also reproduce this using the simpler testcase:
http://people.mozilla.com/~tmielczarek/gamepadtest.html

Open that in a window and leave it visible, then open a new window with that same page and press a button on the gamepad. If you repeatedly reload the second page and press a gamepad button, the first page will receive a gamepadconnected event every time.

Comment 2

[(not currently active) Ted Mielczarek]

Apparently it's just the events being sent erroneously, because navigator.getGamepads().length == 1 on the first page, no matter how many of the bogus events it gets.

Comment 3

[Ada [:adalucinet]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20130716 Firefox/25.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20130716 Firefox/24.0
Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20130721 Firefox/24.0
Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20130721 Firefox/25.0

Also reproducible on Ubuntu and Mac OS X; changing the platform to all.

Comment 4

[Ada [:adalucinet]]

Sorry for my typo from above. The correct User Agents are:

Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20130722 Firefox/24.0
Mozilla/5.0 (X11; Linux i686; rv:25.0) Gecko/20130718 Firefox/25.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20130720 Firefox/24.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20130720 Firefox/25.0

Comment 5

[bhavana bajaj [:bajaj]]

Can QA help identify if this is a Fx24 regression ? Or is the same bug hit in previous firefox versions?

Comment 6

[(not currently active) Ted Mielczarek]

The Gamepad API did not ship in Firefox prior to Firefox 24, so this is not a regression, it's a bug in a new feature.

Additionally, the API will be preffed off in release, so it's not the end of the world. I haven't had time to look at this yet (was on vacation), but it's not likely to be a huge bug, so it's possible I can get a fix landed and backport it to Aurora soon.

Comment 7

[bhavana bajaj [:bajaj]]

(In reply to Ted Mielczarek [:ted.mielczarek] (post-vacation backlog) from comment #6)
> The Gamepad API did not ship in Firefox prior to Firefox 24, so this is not
> a regression, it's a bug in a new feature.
> 
> Additionally, the API will be preffed off in release, so it's not the end of
> the world. I haven't had time to look at this yet (was on vacation), but
> it's not likely to be a huge bug, so it's possible I can get a fix landed
> and backport it to Aurora soon.

Thanks TEed ! Not tracking given your comment but a low risk uplift will be considered based on where we are in the cycle.

Comment 8

[(not currently active) Ted Mielczarek]

Attachment: don't fire gamepadconnected events for windows that have already received them

Okay, so this is a stupid bug caused by me using NewConnectionEvent when button/axis move events are fired. That function sends connection events to *all* listeners, which is clearly not what we want. Instead I changed them to just use FireConnectionEvent, which sends a single connection event.

There are a few unrelated cleanups in NewConnectionEvent, I thought I needed to fix something there so I cleaned that up slightly while I was there, but I didn't actually need to.

I wrote the test first, verified that it failed without this patch, and passed with the patch.

Comment 9

[(not currently active) Ted Mielczarek]

I also verified that the issue reported in comment 0 doesn't appear with this patch.

Comment 10

[(not currently active) Ted Mielczarek]

smaug asked for clarification on IRC, so here's some more info:

The New*Event methods send an event to all windows that are listening for gamepad events and are also in a visible state. The Fire*Event methods send an event to a specific window. This code was previously using NewConnectionEvent when it wanted to send a gamepadconnected event to a window that hadn't previously seen the gamepad in question. This is just wrong, it should have been using FireConnectionEvent to send an event to just that specific window.

If you think the method names are confusing in retrospect, I'm open to renaming them (although the New*Event methods are public API, but not called from so many places that it'd be hard to change them).

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 783916 [details] [diff] [review]
don't fire gamepadconnected events for windows that have already received them

Could you add that comment to .h where those methods are declared.

Comment 12

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/938037f239b1

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/938037f239b1

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 783916 [details] [diff] [review]
don't fire gamepadconnected events for windows that have already received them

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 604039, this is a bug in the initial implementation of this new feature (Gamepad API).
User impact if declined: Extraneous gamepadconnected events will be fired to web cotnent.
Testing completed (on m-c, etc.): Patch includes an automated test, manual testing completed with self builds.
Risk to taking this patch (and alternatives if risky): Gamepad API is preffed off by default, so this should have very little impact, but it would be nice to not ship this feature (even if preffed off) with this silly bug.
String or IDL/UUID changes made by this patch: None

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d759cc9de1df