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

DeviceMotionEvent atttributes can't be null per current IDL #91

Open
saschanaz opened this issue Nov 15, 2020 · 18 comments
Open

DeviceMotionEvent atttributes can't be null per current IDL #91

saschanaz opened this issue Nov 15, 2020 · 18 comments

Comments

@saschanaz
Copy link
Member

#55 made the init dictionary members non-nullable, and thus the acceleration/rotationRate attributes cannot be null.

It's also what Gecko implements:

> new DeviceMotionEvent("type").acceleration

DeviceAcceleration { x: null, y: null, z: null }
@saschanaz
Copy link
Member Author

cc @annevk and @bzbarsky (in case you are still interested).

Am I right here? whatwg/webidl#793 seems to be saying otherwise.

Also, could we make the syntax more intuitive:

dictionary Foo {
  DeviceAccelerationInit init = {}; // where it can't be null, just as arguments do
};

dictionary Bar {
  DeviceAccelerationInit? init = null; // where it really can be null
};

@bzbarsky
Copy link

Since then, I believe WebIDL has been changed to allow dictionary-typed members of dictionaries to be nullable. See the note at https://heycam.github.io/webidl/#idl-nullable-type about dictionary types.

@saschanaz
Copy link
Member Author

Since then, I believe WebIDL has been changed to allow dictionary-typed members of dictionaries to be nullable. See the note at https://heycam.github.io/webidl/#idl-nullable-type about dictionary types.

Thanks! So it seems that https://w3c.github.io/media-capabilities/#mediaconfiguration in the thread can be:

dictionary MediaConfiguration {
  VideoConfiguration? video = null;
  AudioConfiguration? audio = null;
};

... and that my take here is correct.

@rakuco
Copy link
Member

rakuco commented Oct 31, 2023

It's been a while, but I think Blink's behavior here is correct (see also #54 (comment)).

Gecko's behavior of making new DeviceMotionEvent("type").acceleration return a DeviceAcceleration instance whose attributes are null instead of returning null occurs because its DeviceMotionEventInit definition does not conform to the spec (it declares acceleration, accelerationIncludingGravity and rotationRate with default values).

@annevk
Copy link
Member

annevk commented Oct 31, 2023

Presumably Gecko wanted to align with #55 as per OP, right? I guess that change was reverted at a later date?

@annevk
Copy link
Member

annevk commented Oct 31, 2023

Actually no, the specification still looks broken. https://w3c.github.io/deviceorientation/#devicemotion has the event and the event's dictionary out of sync with regards to member types.

@rakuco
Copy link
Member

rakuco commented Oct 31, 2023

The current version of the spec looks fine to me -- even if #55 was reverted, passing {} to DeviceMotionEvent's constructor would still cause it to receive an IDL dictionary whose acceleration, accelerationIncludingGravity and rotationRate would be undefined (in which case I'd expect the corresponding attributes in DeviceMotionEvent to return null).

@rakuco
Copy link
Member

rakuco commented Oct 31, 2023

The spec could also change and convert undefined dictionary members into non-null DeviceMotionEventAcceleration and DeviceMotionEventRotation objects with null attributes, but that'd require changing the IDL in WebKit, Gecko and Blink. OTOH, WebKit doesn't implement the DeviceMotionEvent constructor and the Gecko change would not have any user-visible effects.

Reverting #55 does not help with either approach, but anyway there's the question of whether to make DeviceMotionEvent's attributes non-nullable or to make DeviceMotionEventInit's attributes nullable and defaulting to null.

@saschanaz
Copy link
Member Author

Generally, not passing an init dictionary should be same as passing {}, as you can see in optional DeviceMotionEventInit eventInitDict = {}. I would expect the rule to be recursively applied here and thus new DeviceMotion("type") should behave same as new DeviceMotion("type", {}) and new DeviceMotion("type", { accelaration: {}, accelerationIncludingGravity: {}, rotationRate: {} }), given that each member is also an init dictionary.

rakuco added a commit that referenced this issue Nov 10, 2023
The idea is to make this section less hand-wavy and easier to change in the
future by defining algorithms and steps more clearly while not making any
user-visible changes.

Readability:
- Rename the "Description" section to "API"
- Start each "API" subsection with an IDL block rather than prose.
- Move the `<dfn>`s for attributes and methods to their normative
  descriptions rather than the IDL blocks.
- Use the wording currently recommended by the Web IDL spec to define
  attributes and methods.
- Add `<div algorithm>` around the existing requestPermission() algorithms
  to highlight variables.
- Explain what each Device{Motion,Orientation}Event attribute represents in
  each definition instead of doing so separately later in each subsection.
  Where possible, stop saying what each attribute must be initialized to on
  creation, as DOM's event construction steps guarantee that they will be
  initialized by their respective `*EventInit` dictionaries, where the
  default values are set. This is not the case for a few DeviceMotionEvent
  attributes due to issue #91.

More normative definitions:
- Device Orientation
  * Add a `<dfn>` for "significant change in orientation". Although each
    implementation still gets to decide its meaning, we can now reference
    the term from other parts of the spec. The same requirements and
    suggestions remain.
  * Define a "fire an orientation event" algorithm that normatively
    specifies the process of obtaining device's absolute or relative
    orientation data in the format used by the spec, limiting its precision
    and firing a DeviceOrientationEvent that is constructed and has its
    attributes initialized as described by the DOM spec.
  * Attempt to define what the event target of the event is: this
    specification is an outlier in how the event handlers are part of the
    Window interface -- it is difficult to indicate which Window object the
    event is fired at. For now, use "a navigable's active window". This is
    not airtight since we do not specify which navigable this is referring
    to (it's basically "all navigables"), but I could not think of a better
    way to specify this.
  * Reference the new `<dfn>`s from the ondeviceorientationabsolute
    subsection as well.
- Device Motion
  * Add separate subsections for each interface. The
    DeviceMotionEvent{Acceleration,RotationRate} interfaces have proper
    "associated data" (or internal slots) that are referenced from the
    attribute getter descriptions. The two interfaces now have a short
    description of what they represent as well.
  * Define the process of firing a DeviceMotionEvent as an algorithm with a
    proper set of steps instead of just saying implementations must fire the
    "devicemotion" event. The steps are also responsible for limiting the
    precision of the data and firing a DeviceMotionEvent that is constructed
    and has its attributes initialized as described by the DOM spec.
  * As with the Device Orientation part, an attempt has been made to define
    what the event target of the event is, as it is difficult to indicate
    which Window object the event is fired at. For now, use "a navigable's
    active window". This is not airtight since we do not specify which
    navigable this is referring to (it's basically "all navigables"), but I
    could not think of a better way to specify this.
rakuco added a commit that referenced this issue Nov 13, 2023
The idea is to make this section less hand-wavy and easier to change in the
future by defining algorithms and steps more clearly while not making any
user-visible changes.

Readability:
- Rename the "Description" section to "API"
- Start each "API" subsection with an IDL block rather than prose.
- Move the `<dfn>`s for attributes and methods to their normative
  descriptions rather than the IDL blocks.
- Use the wording currently recommended by the Web IDL spec to define
  attributes and methods.
- Add `<div algorithm>` around the existing requestPermission() algorithms
  to highlight variables.
- Explain what each Device{Motion,Orientation}Event attribute represents in
  each definition instead of doing so separately later in each subsection.
  Where possible, stop saying what each attribute must be initialized to on
  creation, as DOM's event construction steps guarantee that they will be
  initialized by their respective `*EventInit` dictionaries, where the
  default values are set. This is not the case for a few DeviceMotionEvent
  attributes due to issue #91.

More normative definitions:
- Device Orientation
  * Add a `<dfn>` for "significant change in orientation". Although each
    implementation still gets to decide its meaning, we can now reference
    the term from other parts of the spec. The same requirements and
    suggestions remain.
  * Define a "fire an orientation event" algorithm that normatively
    specifies the process of obtaining device's absolute or relative
    orientation data in the format used by the spec, limiting its precision
    and firing a DeviceOrientationEvent that is constructed and has its
    attributes initialized as described by the DOM spec.
  * Attempt to define what the event target of the event is: this
    specification is an outlier in how the event handlers are part of the
    Window interface -- it is difficult to indicate which Window object the
    event is fired at. For now, use "a navigable's active window". This is
    not airtight since we do not specify which navigable this is referring
    to (it's basically "all navigables"), but I could not think of a better
    way to specify this.
  * Reference the new `<dfn>`s from the ondeviceorientationabsolute
    subsection as well.
- Device Motion
  * Add separate subsections for each interface. The
    DeviceMotionEvent{Acceleration,RotationRate} interfaces have proper
    "associated data" (or internal slots) that are referenced from the
    attribute getter descriptions. The two interfaces now have a short
    description of what they represent as well.
  * Define the process of firing a DeviceMotionEvent as an algorithm with a
    proper set of steps instead of just saying implementations must fire the
    "devicemotion" event. The steps are also responsible for limiting the
    precision of the data and firing a DeviceMotionEvent that is constructed
    and has its attributes initialized as described by the DOM spec.
  * As with the Device Orientation part, an attempt has been made to define
    what the event target of the event is, as it is difficult to indicate
    which Window object the event is fired at. For now, use "a navigable's
    active window". This is not airtight since we do not specify which
    navigable this is referring to (it's basically "all navigables"), but I
    could not think of a better way to specify this.
@reillyeon
Copy link
Member

reillyeon commented Feb 12, 2024

I propose reverting #55 and explicit defining the default values for acceleration, accelerationIncludingGravity and rotationRate to be null.

Assuming that is valid from a WebIDL perspective, it matches the intent of the specification because an event with these properties null is meaningful.

@anssiko
Copy link
Member

anssiko commented Feb 25, 2024

@reillyeon do we still need to consult Web IDL experts for this proposed spec change?

Discussed in https://www.w3.org/2024/02/12-dap-minutes.html#t07

reillyeon added a commit that referenced this issue Feb 28, 2024
This change reverts #55 and defines the default values of the
DeviceMotionEventInit as null as I believe was the original intent of
the definition before that change.

Null DeviceMotionEvent attributes have semantic meaning (they declare
that the host does not provide the given sensor) and so script should
be able to initialize an event with its attributes set to null.

Fixed #91.
@reillyeon
Copy link
Member

reillyeon commented Feb 28, 2024

While trying to test #141 in Chromium I discovered that Blink rejects the proposed WebIDL with an error that "[n]ullable dictionary type is forbidden as a dictionary member type." Double-checking the WebIDL spec the note at the end of https://webidl.spec.whatwg.org/#idl-nullable-type explicitly says that "[a]lthough dictionary types can in general be nullable, they cannot when used as the type of an operation argument or a dictionary member."

It looks like @bzbarsky's comment above was either incorrect or is now out of date.

If that is true then it seems like this is insolvable given the current WebIDL specification. The question is whether it is worth pursuing a workaround (or WebIDL spec change) or accepting that it is simply impossible to construct a DeviceMotionEvent from script with these attributes set to null. I am tempted to go with the latter. It doesn't seem worth the effort to resolve this issue.

@annevk
Copy link
Member

annevk commented Feb 29, 2024

Why would you default the dictionaries to null and not {}?

@reillyeon
Copy link
Member

Because creating a DeviceMotionEvent like this,

{ acceleration: null, accelerationIncludingGravity: null, rotationRate: null }

is much more likely what the developer wants than creating one like this,

{
  acceleration: { x: null, y: null, z: null },
  accelerationIncludingGravity: { x: null, y: null, z: null },
  rotationRate: { alpha: null, beta: null, gamma: null }
}

The first option is equivalent to the "null event" that implementations generate when sensors aren't supported on the host while the second option is what an implementation would generate if the host had sensors which provided none of the expected axes, which is nonsense.

@annevk
Copy link
Member

annevk commented Mar 1, 2024

Oh I see. I don't understand why it was decided to use nested data structures for this. They should just have been flattened (accelerationX, etc.), but that's too late now.

@saschanaz
Copy link
Member Author

But how could flattening cover non-support case? Still each member would be null which would still be more about no-axis-data case IMO.

@annevk
Copy link
Member

annevk commented Mar 3, 2024

The difference doesn't really come up in that case. Either you have the field or you don't. But I also don't really see the big problem with initializing the dictionary members to null either. That seems just as valid as initializing the accessor to null, except with possibly fewer "undefined is not an object" exceptions for consumers. (The main problem is probably that it doesn't match existing behavior?)

@reillyeon
Copy link
Member

Agreed, a flat structure would only have one way to represent "no accelerometer" while the current design has two. I'm going to amend my previous statement however because I was wrong about what implementations do in the case where a sensor is not available. The specification steps would generate an event as I described but reading the Chromium and Gecko code I believe they would generate default-initialized DeviceMotionEventAcceleration and DeviceMotionEventRotationRate objects instead.

@rakuco, can you double-check me on that and if so the specification needs to be updated. This issue could then be closed because there won't be any use for an event initialized this way.

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

Successfully merging a pull request may close this issue.

6 participants