third_party/WebKit/LayoutTests/fullscreen/full-screen-stacking-context-expected.html - Issue 2107233002: Reland "Implement FullScreen using top layer."

Side by Side Diff: third_party/WebKit/LayoutTests/fullscreen/full-screen-stacking-context-expected.html

Issue 2107233002: Reland "Implement FullScreen using top layer." (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: restore !important to overflow:hidden Created 4 years, 5 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« no previous file with comments | « third_party/WebKit/LayoutTests/fullscreen/full-screen-stacking-context.html ('k') | third_party/WebKit/LayoutTests/fullscreen/full-screen-video-has-backdrop.html » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 <!DOCTYPE html>	1 <!DOCTYPE html>

2 <video></video>

3 <script>	2 <script>

4 var runPixelTests = true;	3 var runPixelTests = true;

	4

	5 function init() {

	6 waitForEventAndEnd(document, 'webkitfullscreenchange');

	7 runWithKeyDown(function() {

	8 document.getElementById('video').webkitRequestFullScreen();

	9 });

	10 }

5 </script>	11 </script>

6 <script src="full-screen-test.js"></script>	12 <script src="full-screen-test.js"></script>

7 <script>	13 <style>

8 document.onwebkitfullscreenchange = endTest;	14 #video {

9 runWithKeyDown(function() {	15 background-color: black;
	foolip 2016/07/06 13:19:43 This change seems to mask an actual regression in This change seems to mask an actual regression in the CL that I'm investigating. AFAICT, the ::backdrop element simply doesn't work for video elements, and also, it seems, iframes. Maybe others too. Because no backdrop is painted, instead one simply sees the page behind the (transparent) fullscreen element. dsinclair@, does this sound familiar at all, is it a problem you encountered? dsinclair 2016/07/06 13:32:31 This is the expected file, so this is what the cl Show quoted text On 2016/07/06 13:19:43, Philip Jägenstedt wrote: > This change seems to mask an actual regression in the CL that I'm investigating. > AFAICT, the ::backdrop element simply doesn't work for video elements, and also, > it seems, iframes. Maybe others too. Because no backdrop is painted, instead one > simply sees the page behind the (transparent) fullscreen element. > > dsinclair@, does this sound familiar at all, is it a problem you encountered? This is the expected file, so this is what the cl should look like when it's working. This shouldn't effect anything in the actual CL. foolip 2016/07/06 14:14:51 Right, but what I noticed when trying to simplify Show quoted text On 2016/07/06 13:32:31, dsinclair wrote: > On 2016/07/06 13:19:43, Philip Jägenstedt wrote: > > This change seems to mask an actual regression in the CL that I'm > investigating. > > AFAICT, the ::backdrop element simply doesn't work for video elements, and > also, > > it seems, iframes. Maybe others too. Because no backdrop is painted, instead > one > > simply sees the page behind the (transparent) fullscreen element. > > > > dsinclair@, does this sound familiar at all, is it a problem you encountered? > > > This is the expected file, so this is what the cl should look like when it's > working. This shouldn't effect anything in the actual CL. Right, but what I noticed when trying to simplify some test expectations is that if the background-color:black is removed from here, the test fails, but I think it shouldn't, because fullscreen.css has :not(:root):-webkit-full-screen::backdrop { background: black }. I'm in the debugger now and it looks the ::backdrop element isn't ever added to the top layer in Element::createPseudoElementIfNeeded when the fullscreen element is a video, iframe or img, so maybe it has something to do with replaced content. If this doesn't sound at all familiar I'll just keep investigating, just thought this might have been a deliberate workaround for the issue. dsinclair 2016/07/06 14:17:38 It wasn't deliberate. It sounds like there is an i Show quoted text On 2016/07/06 14:14:51, Philip Jägenstedt wrote: > On 2016/07/06 13:32:31, dsinclair wrote: > > On 2016/07/06 13:19:43, Philip Jägenstedt wrote: > > > This change seems to mask an actual regression in the CL that I'm > > investigating. > > > AFAICT, the ::backdrop element simply doesn't work for video elements, and > > also, > > > it seems, iframes. Maybe others too. Because no backdrop is painted, instead > > one > > > simply sees the page behind the (transparent) fullscreen element. > > > > > > dsinclair@, does this sound familiar at all, is it a problem you > encountered? > > > > > > This is the expected file, so this is what the cl should look like when it's > > working. This shouldn't effect anything in the actual CL. > > Right, but what I noticed when trying to simplify some test expectations is that > if the background-color:black is removed from here, the test fails, but I think > it shouldn't, because fullscreen.css has > :not(:root):-webkit-full-screen::backdrop { background: black }. > > I'm in the debugger now and it looks the ::backdrop element isn't ever added to > the top layer in Element::createPseudoElementIfNeeded when the fullscreen > element is a video, iframe or img, so maybe it has something to do with replaced > content. > > If this doesn't sound at all familiar I'll just keep investigating, just thought > this might have been a deliberate workaround for the issue. It wasn't deliberate. It sounds like there is an issue if the ::backdrop isn't getting applied. I was trying to match the expectation to what was produced and what was produced looked right to me. foolip 2016/07/06 15:20:27 I have a hard time deciding what would make most s Show quoted text On 2016/07/06 14:17:38, dsinclair wrote: > On 2016/07/06 14:14:51, Philip Jägenstedt wrote: > > On 2016/07/06 13:32:31, dsinclair wrote: > > > On 2016/07/06 13:19:43, Philip Jägenstedt wrote: > > > > This change seems to mask an actual regression in the CL that I'm > > > investigating. > > > > AFAICT, the ::backdrop element simply doesn't work for video elements, and > > > also, > > > > it seems, iframes. Maybe others too. Because no backdrop is painted, > instead > > > one > > > > simply sees the page behind the (transparent) fullscreen element. > > > > > > > > dsinclair@, does this sound familiar at all, is it a problem you > > encountered? > > > > > > > > > This is the expected file, so this is what the cl should look like when it's > > > working. This shouldn't effect anything in the actual CL. > > > > Right, but what I noticed when trying to simplify some test expectations is > that > > if the background-color:black is removed from here, the test fails, but I > think > > it shouldn't, because fullscreen.css has > > :not(:root):-webkit-full-screen::backdrop { background: black }. > > > > I'm in the debugger now and it looks the ::backdrop element isn't ever added > to > > the top layer in Element::createPseudoElementIfNeeded when the fullscreen > > element is a video, iframe or img, so maybe it has something to do with > replaced > > content. > > > > If this doesn't sound at all familiar I'll just keep investigating, just > thought > > this might have been a deliberate workaround for the issue. > > > It wasn't deliberate. It sounds like there is an issue if the ::backdrop isn't > getting applied. I was trying to match the expectation to what was produced and > what was produced looked right to me. I have a hard time deciding what would make most sense, but in any case the handling of background will change from :-webkit-full-screen { background-color: white; } video:-webkit-full-screen, audio:-webkit-full-screen { background-color: transparent !important; } to just :not(:root):-webkit-full-screen::backdrop { background: black; } So in a lot of cases it will go from white to black, and this is such a case. It's just that the ::backdrop pseudo doesn't actually work for video elements, if it did then the line I've commented on wouldn't be needed. Maybe I'll push back against the spec regarding the specific colors, but in any case ::backdrop needs to work. I was right to suspect replaced content, in StyleResolver::createPseudoElementIfNeeded it bails out because !canHaveGeneratedChildren(*parentLayoutObject), which happens for media elements, some input elements and all LayoutReplaced things. It can be "fixed" by just bypassing the check for PseudoIdBackdrop, but I doubt that's sensible. I'll create a separate review for that I think.
10 document.querySelector("video").webkitRequestFullScreen();	16 }

11 });	17 </style>

12 </script>	18 <body onload="init()">

	19 <video id='video'></video>

	20 </body>

OLD	NEW