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

Add persistentDeviceId to PointerEvent #495

Open
wants to merge 17 commits into
base: gh-pages
Choose a base branch
from
Open

Conversation

sahirv
Copy link

@sahirv sahirv commented Jan 11, 2024

This change proposes the introduction of a new attribute to the PointerEvent interface - persistentDeviceId. See: https://github.com/WICG/pointer-event-extensions/blob/main/pointer-event-device-id-explainer.md

This pull request is expected to make the 4th iteration of the PointerEvent specification.


Preview | Diff

This change proposes the introduction of a new attribute to the PointerEvent interface - deviceId. See: https://github.com/WICG/pointer-event-extensions/blob/main/pointer-event-device-id-explainer.md
@patrickhlauke
Copy link
Member

Worth adding, for context, #353

@sahirv
Copy link
Author

sahirv commented Jan 17, 2024

Happy to take any comments on the changes. The idea is based on that of pointer id; -1 is reserved for invalid ids. In Chromium, 1 is reserved for the mouse pointer (0 is unused); but I left the wording to be similar to the pointerId definition. As long as the deviceId is unique for each device, web devs should not have a problem.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@sahirv sahirv changed the title Add deviceId to PointerEvent spec Add deviceProperties.uniqueId to PointerEvent spec Mar 5, 2024
@sahirv
Copy link
Author

sahirv commented Mar 5, 2024

Made changes to introduce deviceProperties rather than having deviceId on PointerEvent. Rough draft- feedback appreciated

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@sahirv sahirv requested a review from flackr March 11, 2024 18:00
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 28, 2024
This change replaces deviceId on the PointerEvent interface with a new interface, deviceProperties. DeviceProperties contains one member, uniqueId, which functionally behaves the same as the outgoing deviceId.

Spec: w3c/pointerevents#495

This change also removes the setting of deviceId when creating a PointerCancel event, as there is no current application for device/unique id for a PointerCancel.

Bug: 330760871
Change-Id: I0f1a9f7d5589f790d94f498a38bfdf55b6f51073
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 29, 2024
This change replaces deviceId on the PointerEvent interface with a new interface, deviceProperties. DeviceProperties contains one member, uniqueId, which functionally behaves the same as the outgoing deviceId.

Spec: w3c/pointerevents#495

This change also removes the setting of deviceId when creating a PointerCancel event, as there is no current application for device/unique id for a PointerCancel.

Bug: 330760871
Change-Id: I0f1a9f7d5589f790d94f498a38bfdf55b6f51073
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 8, 2024
This change replaces deviceId on the PointerEvent interface with a new interface, deviceProperties. DeviceProperties contains one member, uniqueId, which functionally behaves the same as the outgoing deviceId.

Spec: w3c/pointerevents#495

This change also removes the setting of deviceId when creating a PointerCancel event, as there is no current application for device/unique id for a PointerCancel.

Bug: 330760871
Change-Id: I0f1a9f7d5589f790d94f498a38bfdf55b6f51073
@annevk
Copy link
Member

annevk commented May 13, 2024

I guess I don't understand what @flackr means with optional there. These wouldn't be supported by everyone?

@flackr
Copy link
Contributor

flackr commented May 16, 2024

I fail to see the benefit in introducing a tear-off/namespace object here. Why can't this stay flat?

I believe @smaug---- had concerns with growing the PointerEvent structure with a bunch of one-off "optional" attributes, noting the penCustomizationDetails as another one that may be added in the future, and so we reached a consensus on Jan 31st to have @sahirv suggest a representative structure for these special device capabilities. @smaug----, I couldn't find notes covering the concerns you raised in some of the meetings about this, perhaps you could elaborate? I believe @patrickhlauke may have had thoughts as well.

I guess I don't understand what @flackr means with optional there. These wouldn't be supported by everyone?

My understanding is this particular API is, at least presently, for a limited set of devices for which the particular pen you are using reports extra data so that it is uniquely identifiable from other pens used on the same touchscreen (see Limitations of Current Hardware.

Similarly, the penCustomizationDetails api would have limited hardware devices which provides particular pen customization information.

So by "optional" what was meant is likely to be unreported by the majority of hardware. It's of course entirely possible that at some point most new devices will support some of these concepts, and it is certainly also the case that many current devices don't support properties like tangentialPressure, so personally I don't strongly feel that it can't stay flat, however it would also be nice to avoid churn for @sahirv as to where this property belongs.

@smaug----
Copy link
Contributor

I don't think it was just me thinking about having a separate object.
#353 (comment)
In fact, based on https://www.w3.org/2024/01/31-pointerevents-minutes.html#t07 it was @flackr who came up with the idea of separate struct :)

But the concern is to have various somewhat similar IDs in the main event interface. That makes the API harder to understand. And conceptually the new ID isn't about the event but about the device.

@annevk
Copy link
Member

annevk commented May 17, 2024

But then just prefix them with device? Generally in the web platform we avoid introducing 1:1 objects purely for the purposes of namespacing.

@smaug----
Copy link
Contributor

But the point is that it isn't namespacing. The id is about device. It is like element.style.color. That color could live in element, but we have decided to not have it here.

@annevk
Copy link
Member

annevk commented May 17, 2024

I think that's a little different as there are many objects that can return CSSStyleDeclaration. Anyway, I don't care too strongly, but I don't find this compelling.

@smaug----
Copy link
Contributor

I don't feel super strongly either, but there just is the risk that adding more and more device specific properties to the event interface itself will make it eventually rather hard to understand, and using it in cases when relevant devices aren't available may be confusing.

@flackr
Copy link
Contributor

flackr commented May 17, 2024

I don't think it was just me thinking about having a separate object. #353 (comment) In fact, based on https://www.w3.org/2024/01/31-pointerevents-minutes.html#t07 it was @flackr who came up with the idea of separate struct :)

What wasn't quite captured in the minutes was that I suggested a separate struct as a response to the concern which had been raised prior :-). I'm not sure when it was discussed to find the minutes of. I wanted to find something we could agree on.

And conceptually the new ID isn't about the event but about the device.

To clarify, the "device" in this context is the pen, not the touchscreen "device" which itself can sense multiple different pen's each having their own id's (and possibly style in the future).

I do think if we were to go back it could be an interesting separation to consistently put the properties which are about the pointer generally rather than its current location in a separate structure, e.g. pointerType, pointerId. You could also argue that coalesced events and predicted events are separate structures, that are just behind functions instead.

@annevk
Copy link
Member

annevk commented May 17, 2024

One of the issues I have with this type of design is that event.deviceId will always work, whereas event.device.id might result in an exception due to event.device being null. And it's not really clear that this complexity comes with any kind of benefit that isn't also addressed with clear naming. (Which also means it's not exactly 1:1 like I think element.style is.)

@sahirv
Copy link
Author

sahirv commented May 17, 2024

I don't want to force event.deviceProperties.uniqueId through just because we switched to it in Chromium. Ideally everyone would be happy with the solution; as this impacts the future of one of the most popular web APIs. That being said, it would be great if we could all work together towards a consensus ASAP. We have received positive feedback from our web developer partners on both forms of the API.

My humble 2 cents: Could we mandate in the spec that deviceProperties always exists with a default value of an invalid uniqueId? That way we can still differentiate between device-specific and pointer-specific properties like @smaug---- and @flackr were initially concerned about without the additional null check that @annevk mentions.

@annevk
Copy link
Member

annevk commented May 21, 2024

@sahirv it's probably possible to do that, but I'm pretty sure https://dom.spec.whatwg.org/#constructing-events doesn't directly support it today.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 21, 2024
…roperties in PointerEventInit, a=testonly

Automatic update from web-platform-tests
Set null as the default value of deviceProperties in PointerEventInit

Previously, the default for deviceProperties in PointerEventInit was
undefined; although in chromium a value was set using the default
constructor. This CL changes this behaviour such that the default value
for deviceProperties is null. The corresponding WPT is updated as well.

For further context, please see the discussion here:
w3c/pointerevents#495 (comment)

Bug: 330760871
Change-Id: I5becf25f76e8b8c4572a783bfba8c1c4f36f8a74
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5526091
Commit-Queue: Sahir Vellani <[email protected]>
Reviewed-by: Robert Flack <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1298623}

--

wpt-commits: aec353e3b1cae190e1c0485b1406ea522f38e661
wpt-pr: 46174
@smaug----
Copy link
Contributor

We were discussing this today and one approach which gets rid of the id altogether and also handles the example in the current pr without need to have explicit hashtable around is to have basically an empty DeviceProperties interface (or perhaps it should be called something else), but it is guaranteed that the same object is returned from .deviceProperties for the same device. The value would be null if there isn't a relevant device. Now the web site could attach random properties directly to that object, for example event.deviceProperties?.myFrameworkColor = "#FFFFFFFF"; and that property would stay there, since the deviceProperties object is kept alive.
This does mean that UA needs to keep some objects alive (if there are expando properties), but that isn't too different to all the stuff hanging for example from window.navigator.

@ogerchikov
Copy link

@smaug---- Using .deviceProperties to identify a device is a smart idea. But I'm worried that we might need to retrieve more hardware details in future, and that would mean we have to create a new interface for them. WDYT?

@smaug----
Copy link
Contributor

We would just add those properties to the interface.
And the spec should explicitly say that if one adds their own expando properties (using JS), those properties should have JS framework specific prefix. That would hopefully reduce the risk to breaking existing pages when adding new properties to the interface in a spec level.

@ogerchikov
Copy link

@smaug----

We would just add those properties to the interface.

My understanding is that .deviceProperties will only be created for the devices with persistent IDs. It's possible that we'll need to collect other hardware properties from the devices that don't have persistent IDs, and as a result, they won't have .deviceProperties instantiated. Is there something I'm overlooking?

jamienicol pushed a commit to jamienicol/gecko that referenced this pull request May 23, 2024
…roperties in PointerEventInit, a=testonly

Automatic update from web-platform-tests
Set null as the default value of deviceProperties in PointerEventInit

Previously, the default for deviceProperties in PointerEventInit was
undefined; although in chromium a value was set using the default
constructor. This CL changes this behaviour such that the default value
for deviceProperties is null. The corresponding WPT is updated as well.

For further context, please see the discussion here:
w3c/pointerevents#495 (comment)

Bug: 330760871
Change-Id: I5becf25f76e8b8c4572a783bfba8c1c4f36f8a74
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5526091
Commit-Queue: Sahir Vellani <[email protected]>
Reviewed-by: Robert Flack <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1298623}

--

wpt-commits: aec353e3b1cae190e1c0485b1406ea522f38e661
wpt-pr: 46174
@flackr
Copy link
Contributor

flackr commented May 23, 2024

While discussing the concerns, this seemed like a neat way to avoid having a second identifier with slightly different semantics and a convenient way for developers to use it without having to create a map of their own.

My understanding is that .deviceProperties will only be created for the devices with persistent IDs.

@ogerchikov Yes, that's correct.

It's possible that we'll need to collect other hardware properties from the devices that don't have persistent IDs, and as a result, they won't have .deviceProperties instantiated. Is there something I'm overlooking?

If this information is not associated with a uniquely identifiable pointer, then it wouldn't belong in deviceProperties. If it is, then presumably it wouldn't be known until the persistent id was also known, right?

@sahirv
Copy link
Author

sahirv commented May 23, 2024

@annevk how does this proposal look to you?

I've just got one question. I'm wondering if there may be properties that will be added in the future that are hardware specific but not dependent on the presence of a hardware id. I don't have access to the USI spec, but what if certain styluses have the capability to support customization but not supply a hardware id? For example, the pointing device may be identified and communicated with based on the pointer id of that stroke. I hope I'm not inventing a problem where there is none; but it just seems like this has the potential to not serve the initial purpose of this pivot - which was housing all device specific properties.

@annevk
Copy link
Member

annevk commented May 24, 2024

I'm not sure how it changed since I last commented? I suspect you need to figure out what changes to event construction this requires, but it's prolly doable. It still strikes me as quite a bit of complexity for what is essentially expressible through a getter (and multiple thereof in the future).

@sahirv
Copy link
Author

sahirv commented May 29, 2024

While discussing the concerns, this seemed like a neat way to avoid having a second identifier with slightly different semantics and a convenient way for developers to use it without having to create a map of their own.

I discussed this with our partners, and they will still need to create an identifier and add it to deviceProperties. They will also need their own map to keep track of state when the pointer is not in contact with the digitizer. I think the proposed solution is quite neat and simplifies the example in this spec pr; however, I think it is still important to provide an id because in most cases the web developer will need to create one themselves. Ultimately, it is my opinion that the current impl of supplying the id is the simplest, most intuitive and most versatile solution to the problem.

There are some more hardware specific attribute proposals that would be good candidates for being under deviceProperties. Hover Distance is one and the previously mentioned Pen Customizations is another. If it doesn't make sense to add these under deviceProperties, perhaps we should just revert back to PointerEvent.deviceId as @annevk suggests.

@flackr @smaug---- Would love your thoughts :)

@flackr
Copy link
Contributor

flackr commented May 29, 2024

I'm okay with having a property directly on the PointerEvent, but would suggest some better differentiation from pointerId. E.g. maybe if we call it persistentId it will be obvious how/why it is different from the pointerId.

@sahirv
Copy link
Author

sahirv commented May 30, 2024

maybe if we call it persistentId it will be obvious how/why it is different from the pointerId.

How does persistentDeviceId sound? By naming it this way we make it clear that:

  1. This id will persist (for the session) differentiating it from pointerId.
  2. This id is tied to the pointing device; not the event or sequence.

@smaug----
Copy link
Contributor

That is quite reasonable. It is long and perhaps weird enough that one wouldn't likely mix that with pointerId.

@sahirv
Copy link
Author

sahirv commented May 30, 2024

@annevk @flackr @mustaqahmed Are you all good with this suggestion and the overall direction we're taking here?

i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Jun 1, 2024
… 0, a=testonly

Automatic update from web-platform-tests
Set Invalid DeviceProperties.uniqueId to 0

Replace -1 with 0 for the invalid uniqueId as per recent spec consensus.
Spec: w3c/pointerevents#495

Bug: 330760871
Change-Id: I063515535ee0510c89942f16098c6211bafb480c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5463741
Reviewed-by: Robert Flack <[email protected]>
Reviewed-by: Mustaq Ahmed <[email protected]>
Commit-Queue: Sahir Vellani <[email protected]>
Reviewed-by: Olga Gerchikov <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1290720}

--

wpt-commits: a2bf027ec76e7f183c4f7e3fdcd62d5a0e818598
wpt-pr: 45765
@sahirv
Copy link
Author

sahirv commented Jun 10, 2024

@smaug---- @flackr @mustaqahmed I have updated this PR to introduce persistentDeviceId. Please take a look and leave your feedback. Thank you :)

@sahirv sahirv changed the title Add deviceProperties.uniqueId to PointerEvent spec Add persistentDeviceId to PointerEvent Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants