Hide all ancestors of the full screen element when going full screen
Created attachment 163228 [details] Patch
See: http://code.google.com/p/chromium/issues/detail?id=145562. Test case forthcoming.
Created attachment 163419 [details] Patch
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.
(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.
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
(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?
(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.
Created attachment 163952 [details] Override position, not visibility
(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.
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!
Comment on attachment 163952 [details] Override position, not visibility Clearing flags on attachment: 163952 Committed r128514: <http://trac.webkit.org/changeset/128514>
All reviewed patches have been landed. Closing bug.
(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.
(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..
(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.