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

CSS pseudo-class/styling and Fullscreen API integration #14

Merged
merged 4 commits into from
Feb 20, 2020

Conversation

klausw
Copy link
Contributor

@klausw klausw commented Feb 3, 2020

No description provided.

Copy link

@raviramachandra raviramachandra left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@klausw
Copy link
Contributor Author

klausw commented Feb 15, 2020

PTAL, now updated to have independent styling that is decoupled from Fullscreen API's pseudoclass and UA stylesheet. (There's a rule to set :fullscreen::backdrop transparent, but that has no effect for an implementation which isn't based on Fullscreen API.)

@klausw
Copy link
Contributor Author

klausw commented Feb 15, 2020

@Manishearth FYI, if I remember right you were asking about this at the f2f.

transform: none !important;

/* intentionally not !important */
object-fit: contain;
Copy link

Choose a reason for hiding this comment

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

Do you need this rule? It exists for images going fullscreen, and presumably it makes no sense to have an image as the XR overlay element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a strong opinion either way, but I think it would be preferable to keep the rule for consistency. While using an image as the overlay element would be unusual, there isn't really a reason to prohibit it.

A fullscreen-based inplementation would be applying the object-fit rule since the overlay element matches both :fullscreen and :xr-overlay.

index.bs Outdated
The user-agent style sheet defaults for the DOM Overlay element are as follows:

<pre highlight="css">
:xr-dom-overlay {
Copy link

Choose a reason for hiding this comment

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

Having "dom" as part of the web-exposed name is unusual, would just :xr-overlay be OK? (A selector always matches something in the DOM, so this reads a bit like :first-dom-child instead of :first-child.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds reasonable, I'll change it to :xr-overlay. For the others in the thread, please speak up if you feel strongly about it.

Copy link

Choose a reason for hiding this comment

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

Thanks!

index.bs Outdated
}
</pre>

NOTE: This is based on [[FULLSCREEN#user-agent-level-style-sheet-defaults]], with additional styling to make the overlay element transparent. A user agent MAY use the Fullscreen API to implement the DOM Overlay, and in that case its backdrop must also be transparent. The styling for [=:xr-dom-overlay=] does not explicitly depend on the Fullscreen API's pseudoclass or styling so that user agents have the flexibility to implement it independently of the Fullscreen API.
Copy link

Choose a reason for hiding this comment

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

It looks like all the requirements here are just MAY. Do you expect implementations to differ in practice here, or why not a stronger requirement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for not requiring use of the Fullscreen API is that this seemed too restrictive on a platform with multiple displays. For example, when using a desktop PC with a tethered headset, the current WebXR immersive-vr mode continues displaying a 2D web page on the normal monitor while the headset is showing immersive content. In this case, the Fullscreen API could be expected to apply to the 2D monitor display, and a DOM overlay may be using a separate mechanism for the immersive display, so allowing the APIs to work separately seemed preferable.

@Manishearth, since you were one of the people requesting this distinction, do you have more context?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't actually recall when we last discussed this, but yeah, we basically should be able to distinguish between 2d and 3d fullscreen API uses.

Copy link

Choose a reason for hiding this comment

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

It sounds like the important distinctions are in the hardware the browser is running on / attached to, not differences between browsers. If both Chrome and Firefox would show the same behavior on the desktop PC in the example, does the spec spell out what should happen in enough detail for Chrome and Firefox to achieve interoperability here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this should be the case, with the caveat that the specification intentionally leaves some flexibility to the user agent about details such as placement of the overlay. The goal is that applications should be usable across browsers and for different hardware types.

Copy link

Choose a reason for hiding this comment

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

Ah, so I think the problem is that my reading of the "MAY" isn't what's intended here. I've filed #15 to request clarification.

@klausw
Copy link
Contributor Author

klausw commented Feb 20, 2020

I'll merge this to make discussions less confusing, but please let me know if you have additional feedback or requests for further edits.

@klausw klausw merged commit ae468a8 into immersive-web:master Feb 20, 2020
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.

None yet

4 participants