w3c / geolocation

W3C Geolocation API
https://www.w3.org/TR/geolocation/
81 stars 56 forks source link

`GeolocationCoordinates.toJSON()` - Should the spec cover how to handle `NaN` headings? #171

Closed ezzatron closed 1 month ago

ezzatron commented 3 months ago

I noticed the new GeolocationCoordinates.toJSON() method was added and immediately wondered how it would handle this edge case where heading can have a NaN value: https://github.com/w3c/geolocation/blob/b0ca499b233380b46ec7c4a2ad4beba4a8ec4521/index.html#L1367-L1373

NaN can't be represented in JSON (JSON.stringify(NaN) gives "null"), and null already has a special meaning for the heading property (implementation cannot provide heading information), so I think the conversion to JSON will be lossy unless heading gets special treatment.

It's probably also the case that the current implementation of this (at least in Chromium) will return an object from GeolocationCoordinates.toJSON() that contains a NaN value for heading. I'm not sure how much of an issue that is, but it feels weird to have a method named toJSON() return a value that can't actually be represented in JSON.

I've been working on a testing fake of the Geolocation API, and this test shows how I'm speculating the API will behave currently in this edge case: https://github.com/ezzatron/fake-geolocation/blob/8b7a5f3fc24c9d4d4e2d6a9406a5f0ddc0978724/test/vitest/geolocation-coordinates.spec.ts#L80-L101

Relates to #145, #147

reillyeon commented 2 months ago

The name toJSON() is definitely a misnomer in that it only returns a "plain JavaScript object" which could be serialized to JSON (except for the NaN issue). I don't immediately see a solution to that problem. It's going to come up in the automation work that @marcoscaceres is drafting as well since we can only provide mock data as JSON and so without the ability to represent NaN we won't be able to configure the browser to provide a stationary location with an undefined (rather than unavailable) heading.

While unfortunate, I'm not sure how much this matters in practice. If we do find this limiting we could sentinel value such as the string "NaN" to represent this state. Helpfully, coercion of that string to a number actually produces NaN.

marcoscaceres commented 2 months ago

We should check if NaN is ever returned by any implementation.

Also:

JSON.stringify({a: NaN})

Gives back null for "a".

The semantics of NaN (only for heading) are also a bit wonky, IMO. Could one ever determine if "the hosting device is stationary"? Like, when I check on my Mac Pro, heading is always null in every browser.

Maybe we can drop the NaN requirement from the spec entirely and just always return null?

marcoscaceres commented 2 months ago

Checked Safari on iOS 18 on my iPhone (with the device stationary).... it's null.

@reillyeon, can you check Chrome?

ezzatron commented 2 months ago

Maybe we can drop the NaN requirement from the spec entirely and just always return null?

The way MDN reads, heading and speed are either:

I don't know for sure that that's always true (maybe some platform can still provide a non-null heading at 0 speed). But assuming it is true, then I think you could safely infer the NaN case, right?:

if (position.speed == null) {
  // platform doesn't support speed/heading
} else if (position.heading == null /* alternatively: position.speed === 0 */) {
  // speed/heading supported, stationary (former NaN heading case)
} else {
  // speed/heading supported, not stationary
}
marcoscaceres commented 2 months ago

Right, but that’s kinda my point: these values shouldn’t be used to feature detect anything.

reillyeon commented 2 months ago

Chrome never generates a NaN value. I would consider that a bug given the specification but auditing the various platform location providers none of them appear to distinguish between a heading being unavailable and it being undefined because the speed is 0.

I'm okay with removing comment around NaN and just saying that implementations may set heading to null when it is unavailable or undefined.

marcoscaceres commented 2 months ago

Yeah, I think that's a good change (I'll audit the WebKit codebase, but I'm sure it's the same... i.e., only ever null).

@saschanaz, thoughts on the Gecko side or anything we might have missed?

saschanaz commented 2 months ago

Per my quick look Gecko filters out NaN and makes it null, so disallowing NaN makes sense to me.

marcoscaceres commented 1 month ago

@reillyeon, do you want to take this one? 🙏

anssiko commented 1 month ago

If I'm not mistaken, NaN is not a valid value of type https://webidl.spec.whatwg.org/#idl-double. The proposal to disallow NaN seems reasonable as it would fix both the toJSON() issue and make the following IDL compatible with the prose:

readonly attribute double? heading;

Should there be good use cases or web compat reasons for allowing NaN, then https://webidl.spec.whatwg.org/#idl-unrestricted-double would be the correct type for heading I believe.