w3c / gamepad

Gamepad
https://w3c.github.io/gamepad/
Other
141 stars 48 forks source link

GamepadEvent constructor is missing the type argument #35

Closed foolip closed 6 years ago

foolip commented 8 years ago

https://w3c.github.io/gamepad/#gamepadevent-interface

[Constructor(GamepadEventInit eventInitDict)]
interface GamepadEvent : Event {
    readonly        attribute Gamepad gamepad;
};

A type argument needs to precede the dictionary. [SameObject] would be nice too, so:

[Constructor(DOMString type, GamepadEventInit eventInitDict)]
interface GamepadEvent : Event {
    [SameObject] readonly attribute Gamepad gamepad;
};
cdumez commented 7 years ago

Yes, I noticed it as well when updating WebKit. Gecko / Blink and WebKit all take a type as first constructor parameter.

cdumez commented 7 years ago

eventInitDict should also be marked as optional. This matches both Firefox and Chrome's behavior. I will align WebKit accordingly.

cdumez commented 7 years ago

Per Web IDL: """ If the type of an argument is a dictionary type (or a union type that has a dictionary as one of its flattened member types), and that dictionary type and its ancestors have no required members, and the argument is either the final argument or is followed only by optional arguments, then the argument must be specified as optional. Such arguments are always considered to have a default value of an empty dictionary, unless otherwise specified. """

foolip commented 7 years ago

@toji, can you take a look at this?

nondebug commented 7 years ago

The PR looks good to me, but I think the GamepadEventInit param need not be optional unless we also change the gamepad member to optional.

cdumez commented 7 years ago

Good point about making the gamepad dictionary member optional.

That said, Firefox and Chrome have the GamepadEventInit parameter as optional. The following WPT relies on it being optional too: http://w3c-test.org/dom/events/Event-timestamp-high-resolution.html

I made it optional yesterday in WebKit to align with Firefox and Chrome.

As such, I would suggest updating the spec to match browsers?

foolip commented 7 years ago

Ah, another of these cases where the dictionary member being optional makes the corresponding interface attribute nullable be necessity. There's a TODO around this in Blink pointing to https://crbug.com/647693 and this very issue.

Making it nullable+optional would make sense given that it already effectively is all implementations it seems.

cdumez commented 7 years ago

Yes, I confirmed it is nullable in WebKit as well:

[
    Constructor(DOMString type, optional GamepadEventInit eventInitDict),
] interface GamepadEvent : Event {
    readonly attribute Gamepad? gamepad;
};

dictionary GamepadEventInit : EventInit {
    // The specification says this member should be required and non-nullable.
    // However, this does not match the behavior of Chrome or Firefox.
    Gamepad? gamepad = null;
};
ddorwin commented 5 years ago

@foolip Should GamepadEventInit be made optional too? This is still unresolved (https://github.com/web-platform-tests/wpt/issues/14357). @ylafon, please reopen to address this issue.

foolip commented 5 years ago

@ddorwin, it should be optional if there are no required members, and otherwise non-optional.

foolip commented 5 years ago

(This is just generally how dictionary arguments are handled made optional or not depending on dictionary members, not something special about this case.)

marcoscaceres commented 5 years ago

And default values in the dictionary, if possible 😊 general advice too. Feel free to tag me for review.