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

Clarified when immersive sessions are rejected #360

Merged
merged 1 commit into from
Feb 13, 2019
Merged

Conversation

toji
Copy link
Member

@toji toji commented May 25, 2018

Fixes #343.

@NellWaliczek
Copy link
Member

Looking at issue #343, it sounds like that's a solid approach for in-progress session creation requests. What about existing sessions? What's the reasoning behind blocking a new session from supplanting an existing session? Off the top of my head that seems potentially unnecessarily restrictive...

Copy link
Contributor

@cvan cvan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! if you agree it would be useful, could you add a code example of handling the rejected Promise?

@RafaelCintron
Copy link

One of the reasons requestSession returns a promise (at least in desktop) is so that the browser can ask the user permission to enter exclusive mode for a given domain. If we allow new sessions to supplant existing sessions in all cases, that means we need to ask the user for this permission inside of the existing exclusive session, thus breaking the original experience. I suppose we can allow it for same domain requests but, in that case, the developer might as well figure out a way to reuse the session objects themselves.

For the time being, I'm inclined to favor the "one at a time" proposal in this PR.

@NellWaliczek
Copy link
Member

I see where you're coming from. Thinking this through a couple extra steps..here are two other things to consider:

  1. What about iframes? Do we want to enable a user to cause the active session to be supplanted by a session from another iframe?
  2. Won't we need to address the "where is my permissions dialog?" question when we wrap up the page-to-page navigation design?

@RafaelCintron
Copy link

What about iframes? Do we want to enable a user to cause the active session to be supplanted by a session from another iframe?

Maybe but we should think through the ramifications.

The user experience starts with the user saying 'yes' to a specific domain. I'm willing to let other iframes from the same domain supplant with no permission and maybe even the top level document. After all, the user made the choice to navigate to the top domain in the first place. But not so sure about some unrelated domain on the page that may not be visible.

Even if we get the user's permission, if we allow unrelated domains to supplant each other, we could end up with two pieces of code fighting for control for the HMD with blur events flying all over the place. We need to put the user in control of these situations as much as we can.

Won't we need to address the "where is my permissions dialog?" question when we wrap up the page-to-page navigation design?

I'm less worried about page-to-page navigation. Here, only the top-level domain will be navigating. iFrames can't navigate the top level document, only the iframe. Any dialogs for page to page navigation won't really be "permission" dialogs but dialogs that contain the name of the new place and a progress indicator.

@ryzngard
Copy link

I think we should consider what type of experiences will be restricted by this decision.

Off the top of my head, I can think of at least one; A gallery of XR experiences. Do we require all experiences in the gallery to be hosted in the same origin to work as a continuous experience? Is it enough to require that such an experience use navigation to achieve it's goal rather than overtaking a session?

The iframe issue is interesting. I looked at some of how fullscreen handles this paradigms, and it looks like there's an explicit attribute on iframe allowfullscreen. Will we require a similar attribute for xr?

@NellWaliczek NellWaliczek added this to the CR for 1.0 milestone Sep 13, 2018
@NellWaliczek NellWaliczek modified the milestones: CR for 1.0, Jan '19 F2F Nov 7, 2018
@toji toji force-pushed the exclusive_race branch 2 times, most recently from 1958214 to d16fbf2 Compare February 12, 2019 22:58
@toji toji changed the title Clarified when exclusive sessions are rejected Clarified when immersive sessions are rejected Feb 12, 2019
@toji
Copy link
Member Author

toji commented Feb 12, 2019

Okay, finally (finally) coming back around to this issue. Rebased the commit and re-read the comments to figure out where we stand on this. My conclusion is that the original assertion that only one immersive session, whether pending or resolved, is allowed at a time still stands. In fact, it turns out that the spec already had language to that effect in it, so this is one of the few areas where the explainer is lagging behind.

To address a couple of other questions directly:

Will we require an iframe permission attribute for xr?
Yes, using the feature policy API. This is actually already specified, but there are remaining questions about the feature naming and granularity.

Will this block gallery-style pages that want to transition between isolated experiences hosted on the same origin?
There's still a few ways to achieve that, the simplest answer being that if the page has enough visibility of the experiences to present them as a gallery in the immersive scene then it probably has enough context to share a single immersive session between those experiences. Otherwise I updated the spec in this PR to allow requestSession() to succeed immediately after end() was called on an existing immersive session, which would allow for new immersive sessions to be spawned in response to a single click event from the previous session.

What about more general navigation?
We still need to spec more general navigation capabilities, but that's beyond the scope of this specific PR and should not be affected by this limitation.

Copy link
Member

@NellWaliczek NellWaliczek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable to me (and omg yay we can hopefully finally close this PR???). That said, I'd like to get a confirmation from Rafael that he's happy with this given his prior interest in this issue.

@toji
Copy link
Member Author

toji commented Feb 12, 2019

You forgot to invoke the name of @RafaelCintron with the appropriate runes so that the email imps deep within GitHub will summon him. 😉

@toji toji merged commit eea13b6 into master Feb 13, 2019
@toji toji deleted the exclusive_race branch February 13, 2019 18:49
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

Successfully merging this pull request may close these issues.

5 participants