search

immersive-web / webxr-polyfill

Use the WebXR Device API today, providing fallbacks to native WebVR 1.1 and Cardboard
Apache License 2.0
381 stars 84 forks source link

Proper Event class #82

Closed takahirox closed 4 years ago

takahirox commented 5 years ago

Background

The polyfill dispatches an event with an object like

this.dispatchEvent('selectstart', {
  frame: this[PRIVATE].frame,
  inputSource: evt.inputSource
});

https://github.com/immersive-web/webxr-polyfill/blob/master/src/api/XRSession.js#L104-L107

But if I understand correctly the second argument should be XR*Event inheriting Event.

https://www.w3.org/TR/webxr/#events

So the code above should be like this

class XRInputSourceEvent extends Event {
  constructor(type, initDict) {
    super(type, initDict);
    this.frame = initDict.frame;
    this.inputSource = initDict.inputSource;
  }
}

...

this.dispatchEvent('selectstart', new XRInputSourceEvent('selectstart', {
  frame: this[PRIVATE].frame,
  inputSource: evt.inputSource
});

Problem

Because the polyfill currently dispatches an event with an object, the user code relying on Event API doesn't work. For example Three.js has a code using Event.type.

    controllers[ i ].dispatchEvent( { type: event.type } );

https://github.com/mrdoob/three.js/blob/r108/src/renderers/webvr/WebXRManager.js#L80

Question

Is there any special reasons why the polyfill doesn't define XR*Event yet? If it's because just lack of resources we'd help.

jsantell commented 5 years ago

Some thoughts:

device.addEventListener('vrdevicechange', e => {
  const vrDevice = e.detail ? e.detail.vrDevice : e.vrDevice;
 // ...
});
takahirox commented 5 years ago

Events have not been updated for the recent WebXR-2019 changes. Sounds good to me for matching the spec!

Sounds good to me, too!

CustomEvents require data on the { detail } property of event init. Not sure if the custom event solves this to match the spec properties. In the webvr polyfill, events had to be polyfill aware due to this limitation, e.g.:

Ideally user code shouldn't need to be aware of the polyfill so I prefer defining XR*Event in the polyfill as I wrote the above rather than CustomEvents.

Is it correct to say that developers could instantiate their own XRSessionEvent? Definitely should match the spec in that case, but why would a dev be pushing their own XR events?

Who are "developers" in this context? WebXR polyfill developers? Or WebXR application developers? (Sorry my bad English!) If the latter, they should be able to instantiate (according to the spec, if I understand correctly) but I don't think they do in general.

Note that the three.js EventDispatcher API is different than dispatching an Event on an EventTarget -- I'm not sure how that is related

Yes, I knew that. Sorry if the Three.js core snipped code above is confusing. Just I wanted you to know is Three.js has some codes relying on correct XR*Event (more precisely Event inherited by them) API.

jsantell commented 5 years ago

Events have not been updated for the recent WebXR-2019 changes. Sounds good to me for matching the spec!

Sounds good to me, too!

CustomEvents require data on the { detail } property of event init. Not sure if the custom event solves this to match the spec properties. In the webvr polyfill, events had to be polyfill aware due to this limitation, e.g.:

Ideally user code shouldn't need to aware the polyfill so I prefer defining XR*Event in the polyfill as I wrote the above rather than CustomEvents.

Agreed -- this is the only place I'm aware of where content developers need to do anything polyfill-specific. Unfortunately, this seems to be a restriction on any Event subclasses. In Firefox:

class MyEvent extends Event {}
const target = new EventTarget();

const e1 = new MyEvent('foo', { bar: 1 });
const e2 = new MyEvent('foo', { detail: { bar: 1 }});

target.dispatchEvent(e1);
target.dispatchEvent(e2);

The first event (e1) will have { detail: null } and no mention of bar; the second argument only allows the known properties to be applied (detail, cancelable, etc.) -- although, applying the properties after (or before, seems to work) creation, as long as you're not overriding known props, like target or type. Testing now, something like this seems to be working in Firefox, but this area of overriding events is a bit hairy, cross-platform wise. Something to give another look and test.

All that is to say, I think some of my concerns are outdated, and it looks like your solution in the OP works great, providing: 1) real Event subclassing, 2) no { detail } leaky abstraction, 3) clean!

EventTarget seems extensible on Safari now. If there are any modern browsers that fail this check, we should note which features are necessary so that they can be polyfilled -- model-viewer has a good polyfill document, specifying which features are used and needed, so that consumers can provide the polyfills of their choice.

Is it correct to say that developers could instantiate their own XRSessionEvent? Definitely should match the spec in that case, but why would a dev be pushing their own XR events?

Who are "developers" in this context? WebXR polyfill developers? Or WebXR application developers? (Sorry my bad English!) If the latter, they should be able to instantiate (according to the spec, if I understand correctly) but I don't think they do in general.

"Developers" in this case (it's always ambiguous when working on developer tools :smile:) I mean folks creating XR content -- ya it looks like they may do it, but unclear of the benefit. Maybe for more extensibility as well? Either way, I think your solution achieves this.

Note that the three.js EventDispatcher API is different than dispatching an Event on an EventTarget -- I'm not sure how that is related

Yes, I knew that. Sorry if the Three.js core snipped code above is confusing. Just I wanted you to know is Three.js has some codes relying on correct XR*Event (more precisely Event inherited by them) API.

It looks like the polyfill-only-caveat (checking e.detail) isn't necessary given your solution, so three.js's event handling shouldn't have to change. Great :smiley:

takahirox commented 4 years ago

Thanks for the comment.

My idea seems to sound good. I want to go forward. I first want to confirm if it works correctly, so I'm thinking to make XRInputSourceEvent PR before implementing all of the events. After confirming, we can add others later.

It seems one class definition per one JS file. So should I create XRInputSourceEvent.js under src/api rather than defining it in XRInputSource.js?

jsantell commented 4 years ago

My idea seems to sound good. I want to go forward. I first want to confirm if it works correctly, so I'm thinking to make XRInputSourceEvent PR before implementing all of the events. After confirming, we can add others later.

Sounds great! Let me know if I can help

It seems one class definition per one JS file. So should I create XRInputSourceEvent.js under src/api rather than defining it in XRInputSource.js?

They could also live under their corresponding class e.g. export class XRSessionEvent {} in XRSession.js if it'd make sense, or maybe just events.js that contains all the XR* event classes. Or maybe just stick with class name for obviousness. Not something that has been investigated in awhile, so I think organization and naming is open as well!

takahirox commented 4 years ago

They could also live under their corresponding class e.g. export class XRSessionEvent {} in XRSession.js if it'd make sense,

~Coming to think it'd be simple. I'll do that in my PR.~ I changed my mind at last minute and created XRInputSourceEvent.js in the PR. But it isn't strong opinion.

jsantell commented 4 years ago

Fixed via #83. I'm going to open up two more issues for the other event classes so we can track it. Thanks!