-
Notifications
You must be signed in to change notification settings - Fork 386
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
Update navigator interface and event targets #116
Conversation
index.bs
Outdated
|
||
<pre class="idl"> | ||
[NoInterfaceObject] | ||
interface VRDisplayEventHandlers { | ||
attribute EventHandler onvrdisplayactivate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are making the breaking change, we should remove on* events and have people use addEventListener. on* should be used only for legacy cases that require its semantics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'll update this in a bit.
index.bs
Outdated
}; | ||
|
||
VR implements EventTarget; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't implements EventTarget, this breaks the ability to do event listener logging by swapping out the EventTarget.prototype values since EventTarget as an implements is not part of your prototype chain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, didn't know that. This syntax was picked up from the WebBluetooth spec, so they've probably got it wrong. Fix incoming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, a lot of people get this wrong. In general, the pro of EventTarget is that it goes into your prototype chain and becomes a single source for the functions add/remove/dispatch, but the con is that it goes into your prototype chain so you have to design it in.
If you do implements it, then you get your own "copy" of those functions and people have to crawl the entire type system if they want to do any sort of event listener logging via script. This is how IE 11 was implemented and then I fixed this in Edge.
index.bs
Outdated
Promise<sequence<VRDisplay>> getVRDisplays(); | ||
readonly attribute FrozenArray<VRDisplay> activeVRDisplays; | ||
readonly attribute boolean vrEnabled; | ||
interface VR { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interface VR : EventTarget
index.bs
Outdated
readonly attribute FrozenArray<VRDisplay> activeVRDisplays; | ||
readonly attribute boolean vrEnabled; | ||
interface VR { | ||
Promise<sequence<VRDisplay>> getDisplays(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider: getDevices, but only if we have a good reason for it. Same for active* if we change the getter here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean here: We did use the terminology "devices" previously, but moved away from that with WebVR 1.0. I don't see any reason to stop using the term "Displays" at this point.
index.bs
Outdated
@@ -168,6 +168,9 @@ interface VRDisplay : EventTarget { | |||
*/ | |||
void submitFrame(); | |||
}; | |||
|
|||
VRDisplay implements EventTarget; | |||
VRDisplay implements VRDisplayEventHandlers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the direction everyone is heading? We preferred partials over implements UNLESS multiple things would use the implements. At this time, only VR would use one set of handlers and only VRDisplay would use the other set. So why implements instead of partial or simply adidng it on the interface directly in this case since we are in a single spec? Curious what others think here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, this was picked up from WebBluetooth. Not sure what the reasoning was there. Worth asking them.
index.bs
Outdated
A user agent MAY dispatch this event type to indicate that a {{VRDisplay}} has been disconnected. | ||
|
||
<dfn event for="VREventHandlers" id="vreventhandlers-vrdisplaynavigate-event">onvrdisplaynavigate</dfn> | ||
A user agent MAY dispatch this event type to indicate that the current page has been navigated to from a page that was actively presenting VR content to the {{VRDisplay}}. The current page can call {{requestPresent()}} in response to this event in order to stay in VR presentation mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to know when to fire this event since this will be one of the indications that we should stay in VR presentation mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, we definitely need to flesh out the order of operations on all the events really. I just wanted to make sure this was declared to head off questions about "wait, if activate
is on the display then how does navigation work?"
index.bs
Outdated
User agents implementing this specification MUST provide the following new DOM events. The corresponding events must be of type {{VRDisplayEvent}} and must fire on the {{vr}} object. Registration for and firing of the events must follow the usual behavior of DOM4 Events. | ||
|
||
<dfn event for="VREventHandlers" id="vreventhandlers-vrdisplayconnect-event">onvrdisplayconnect</dfn> | ||
A user agent MAY dispatch this event type to indicate that a {{VRDisplay}} has been connected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to define more what this means. We've been having issues with the expectations around this event. For instance, a Gear VR is technically always connected, but there is also the USB drop in connection and the mounted indicators. We don't want people to wait to hear a vrdisplayconnect
event before they requestPresent
or otherwise try to use VR.
Now that we have Rift, Gear and DayDream, let's define what we think this means for all 3. Rift is kind of easy since maybe it means you've either plugged it in via USB or that you've told the browser you want to be in VR available mode (this is the toggle I've been proposing for the UI so someone could say, please don't enable VR experiences right now I don't want a page to automatically use my Rift).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely agreed, but maybe as a different PR? (Since we're not changing the definition here, just where it fires from)
index.bs
Outdated
@@ -50,7 +50,7 @@ This section describes the interfaces and functionality added to the DOM to supp | |||
The {{VRDisplay}} interface forms the base of all VR devices supported by this API. It includes generic information such as device IDs and descriptions. | |||
|
|||
<pre class="idl"> | |||
interface VRDisplay : EventTarget { | |||
interface VRDisplay { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be reverted since we do want to extend and not mixin the EventTarget
index.bs
Outdated
@@ -689,7 +718,7 @@ partial interface HTMLIFrameElement { | |||
}; | |||
</pre> | |||
|
|||
The {{allowvr}} attribute is a boolean attribute. When specified, it indicates that {{Document}} objects in the iframe element's browsing context are to be allowed to access VR devices (if it's not blocked for other reasons, e.g. there is another ancestor iframe without this attribute set). {{Document}} objects in an iframe element without this attribute should reject calls to {{getVRDisplays()}} and should not fire any {{VRDisplayEvent}}. | |||
The {{allowvr}} attribute is a boolean attribute. When specified, it indicates that {{Document}} objects in the iframe element's browsing context are to be allowed to access VR devices (if it's not blocked for other reasons, e.g. there is another ancestor iframe without this attribute set). {{Document}} objects in an iframe element without this attribute should reject calls to {{getDisplays()}} and should not fire any {{VRDisplayEvent}}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is 'reject the call' the right lingo for a Promise returning API? I'm actually not sure, but I want to learn ;-) I would expect it to be 'reject the Promise returned by the call to getDisplays()'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does sounds better, agreed.
index.bs
Outdated
<dfn event for="VRDisplay" id="vrdisplayblur-event">vrdisplayblur</dfn> | ||
A user agent MAY dispatch this event type to indicate that presentation to the display by the page is paused by the user agent, OS, or VR hardware. While a {{VRDisplay}} is blurred it does not lose it's presenting status ({{isPresenting}} continues to report true) but {{getFrameData()}} returns false without updating the provided {{VRFrameData}} and {{getPose()}} returns null. This is to prevent tracking while the user interacts with potentially sensitive UI. For example: A user agent SHOULD blur the presenting application when the user is typing a URL into the browser with a virtual keyboard, otherwise the presenting page may be able to guess the URL the user is entering by tracking their head motions. | ||
|
||
<dfn event for="VRDisplay" id="vrdisplayblur-event">vrdisplayfocus</dfn> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small typo on the id, should be vrdisplayfocus
here's some proposed pseudo-code I've written for the common use cases of the APIs. thoughts? Detection// Exposes whether a VR display is available and accessible (e.g., permissions) on the user's system.
let vrButton = document.querySelector('#vr-button');
vrButton.dataset.available = false;
// This would replace `navigator.vrEnabled`.
navigator.vr.getAvailability().then(isAvailable => {
vrButton.dataset.available = isAvailable;
});
navigator.vr.addEventListener('availabilitychanged', e => {
vrButton.dataset.available = e.value;
}); Querying for displays// This would replace `navigator.getVRDisplays()`.
navigator.vr.requestDisplays().then(displays => {
console.log(displays);
// >> [VRDisplay, VRDisplay]
console.log(displays[0]);
// >> VRDisplay {displayId: 1, displayName: "Oculus Rift", isConnected: true…}
console.log(displays[1]);
// >> VRDisplay {displayId: 2, displayName: "HTC Vive", isConnected: true…}
}).catch(err => {
console.error(err);
});
navigator.vr.requestDisplays({
filters: [{displayName: 'Oculus Rift'}]
}).then(displays => {
console.log(displays);
// >> [VRDisplay]
console.log(displays[0]);
// >> VRDisplay {displayId: 1, displayName: "Oculus Rift", isConnected: true…}
});
navigator.vr.requestDisplays({
filters: [{displayNamePrefix: 'Oculus'}]
}).then(displays => {
console.log(displays);
// >> [VRDisplay]
console.log(displays[0]);
// >> VRDisplay {displayId: 1, displayName: "Oculus Rift", isConnected: true…}
});
navigator.vr.requestDisplays({
filters: [
{displayName: 'Oculus Rift'},
{displayName: 'HTC Vive'}
]
}).then(displays => {
console.log(displays);
// >> [VRDisplay]
console.log(displays[0]);
// >> VRDisplay {displayId: 1, displayName: "Oculus Rift", isConnected: true…}
console.log(display[1]);
// >> VRDisplay {displayId: 2, displayName: "HTC Vive", isConnected: true…}
}); Presenting to displays// Present to a connected HTC Vive.
navigator.vr.requestDisplays({
filters: [{displayNamePrefix: 'HTC Vive'}]
}).then(displays => {
if (!displays.length) {
return;
}
return displays[0].requestPresent([
{source: canvas}
]).then(
render(display)
);
});
// Very theoretical use case.
// Present to multiple connected headsets
// if we have both an Oculus Rift and HTC Vive.
navigator.vr.requestDisplays({
filters: [
{displayNamePrefix: 'Oculus Rift'},
{displayNamePrefix: 'HTC Vive'}
]
}).then(displays => {
return Promise.all(displays.map(displays => {
return display.requestPresent([
{source: canvas}
]).then(
render(display)
);
});
}).catch(err => {
console.error(err);
});
function render (display) {
var onVRFrame = () => {
display.requestAnimationFrame(onVRFrame);
display.getFrameData(frameData);
// …
display.submitFrame();
};
return () => {
display.requestAnimationFrame(onVRFrame);
};
} Link traversalIf display(s) is presenting, after the page is unloaded, the browser sets The next page can look at if (navigator.vr.referringDisplay) {
presentOrRender(navigator.vr.referringDisplay);
} else {
navigator.vr.requestDisplays().then(displays => {
if (!displays.length) {
return Promise.reject();
}
return presentOrRender(displays[0]);
});
}
function presentOrRender (display) {
if (display.isPresenting) {
return render(display);
}
return display.requestPresent([
{source: canvas}
]).then(
render(display)
);
} |
What would the benefit be for the API itself to support filtering in |
I'm not particularly in favor of filtering based on the name. For one, that encourages needlessly exclusionary pages and two it relies on browser all exposing the name the same way AND never changing it. (Even the underlying SDK could change how it's reported and possibly break things.) True, you can already do this if you really want by just looping through the detected headsets and selecting one with name == "...", but that's an antipattern that I'd rather not see baked into the API. I can see an argument for filtering on capabilities. Querying only 6DoF devices or only those with motion controls seems valuable. But again, it encourages exclusionary uses when I'd rather see developers focusing on progressive enhancements 99% of the time. (If you build a Tilt Brush clone, sure, require motion controllers, but don't block me from your 360 photo viewer because all I have is cardboard.) Given the shape of the API filtering could be added in later without much trouble, so I'd vote to hold off on that till v2. I'm curious about your intentions for |
thanks, good feedback. I agree the filtering is probably out of scope. It just seems like a common thing to want to get a Vive if it's present. what are your thoughts on the |
Hm... As I recall it was Mozilla that wanted the "display" name in the first place. ;) I'm not really invested in either name, but I'm not excited to churn verbiage simply for the sake of churn. I also think there's something to be said for making it explicit that this API is intended to deal with devices that display content. That nicely separates it from things like controllers, which are arguable a "VRDevice" but definitely not a "VRDisplay". |
partial interface Window { | ||
attribute EventHandler onvrdisplayconnect; | ||
attribute EventHandler onvrdisplaydisconnect; | ||
attribute EventHandler onvrdisplayactivate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this is a silly question... but where did this portion of the idl get moved to? I thought the events were going to be added to VRDisplay and while I see the comments moved above, but I don't see the EventHandlers themselves. Am I misunderstanding how this works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are making the breaking change, we should remove on* events and have people use addEventListener. on* should be used only for legacy cases that require its semantics.
#116 (comment)
Justin suggested that we remove them, which seemed reasonable to me. I'd like to make the required events more visible and easy to scan, but I've also found that there's no real convention for how events are presented in similar specs. Suggestions welcome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As at @domenic suggests, the way to do this is to add "attribute EventHandlers". The HTML 5 spec does this for events like onload, onplaying and many others.
index.bs
Outdated
@@ -213,6 +213,24 @@ Returns an array with the {{VRLayer}} currently being presented. MUST return an | |||
<dfn method for="VRDisplay">submitFrame()</dfn> | |||
Captures the current state of the {{VRLayer}} currently being presented and displays it on the {{VRDisplay}}. It is assumed that the frame was rendered using the {{VRPose}} and matrices provided by the last call to {{getFrameData()}}. If {{getFrameData()}} was not called prior to calling {{submitFrame()}} the user agent MAY warn the user of potentially malformed visuals or prevent the frame from being shown at all. | |||
|
|||
### Events ### {#vrdisplay-events} | |||
|
|||
User agents implementing this specification MUST provide the following new DOM events. The corresponding events must be of type {{VRDisplayEvent}} and must fire on a {{VRDisplay}} object. Registration for and firing of the events must follow the usual behavior of DOM4 Events. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can just say The UA MUST provide…
index.bs
Outdated
Promise<sequence<VRDisplay>> getVRDisplays(); | ||
readonly attribute FrozenArray<VRDisplay> activeVRDisplays; | ||
readonly attribute boolean vrEnabled; | ||
interface VR : EventTarget{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't you want to update index.idl
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or we could just remove the file. thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space before {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the IDL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm! opened #140
index.bs
Outdated
interface VR : EventTarget{ | ||
Promise<sequence<VRDisplay>> getDisplays(); | ||
readonly attribute FrozenArray<VRDisplay> activeDisplays; | ||
readonly attribute boolean isEnabled; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this should be a Promise
that resolves a boolean of true
or false
. and possibly let's call it like navigator.vr.getAvailability()
.
index.bs
Outdated
readonly attribute boolean vrEnabled; | ||
interface VR : EventTarget{ | ||
Promise<sequence<VRDisplay>> getDisplays(); | ||
readonly attribute FrozenArray<VRDisplay> activeDisplays; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking about this for a while. I'm not sure how useful this attribute is, especially considering the liveness of the VRDisplay
instances (they have isConnected
and isPresenting
attributes).
how about instead having navigator.vr.referringDisplays()
, which would resolve the displays that are actively being presented to and set upon page navigation, and the next page could look at those to know which to present to (though in most cases, only one)?
index.bs
Outdated
|
||
User agents implementing this specification MUST provide the following new DOM events. The corresponding events must be of type {{VRDisplayEvent}} and must fire on a {{VRDisplay}} object. Registration for and firing of the events must follow the usual behavior of DOM4 Events. | ||
|
||
<dfn event for="VRDisplay" id="vrdisplayactivate-event">vrdisplayactivate</dfn> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how do you feel about dropping the vrdisplay
prefixes?
index.bs
Outdated
Return a Promise which resolves to a list of available {{VRDisplay}}s. The Promise MUST be rejected if the {{Document}} object is inside an iframe that does not have the {{allowvr}} attribute set. | ||
|
||
<dfn attribute for="Navigator" id="navigator-activevrdisplays-attribute">activeVRDisplays</dfn> | ||
{{activeVRDisplays}} includes every {{VRDisplay}} that is currently presenting. | ||
<dfn attribute for="VR" id="vr-activedisplays-attribute">activeDisplays</dfn> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this? why can't we just use getDisplays()
and look at displays[0].isPresenting
?
index.bs
Outdated
<dfn attribute for="VR" id="vr-activedisplays-attribute">activeDisplays</dfn> | ||
{{activeDisplays}} includes every {{VRDisplay}} that is currently presenting. | ||
|
||
<dfn attribute for="VR" id="vr-isenabled-attribute">isEnabled</dfn> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason this isn't a Promise? why not getAvailability()
?
index.bs
Outdated
|
||
<dfn attribute for="Navigator" id="navigator-vrenabled-attribute">vrEnabled</dfn> | ||
The {{vrEnabled}} attribute's getter must return true if the context object is allowed to use the feature indicated by attribute name {{allowvr}} and VR is supported, and false otherwise. | ||
<dfn event for="VR" id="vrdisplayconnect-event">vrdisplayconnect</dfn> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to prefix these events with vrdisplay
? FWIW, the events in the Web Bluetooth spec aren't prefixed with anything.
and we should probably use the past tense, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call on dropping the prefix.
As for the tense, I recall that we've discussed that before and found that the web at large is inconsistent. I would LOVE to be pointed at some proper codified guidance that says one or the other. I have no personal preference, as long as the spec is at least internally consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I forget what our original rationale was. perhaps some DOM person (@annevk?) could point us to some reference/spec?
there's already a bunch of inconsistencies across APIs (an obvious one that comes to mind is load
vs. DOMContentLoaded
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://w3ctag.github.io/design-principles/#casing-rules by @domenic covers some but not tense. I think past tense is generally not used for events. We use past sense for promise-state attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking through https://html.spec.whatwg.org/#events-2 it looks like present tense is correct (with some legacy exceptions as noted). I'll add that to the design principles list.
index.bs
Outdated
<dfn event for="VR" id="vrdisplaydisconnect-event">vrdisplaydisconnect</dfn> | ||
A user agent MAY dispatch this event type to indicate that a {{VRDisplay}} has been disconnected. | ||
|
||
<dfn event for="VR" id="vrdisplaynavigate-event">vrdisplaynavigate</dfn> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when would this fire. IMO, we should use a separate method such as navigator.getReferringDisplays()
(or similarly named)? (see navigator.bluetooth.referringDisplay
, for inspiration.)
No!! Event handler IDL attributes are here to stay and should be on all new interfaces as well as old ones. w3ctag/design-principles#29
Agreed. |
index.bs
Outdated
A user agent MAY dispatch this event type to indicate that a {{VRDisplay}} has been disconnected. | ||
|
||
<dfn event for="VR" id="vr-navigate-event">navigate</dfn> | ||
A user agent MAY dispatch this event type to indicate that the current page has been navigated to from a page that was actively presenting VR content to the {{VRDisplay}}. The current page can call {{requestPresent()}} in response to this event in order to stay in VR presentation mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"... has been navigated to from a page ...." The words to and from being next to each other confuses me. I presume what was meant to go here is " ... has been navigated from a page ... "
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call on the phrasing.
borrowing some verbiage from the WHATWG's HTML Standard, perhaps we could change the text to read like this:
… the current browsing context is navigated from the initial document to another …
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to get @annevk's opinion on anything that ties into navigation. From a brief skim this seems redundant with the Window's load event, but I'm sure I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be good to also get folks' opinions on the proposed interfaces (inspired by the Web Bluetooth API interfaces) outlined here (instead of, or in supplement, to the events): https://blog.mozvr.com/connecting-virtual-worlds-hyperlinks-in-webvr/#howitworks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the current browsing context is navigated from the initial document to another" would be way too unspecific. You basically need to add a hook in HTML's processing model somewhere that defines exactly when this event is dispatched, provided it is indeed needed.
That is, you need to write a patch for the HTML Standard to enable this kind of thing.
partial interface Window { | ||
attribute EventHandler onvrdisplayconnect; | ||
attribute EventHandler onvrdisplaydisconnect; | ||
attribute EventHandler onvrdisplayactivate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As at @domenic suggests, the way to do this is to add "attribute EventHandlers". The HTML 5 spec does this for events like onload, onplaying and many others.
I suggest adding a new vrdisplay event type, called something like What do you think? |
This updates the VR interfaces to remove dependencies on the `window` object and enable use of the API in contexts other than the main page thread, such as workers. (Note that the WorkerNavigator interface and worker exposure meta tags will be added in a follow-up PR)
Just rebased this patch. I think at this point it's non-controversial, though we'll probably need to make a few follow up tweaks. I do want to get this merged soon because we will be discussing several other significant changes to be built on top of it. I'll leave it up for the day, but barring any significant concerns being raised I plan on merging this tonight. |
@toji I did a bunch of testing, and I have some proposed changes to this interface. I'll show you a mock interface I made for |
That would be great, @cvan! Please do send them over. I've definitely got some changes to propose as well beyond what this patch does, but my proposals are more disruptive so I wanted to bring them up separately. |
In my extension that polyfills It's probably a bit hard to follow without all the context. We've talked about this a lot in various issues, including #30, this PR, and at the W3C Workshop and in meetings. Let me know if this helps convey what I was thinking. Pay attention to I researched Web Bluetooth, WebUSB, Presentation API, Web Audio, Web MIDI, among others, and this felt the most right to me. Open to suggestions. This will probably be easier to go through over our meeting next week, but I'd love to get people thinking about this now. |
@cvan: I'm assuming the changes that you talked about are what you showed in the IDL you posted to #179? As I mentioned in that issue, I think the concepts you're presenting have merit but should probably be broken out into individual issues/proposals/pull requests. In that light, it seems like there's value to merging this PR as-is, even if we want to make some adjustments on top of it, simply so that the delta of future updates is smaller and more manageable? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking really good. I'm super pumped to get this in. see my comments :)
@@ -169,6 +169,12 @@ interface VRDisplay : EventTarget { | |||
* created without preserveDrawingBuffer set to true will be cleared. | |||
*/ | |||
void submitFrame(); | |||
|
|||
attribute EventHandler onactivate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just call this mount
and unmount
? the verbiage is confusing (see #163).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there's an event for this, IMO there should be a boolean (isMounted
) à la isConnected
and isPresenting
. for example, during page navigation, I wouldn't expect activate
to fire, but I may want to know (and can't assume) whether a headset is (still) worn.
attribute EventHandler onactivate; | ||
attribute EventHandler ondeactivate; | ||
attribute EventHandler onblur; | ||
attribute EventHandler onfocus; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given we have blur
and focus
, it's odd there's no visibilitychange
(especially since we have a presentchange
).
FWIW, because the original blur
and focus
events weren't meaningful enough, browsers introduced the Page Visibility API, which gives you document.visibilityState
, document.hidden
, document.onvisibilitychange
.
attribute EventHandler ondeactivate; | ||
attribute EventHandler onblur; | ||
attribute EventHandler onfocus; | ||
attribute EventHandler onpresentchange; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a bit odd that we have presentchange
as opposed to presentbegin
and presentend
(or presententer
and presentexit
)? (Also, see comment above.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was modeled after fullscreenchange
and pointerlockchange
, which seemed to me like similar state transitions. I also see that the Page Visibility API include visibilitychange
, so I'd say this verbiage is consistent with the rest of the web platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see different tenses and inconsistent patterns of usage and naming (and method signatures/args) with event names, booleans vs. state enums, and property types even with the newer APIs. The Presentation API, Web MIDI, Web USB, and Web Bluetooth in particular all handle input/output a little differently.
Would like to get @annevk, et. al.'s opinions on these, so we can change the API interfaces now to be as closest to the latest best practice today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a little easier to discuss design in dedicated threads, rather than subthreads of a PR.
What matters here are the use cases and what the typical programming experience would be like. Having said that, exposing state transitions through promises is the latest trend and if that makes sense for your API that'd the best to align on.
index.bs
Outdated
A user agent MAY dispatch this event type to indicate that something has occured which suggests the {{VRDisplay}} should exit presentation. For example, if the {{VRDisplay}} is capable of detecting when the user has taken it off, this event SHOULD fire when they do so with the reason "unmounted". | ||
|
||
<dfn event for="VRDisplay" id="vrdisplay-onblur-event">onblur</dfn> | ||
A user agent MAY dispatch this event type to indicate that presentation to the {{VRDisplay}} by the page is paused by the user agent, OS, or VR hardware. While a {{VRDisplay}} is blurred it does not lose it's presenting status ({{isPresenting}} continues to report true) but {{getFrameData()}} returns false without updating the provided {{VRFrameData}} and {{getPose()}} returns null. This is to prevent tracking while the user interacts with potentially sensitive UI. For example: A user agent SHOULD blur the presenting application when the user is typing a URL into the browser with a virtual keyboard, otherwise the presenting page may be able to guess the URL the user is entering by tracking their head motions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will there be a reason
in the event detail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a "blur" reason and text indicating it must be used here.
index.bs
Outdated
A user agent MAY dispatch this event type to indicate that presentation to the {{VRDisplay}} by the page has resumed after being blurred. | ||
|
||
<dfn event for="VRDisplay" id="vrdisplay-onpresentchange-event">onpresentchange</dfn> | ||
A user agent MUST dispatch this event type to indicate that the {{VRDisplay}} has begun or ended VR presentation. This event should not fire on subsequent calls to {{requestPresent()}} after the {{VRDisplay}} has already begun VR presentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will there be a reason
in the event detail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added "presentstart" and "presentend" reasons and text indicating they must be used here.
index.bs
Outdated
A user agent MAY dispatch this event type to indicate that presentation to the {{VRDisplay}} by the page is paused by the user agent, OS, or VR hardware. While a {{VRDisplay}} is blurred it does not lose it's presenting status ({{isPresenting}} continues to report true) but {{getFrameData()}} returns false without updating the provided {{VRFrameData}} and {{getPose()}} returns null. This is to prevent tracking while the user interacts with potentially sensitive UI. For example: A user agent SHOULD blur the presenting application when the user is typing a URL into the browser with a virtual keyboard, otherwise the presenting page may be able to guess the URL the user is entering by tracking their head motions. | ||
|
||
<dfn event for="VRDisplay" id="vrdisplay-onfocus-event">onfocus</dfn> | ||
A user agent MAY dispatch this event type to indicate that presentation to the {{VRDisplay}} by the page has resumed after being blurred. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will there be a reason
in the event detail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a "focus" reason and text indicating it must be used here.
index.bs
Outdated
interface VR : EventTarget { | ||
Promise<sequence<VRDisplay>> getDisplays(); | ||
readonly attribute FrozenArray<VRDisplay> activeDisplays; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was suggesting adding navigator.vr.getAvailability
(as a replacement for the navigator.vrEnabled
property, which should obviously be removed).
This would respect the Feature Policy (which supports <iframe enable="vr" src="https://example.com/scene.html"></iframe>
– see issue #86).
Besides the aforementioned <iframe>
case, there are cases (e.g., browser extension) when a UA may not want to (or cannot attempt to) expose VR capabilities. (A VR display could still be connected, but the page may not be privileged to use it.)
Some similar methods in other web APIs:
new PresentationRequest([urls]).getAvailability().then(…)
navigator.bluetooth.getAvailability().then(…)
navigator.usb.requestDevice({filters: [], optionalServices: [], acceptAllDevices: true})
.then(…)
Alternatively, instead of a Promise<boolean> getAvailability()
, we could have the following:
readonly attribute VRAvailability availability;
attribute EventHandler onavailabilitychange;
enum VRAvailability {
'available',
'unavailable',
'unknown'
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM, thanks for the research and suggestion! I'll make the update soon.
index.bs
Outdated
readonly attribute FrozenArray<VRDisplay> activeVRDisplays; | ||
interface VR : EventTarget { | ||
Promise<sequence<VRDisplay>> getDisplays(); | ||
readonly attribute FrozenArray<VRDisplay> activeDisplays; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense to me. I'm unclear of the use case, and it's ambiguous.
See my proposed navigator.vr.referringDisplay(s)
idea: https://gist.github.com/cvan/9c0a2e6e1a6839816128aeb461eb3184#file-sample-usage-js-L21-L32
À la navigator.languages
), I suggest having introducing a navigator.vr.preferredDisplays
, an array of display name strings for user-preferred VR headsets.
If we think of VR devices/displays à la languages/locales, you can imagine a user flow something like this:
- When a user first plugs in a headset, the UA remembers that headset name in some internal persisted list of "known VR displays" in that user's UA profile.
- A user opens the UA's "Preferences" to add/remove/rearrange the preferred headsets to use when display WebVR content.
- When a requests any page, the browser now starts sending a
Accept-VR-Display
request header (e.g.,Accept-VR-Display: HTC Vive, Oculus Rift
) – à la theAccept-Language
request header [e.g.,Accept-Language: en-US, fr; q=0.7
]). - If a page's response header is
Content-VR-Display: Gear VR, HTC Vive
(or a<meta>
tag:<meta http-equiv="content-vr-display" content="available HTC Vive, Oculus Rift">
– [or CSP-Style:<meta http-equiv="Content-VR-Policy" content="default-display 'HTC Vive'; supported-displays 'HTC Vive', 'Oculus Rift'; prefetch-navigation-to 'https://*.neocities.org'">
]), then the UA knows which displays are supported by this WebVR page (or scenes within the page, if we're going to get complicated). - The UA now knows that when the page loads, the user may want to enter VR by clicking on some in-content button that calls
<VRDisplay>.requestPresent(…)
(where<VRDisplay>
would be a VR Display that exists innavigator.vr.preferredDisplays
and is also supported by the content creator of the WebVR scene).
Here's what I mocked up in Firefox's Preferences, for example:
If while a WebVR source is presenting to a headset and a window navigation occurs, the UA can ought to *fire the navigate
event on navigator.vr
in addition to pushing the display to navigator.vr.referringDisplays
(i.e., attribute FrozenArray<VRDisplay> referringDisplays
).
A developer could then check that and immediately request presentation back to the display the user was using before/during the page navigation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think allowing sites to say which headsets they support would be as dangerous as letting them say which browsers or OS they support and increase fragmentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was giving some thought to this as well, since it's going to be very difficult to get pages to always properly handle multiple headsets. (Even my WebVR.info samples don't, though there's comments in the source alluding to the fact that you should.) I think it's going to be very very very very common to have sited just use display[0]
.
It seems like the dialog that you mocked up above could be pretty effective if the only thing it did was control the order that headsets are exposed in through getDisplays()
. Want to use your Vive instead of your Rift? Make it always show up as display[0]
. Sites that do the dumb thing will almost always pick it up, sites that are better behaved will allow you to pick anyway.
(A page can still break it by actively filtering out displays unless displayName.includes('Rift')
, but at that point they're just being actively user hostile and I don't think we can help them much.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm quite happy with these changes. just had a few questions about clarification and the naming of the events (no on…
) vs. event target methods (yes on…
). very close! great work :)
P.S. I have quite a few other ideas - some of which I commented on/filed now, while the rest of the ideas I've mentally tabled for now (scribbled down in a Gist to articulate later).
attribute EventHandler ondeactivate; | ||
attribute EventHandler onblur; | ||
attribute EventHandler onfocus; | ||
attribute EventHandler onpresentchange; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see different tenses and inconsistent patterns of usage and naming (and method signatures/args) with event names, booleans vs. state enums, and property types even with the newer APIs. The Presentation API, Web MIDI, Web USB, and Web Bluetooth in particular all handle input/output a little differently.
Would like to get @annevk, et. al.'s opinions on these, so we can change the API interfaces now to be as closest to the latest best practice today.
index.bs
Outdated
|
||
The UA MUST provide the following new events. The corresponding events must be of type {{VRDisplayEvent}} and must fire on a {{VRDisplay}} object. Registration for and firing of the events must follow the usual behavior of DOM4 Events. | ||
|
||
<dfn event for="VRDisplay" id="vrdisplay-onactivate-event">onactivate</dfn> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these should not have the on
; event names never have prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since OpenVR uses the activate/deactivate terminology, you can ignore my other comment about mounted/unmounted.
index.bs
Outdated
A user agent MAY dispatch this event type to indicate that something has occured which suggests the {{VRDisplay}} should be presented to. For example, if the {{VRDisplay}} is capable of detecting when the user has put it on, this event SHOULD fire when they do so with the {{VRDisplayEvent/reason}} "mounted". | ||
|
||
<dfn event for="VRDisplay" id="vrdisplay-ondeactivate-event">ondeactivate</dfn> | ||
A user agent MAY dispatch this event type to indicate that something has occured which suggests the {{VRDisplay}} should exit presentation. For example, if the {{VRDisplay}} is capable of detecting when the user has taken it off, this event SHOULD fire when they do so with the {{VRDisplayEvent/reason}} "unmounted". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: "occured" should be "occurred"
index.bs
Outdated
|
||
<dfn event for="VRDisplay" id="vrdisplay-onactivate-event">onactivate</dfn> | ||
A user agent MAY dispatch this event type to indicate that something has occured which suggests the {{VRDisplay}} should be presented to. For example, if the {{VRDisplay}} is capable of detecting when the user has put it on, this event SHOULD fire when they do so with the {{VRDisplayEvent/reason}} "mounted". | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a update
or statechange
event that fires when a property on the VRDisplay
object changes? see this OpenVR event.
since there's no way to observe an object (anymore), this can be problematic when folks want to know when the property's values have changed but no event otherwise fired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also see the TrackedDevicePropertiesChanged
OpenVR event.
|
||
attribute EventHandler ondisplayconnect; | ||
attribute EventHandler ondisplaydisconnect; | ||
attribute EventHandler onnavigate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to improve the whole Web platform and just properly support a navigate
event in the form of @jakearchibald's latest Navigation Transitions proposal. I understand the desire to not cause cross-spec dependencies and being able to nicely confine the WebVR API surface area to this navigator.vr
namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this event integrate with HTML? Is it properly hooked?
Promise<sequence<VRDisplay>> getVRDisplays(); | ||
readonly attribute FrozenArray<VRDisplay> activeVRDisplays; | ||
interface VR : EventTarget { | ||
Promise<boolean> getAvailability(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use SecureContext
:
[SecureContext] Promise<boolean> getAvailability();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cvan Out of curiosity, what is the rational for marking getAvailability
with [SecureContext]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I thought SecureContext
is required for Feature Policy stuff. but, it appears – as I should have known – that SecureContext
literally means for https:
window contexts.
Promise<boolean> getAvailability(); | ||
Promise<sequence<VRDisplay>> getDisplays(); | ||
|
||
attribute EventHandler ondisplayconnect; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also [SecureContext]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps we add a displayadd
event (à la OpenVR's IServerDriverHost. TrackedDeviceAdded)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cvan , Are you suggesting we add displayadd
in addition to displayconnect
or replace displayconnect
with displayadd
? In your mind, what is the difference between 'adding' a display and 'connecting' a display?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, replace. I find the displayadd
verbiage make a bit more sense.
Promise<sequence<VRDisplay>> getDisplays(); | ||
|
||
attribute EventHandler ondisplayconnect; | ||
attribute EventHandler ondisplaydisconnect; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add onavailabilitychange
(imagine this could change based on a UA-presented dialogue via the Permissions API, for example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see Web Bluetooth's, for measure
@@ -169,6 +169,12 @@ interface VRDisplay : EventTarget { | |||
* created without preserveDrawingBuffer set to true will be cleared. | |||
*/ | |||
void submitFrame(); | |||
|
|||
attribute EventHandler onactivate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there's an event for this, IMO there should be a boolean (isMounted
) à la isConnected
and isPresenting
. for example, during page navigation, I wouldn't expect activate
to fire, but I may want to know (and can't assume) whether a headset is (still) worn.
@@ -619,6 +676,19 @@ The user agent MAY request start VR presentation mode. This allows user agents t | |||
<dfn enum-value for="VRDisplayEventReason">unmounted</dfn> | |||
The {{VRDisplay}} has detected that the user has taken it off. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we have navigationstart
and/or beforenavigation
events (similar to window
's beforeunnload
and unload
events)?
@toji: this looks quite good to me. if you can address the last few comments, I'm comfortable with this getting merged. these in particular could use comments, and, if necessary, spec tweaks (or new issues filed):
awesome work. and, thanks for the patience. apologies for derailing the conversation; moving some properties and changing the interfaces involved dropping some and considerable rethinking. doing this in piecemeal is smart, and this is starting to shape up nicely. thanks! |
Addressed several more points of feedback from @cvan, especially around how the events are documented (now has an attribute doc for the Going to merge this now, and follow up with creating some separate issues for items that weren't addressed in the initial pull. |
THIS IS A BACKWARDS-COMPATIBILITY BREAKING CHANGE
TL;DR:
navigator.getVRDisplays()
is nownavigator.vr.getDisplays()
, connect and disconnect events now fire from thenavigator.vr
, and all other events fire from theVRDisplay
. This is to enable use in workers or other non-main-thread contexts.This updates the VR interfaces to remove dependencies on the
window
object and enable use of the API in contexts other than the main page thread, such as workers. (Note that the WorkerNavigator interface and worker exposure meta tags will be added in a follow-up PR.)Because workers don't have access to the
window
object the API cannot depend on events that are fired from thewindow
object if we want it to be useable in a worker or similar context. To facilitate this most events will be fired from theVRDisplay
that the event is associated with, with the exception of events that describe display lifetime, such asvrdisplayconnect
andvrdisplaydisconnect
, which will now be fired off of thevr
"namespace" object.Also added a
vrdisplaynavigate
event that fires from thevr
object which is functionally similar tovrdisplayactivate
but is specifically for page-to-page navigation within VR. With the new changes it's not practical to havevrdisplayactivate
handle navigation events since the event is intended to be fired early in the page lifetime and the page may not have had a chance to callgetDisplays
and install the appropriate listeners.