w3c / deviceorientation

W3C Device Orientation spec
https://www.w3.org/TR/orientation-event/
Other
49 stars 32 forks source link

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

Open saschanaz opened 3 years ago

saschanaz commented 3 years ago

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 commented 3 years ago

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

Am I right here? https://github.com/heycam/webidl/issues/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 commented 3 years ago

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 commented 3 years ago

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 commented 8 months ago

It's been a while, but I think Blink's behavior here is correct (see also https://github.com/w3c/deviceorientation/issues/54#issuecomment-425508807).

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 commented 8 months ago

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

annevk commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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.

reillyeon commented 5 months ago

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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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

reillyeon commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.