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

Remove reason from Events, add session #191

Closed
toji opened this issue Feb 23, 2017 · 5 comments
Closed

Remove reason from Events, add session #191

toji opened this issue Feb 23, 2017 · 5 comments
Milestone

Comments

@toji
Copy link
Member

toji commented Feb 23, 2017

The idea of stating a reason with VR events was initially brought up to allow things like the activate event to differentiate between activating because a proximity sensor was set off or a page was navigated or a phone was placed in a harness, etc. Upon reflection I question the utility of this, though. We're already seeing that in the 2.0 spec we need separate events for navigate and activate, and it's not clear that developers will benefit from being able to differentiate between other reasons for activation. For most other events (like connected or blur) the reason ended up simply being a repetition of the event name itself, which isn't useful.

For this reason I propose that in 2.0 we drop the reason attribute on the VR events. Additionally, presuming that we go with the VRSession concept in #179, it DOES seem useful to communicate the session that an event is associated with (if there is one). This means the IDL would end up looking like this:

interface VRDisplayEvent : Event {
  readonly attribute VRDisplay display;
  readonly attribute VRSession? session;
};

Alternatively, we could have separate event types for a display and session, like so:

interface VRDisplayEvent : Event {
  readonly attribute VRDisplay display;
};

interface VRSessionEvent : Event {
  readonly attribute VRSession session;
};

But this feels overly complicated to me without a clear benefit.

@toji toji added this to the 2.0 milestone Feb 23, 2017
@NellWaliczek
Copy link
Member

Thanks for cleaning this up! I've got a slight preference for the latter option; it seems a bit cleaner. The question is for events off of navigator.vr, do we need a VREvent type? My hunch is no, since the connected/disconnected events will still need a VRDisplay as a parameter.
Thoughts?

@toji
Copy link
Member Author

toji commented Feb 23, 2017

I don't think there's any vr-centric events that don't have a related display or session, so I don't think that's needed.

Related, though I didn't want to tackle it in this particular issue: I'm going to propose renaming it to VREvent anyway to prevent some naming conflicts between 1.1/2.0 [Edit: Filed #192 to address this.]

@RafaelCintron
Copy link

I am also in favor of having separate event types. If we have the session point back at the display, the web developer can go from one to the other.

@toji toji mentioned this issue Feb 27, 2017
@toji
Copy link
Member Author

toji commented Mar 1, 2017

Talked with some of the platform leads on Google's end and it seems we have a slight preference for two event types in this situation as well, so I'm going to update the explainer to reflect that.

@toji
Copy link
Member Author

toji commented Mar 8, 2017

Closing since it seems like we've come to a general consensus on this, which is now reflected in the explainer.

If anyone has further issues with the shape of WebVR's event interfaces please open a new issue.

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

No branches or pull requests

4 participants