Re: [whatwg/fullscreen] Update fullscreen spec to be Shadow DOM compliant (#54)

foolip commented on this pull request.

This looks great, thank you. I have only nits now, @annevk can you take a look so that we can finalize this in the next round?

> @@ -110,8 +110,8 @@ and <a>add</a> it to its <a>node document</a>'s <a>top layer</a>.
 <p>Whenever the <a>removing steps</a> run with an <var>oldNode</var>, run these steps:
 
 <ol>
- <li><p>Let <var>nodes</var> be <var>oldNode</var>'s <a>inclusive descendants</a> that have their
- <a>fullscreen flag</a> set, in <a>tree order</a>.
+ <li><p>Let <var>nodes</var> be <var>oldNode</var>'s <a>shadow-including inclusive descendants</a>

Nice, this is simpler than traversing in "unfullscreen element".

>  <p>The <dfn attribute for=Document><code>fullscreen</code></dfn> attribute's getter must return
 false if <a>context object</a>'s <a>fullscreen element</a> is null, and true otherwise.
 
 <p class=note>Use
-<a attribute for=Document lt=fullscreenElement><code>document.fullscreenElement</code></a> instead.
+<a attribute for=DocumentOrShadowRoot lt=fullscreenElement><code>document.fullscreenElement</code></a>
+instead.
+
+<p>The <dfn attribute for=DocumentOrShadowRoot><code>fullscreenElement</code></dfn> attribute's
+getter must return the result of <a>retargeting</a> <a>context object</a>'s

Yeah, I agree that putting the retargeting on just `fullscreenElement` seems better, thanks!

>  <p>The <dfn attribute for=Document><code>fullscreen</code></dfn> attribute's getter must return
 false if <a>context object</a>'s <a>fullscreen element</a> is null, and true otherwise.
 
 <p class=note>Use
-<a attribute for=Document lt=fullscreenElement><code>document.fullscreenElement</code></a> instead.
+<a attribute for=DocumentOrShadowRoot lt=fullscreenElement><code>document.fullscreenElement</code></a>
+instead.
+
+<p>The <dfn attribute for=DocumentOrShadowRoot><code>fullscreenElement</code></dfn> attribute's
+getter must return the result of <a>retargeting</a> <a>context object</a>'s
+<a>shadow-including root</a>'s <a>fullscreen element</a> against the <a>context object</a> if the

Here I think it's possible that "context object's shadow-including root's" isn't a document if the shadow host isn't connected? Would it work to just use "node document" here and let that case fall back to "in the same tree, otherwise return null"?

>  <p>The <dfn attribute for=Document><code>fullscreen</code></dfn> attribute's getter must return
 false if <a>context object</a>'s <a>fullscreen element</a> is null, and true otherwise.
 
 <p class=note>Use
-<a attribute for=Document lt=fullscreenElement><code>document.fullscreenElement</code></a> instead.
+<a attribute for=DocumentOrShadowRoot lt=fullscreenElement><code>document.fullscreenElement</code></a>

I think that just `{{DocumentOrShadowRoot/fullscreenElement}}` would be fine here, I preserved the `document.` when converting to Bikeshed, but in the other two notes it's just "fullscreenElement". Unless @annevk had some reason to spell this out.

> @@ -505,9 +516,16 @@ properties apply to this pseudo-element either.
 <p>The <dfn id=css-pc-fullscreen><code>:fullscreen</code></dfn> pseudo-class must match any
 <a>element</a> that has its <a>fullscreen flag</a> set.
 
-<p class="note no-backref">This makes it different from the {{Document/fullscreenElement}} API,
+<p class="note no-backref">This makes it different from the
+{{DocumentOrShadowRoot/fullscreenElement}} API,
 which returns a <a>document</a>'s <a>fullscreen element</a>.

In order to avoid repeating the details about "or shadow root" and retargeting here, mayb just say "This makes it different from the fullscreenElement API, which returns the topmost fullscreen element", without linking to the definition of "fullscreen element"? I fear that trying to make this technically correct will just make it harder to understand what the difference is.

>  which returns a <a>document</a>'s <a>fullscreen element</a>.
 
+For each <a>node tree</a>, if the result of <a>retargeting</a> its <a>shadow-including root</a>'s

This is about making the shadow host also match the `:fullscreen` selector, right? The shadow host isn't being automatically added to the top layer, so it won't have a `::backdrop` pseudo-element and would never be visible unless the fullscreen element has a transparent background.

In this revision of this PR, shadow trees are handled quite unlike iframes, and I think that actually turned out better, so can we take it even further?

(Not sure how to implement this otherwise, actually doing this as part of selector matching would be too slow.)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/fullscreen/pull/54#pullrequestreview-737181