Bug 96332 – Hide all ancestors of the full screen element when going full screen

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adrienne Walker]

Hide all ancestors of the full screen element when going full screen

[Adrienne Walker]

Created attachment 163228 [details]
Patch

[Adrienne Walker]

See: http://code.google.com/p/chromium/issues/detail?id=145562.

Test case forthcoming.

[Adrienne Walker]

Created attachment 163419 [details]
Patch

[Jer Noble]

The correct way to fix this is not to hide all non-full screen elements, but to make all ancestors non-fixed position.  Also, this code will be unnecessary once the new full screen stacking context work (in the newest version of the full screen spec) goes in.

[Adrienne Walker]

(In reply to comment #4)
> The correct way to fix this is not to hide all non-full screen elements, but to make all ancestors non-fixed position.  Also, this code will be unnecessary once the new full screen stacking context work (in the newest version of the full screen spec) goes in.

Why is removing fixed position a better solution than hiding?

Also, I'm trying to fix this now so that it doesn't block the "fixed position creates stacking context" flag in Chromium.  Please rip this out once the new stacking context work goes in.

[WebKit Review Bot]

Comment on attachment 163419 [details]
Patch

Attachment 163419 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13806909

New failing tests:
http/tests/fullscreen/fullscreenelement-same-origin.html
http/tests/fullscreen/fullscreenelement-different-origin.html
plugins/fullscreen-plugins-dont-reload.html

[Jer Noble]

(In reply to comment #5)
> (In reply to comment #4)
> > The correct way to fix this is not to hide all non-full screen elements, but to make all ancestors non-fixed position.  Also, this code will be unnecessary once the new full screen stacking context work (in the newest version of the full screen spec) goes in.
> 
> Why is removing fixed position a better solution than hiding?

Because instead of only affecting ancestors with fixed position, this affects all ancestors.  Additionally, since the list of ancestors necessarily includes the body element, won't this effectively hide the entire (non-full screen) DOM?

[Adrienne Walker]

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > The correct way to fix this is not to hide all non-full screen elements, but to make all ancestors non-fixed position.  Also, this code will be unnecessary once the new full screen stacking context work (in the newest version of the full screen spec) goes in.
> > 
> > Why is removing fixed position a better solution than hiding?
> 
> Because instead of only affecting ancestors with fixed position, this affects all ancestors.  Additionally, since the list of ancestors necessarily includes the body element, won't this effectively hide the entire (non-full screen) DOM?

Yes, it will hide the entire non-full screen DOM.  Isn't that the intention when going full screen, that you want the full screen element to be on top and cover the rest of the page?

I am not a stacking context expert, but just removing fixed position doesn't feel sufficient to me.  The way the current implementation of full screen works seems to be to set the z-index of the full screen renderer to be INT_MAX, in an effort to make it sort on top of the rest of the page.  If the render layer created by the full screen renderer has some non-root ancestor stacking context (fixed pos or not), it seems like the full screen renderer is going to potentially be sorted incorrectly within that ancestor stacking context.

I don't see any way to get around that, other than to just hide the rest of the page.

[Adrienne Walker]

Created attachment 163952 [details]
Override position, not visibility

[Adrienne Walker]

(In reply to comment #9)
> Created an attachment (id=163952) [details]
> Override position, not visibility

After an irc conversation with jernoble, I updated this patch to just set position as mentioned in comment #4.  This is slightly less invasive (since Safari animates to the full screen element and changing styles drastically could cause problems) and also doesn't require updating all the fullscreen tests as well.

[James Robinson]

Comment on attachment 163952 [details]
Override position, not visibility

View in context: https://bugs.webkit.org/attachment.cgi?id=163952&action=review

> Source/WebCore/css/fullscreen.css:10
>  :-webkit-full-screen-ancestor:not(iframe) {

woah so we just clobber everything that forms a new stacking context? that's fucked up!

[WebKit Review Bot]

Comment on attachment 163952 [details]
Override position, not visibility

Clearing flags on attachment: 163952

Committed r128514: <http://trac.webkit.org/changeset/128514>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jer Noble]

(In reply to comment #11)
> (From update of attachment 163952 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163952&action=review
> 
> > Source/WebCore/css/fullscreen.css:10
> >  :-webkit-full-screen-ancestor:not(iframe) {
> 
> woah so we just clobber everything that forms a new stacking context? that's fucked up!

Indeed. :)

However, this hack will go away as a result of bug #84796, which will give a full screen element (and <dialog> elements) an explicit top-level stacking layer.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #14)
> (In reply to comment #11)
> > (From update of attachment 163952 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=163952&action=review
> > 
> > > Source/WebCore/css/fullscreen.css:10
> > >  :-webkit-full-screen-ancestor:not(iframe) {
> > 
> > woah so we just clobber everything that forms a new stacking context? that's fucked up!
> 
> Indeed. :)
> 
> However, this hack will go away as a result of bug #84796, which will give a full screen element (and <dialog> elements) an explicit top-level stacking layer.

The bug #84796, which you mentioned at the comment above, has been merged.
Is there any chance we can revert this patch?

Regarding your comment, we can roll this patch out..

[Jer Noble]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #11)
> > > (From update of attachment 163952 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=163952&action=review
> > > 
> > > > Source/WebCore/css/fullscreen.css:10
> > > >  :-webkit-full-screen-ancestor:not(iframe) {
> > > 
> > > woah so we just clobber everything that forms a new stacking context? that's fucked up!
> > 
> > Indeed. :)
> > 
> > However, this hack will go away as a result of bug #84796, which will give a full screen element (and <dialog> elements) an explicit top-level stacking layer.
> 
> The bug #84796, which you mentioned at the comment above, has been merged.
> Is there any chance we can revert this patch?
> 
> Regarding your comment, we can roll this patch out..

No, the contents of that patch are wrapped in a ENABLE flag.  Rolling out this patch would break ports which enable FULLSCREEN but not DIALOG_ELEMENT.