Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 9 months ago by philipj_slow Modified:
5 years, 9 months ago Reviewers:
falken, Julien - ping for review CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, dglazkov+blink, ed+blinkwatch_opera.com, rwlbuis Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionMatch the Fullscreen spec's CSS as far as currently practical
This change was prompted by the "Fullscreen API bug fixes" blink-dev
thread started by Ali Alabbas at Microsoft:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/f-V2GWatXkA
This is very similar to, and borrows from, the in-progress review to
move Fullscreen to the top layer system:
https://codereview.chromium.org/788073004/
With this change, any fullscreen element (except :root) will have
style applied that makes match the screen size, where previously this
was only done for iframe and video. This will cause a re-layout of
those elements when entering and exiting fullscreen, with the side
effect that the full-screen-render-inline.html test now works. While
the bug does not reproduce now, the underlying cause has not been
fixed, the move to top layer is probably the best cure.
iframe:-webkit-full-screen { border: none; } was previously
!important, but isn't in the spec. This is the cause of the changed
test expectations for full-screen-iframe-zIndex.html.
The flex:1 and display:block rules for audio and video originate from
a Vimeo fullscreen bug: https://bugs.webkit.org/show_bug.cgi?id=58291
With the size and position of the fullscreen element forced to match
the viewport by the UA style sheet, neither of these rules should have
any observable effect, so they are removed.
BUG=402378, 246077
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191409
Patch Set 1 #Patch Set 2 : tests #
Total comments: 29
Patch Set 3 : address feedback #
Total comments: 8
Patch Set 4 : drop flex rule and tweak tests #
Total comments: 2
Patch Set 5 : disambiguate Fullscreen UA style sheet #
Created: 5 years, 9 months ago
Messages
Total messages: 17 (3 generated)
tests
philipj@opera.com changed reviewers: + falken@chromium.org, jchaffraix@chromium.org
PTAL
https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/f... File LayoutTests/fullscreen/full-screen-render-inline-expected.html (left): https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/f... LayoutTests/fullscreen/full-screen-render-inline-expected.html:2: <!-- FIXME: the <br> is needed until http://crbug.com/246077 is fixed --> The description should mention this bug as it seems solved now. https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... File LayoutTests/fullscreen/rendering/style-defaults-iframe.html (right): https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/style-defaults-iframe.html:2: <title>User-agent level style sheet defaults for an iframe</title> Using <title> is useless in test. If it is meaningful, it should be dumped in the final output. If not, it should be removed. Also the description doesn't match what you're trying to do. You're not testing the default at all, you're making sure they can't be overridden. https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/style-defaults-iframe.html:47: assert_equals(style.height, window.innerHeight + "px"); width and height are not overriden https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/style-defaults-iframe.html:49: assert_equals(style.flex, "0 0 auto"); Nor is flex https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/style-defaults-iframe.html:50: assert_equals(style.transform, "none"); Nor transform. https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... File LayoutTests/fullscreen/rendering/style-defaults-root.html (right): https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/style-defaults-root.html:27: // No properties should match the UA style sheet. I have no clue what this test is trying to do, nor why the output is even correct. https://codereview.chromium.org/974783002/diff/20001/Source/core/css/fullscre... File Source/core/css/fullscreen.css (left): https://codereview.chromium.org/974783002/diff/20001/Source/core/css/fullscre... Source/core/css/fullscreen.css:2: background-color: white; I don't agree with this change nor the justification above. You're making us behave differently for no compelling reasons and it's better to keep our old behavior until we can land the top layer bits. We are already taking a compatibility risk by landing this change so I would rather stick to the minimal amount of diff. The change in LayoutTests/fullscreen/resources/empty.html underlines this risk. https://codereview.chromium.org/974783002/diff/20001/Source/core/css/fullscre... File Source/core/css/fullscreen.css (right): https://codereview.chromium.org/974783002/diff/20001/Source/core/css/fullscre... Source/core/css/fullscreen.css:6: * :fullscreen. crbug.com/402378 That's not a really actionable (btw the rest of the comment is great). https://codereview.chromium.org/974783002/diff/20001/Source/core/css/fullscre... Source/core/css/fullscreen.css:37: * https://crbug.com/356282 The (apparently) semi-random selection of bugs you've picked up confused me. It would be better to pick one or 2 that *are* relevant and we don't have to dig through a lot of history. It doesn't seem to justify the flex change (see below). https://codereview.chromium.org/974783002/diff/20001/Source/core/css/fullscre... Source/core/css/fullscreen.css:40: flex: none !important; I would have liked a focused explanation on why it's correct to convert flex: 1 !important to flex: none !important (it doesn't have to be here but can be done in the description) https://codereview.chromium.org/974783002/diff/20001/Source/core/css/fullscre... Source/core/css/fullscreen.css:43: I would love to keep the separation between spec and our implementation.
address feedback
Thanks for the detailed feedback, Julien! There are a few issues left to discuss in the comments. https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/f... File LayoutTests/fullscreen/full-screen-render-inline-expected.html (left): https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/f... LayoutTests/fullscreen/full-screen-render-inline-expected.html:2: <!-- FIXME: the <br> is needed until http://crbug.com/246077 is fixed --> On 2015/03/04 01:07:37, Julien Chaffraix - PST wrote: > The description should mention this bug as it seems solved now. I've added what I think is the explanation to the description. https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... File LayoutTests/fullscreen/rendering/style-defaults-iframe.html (right): https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/style-defaults-iframe.html:2: <title>User-agent level style sheet defaults for an iframe</title> On 2015/03/04 01:07:37, Julien Chaffraix - PST wrote: > Using <title> is useless in test. If it is meaningful, it should be dumped in > the final output. If not, it should be removed. With testharness.js the <title> becomes the test title, equivalent to putting it after the function in async_test. It is included in the test expectations ("PASS User-agent level style sheet defaults for an iframe") but those don't need to be checked in for all-passing testharness.js tests. > Also the description doesn't match what you're trying to do. You're not testing > the default at all, you're making sure they can't be overridden. I've renamed the tests to ua-style-*.html, does that work? I think it makes sense to test the defaults and overriding them in the same test, because trying to override them is how we verify that !important is included. https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/style-defaults-iframe.html:47: assert_equals(style.height, window.innerHeight + "px"); On 2015/03/04 01:07:37, Julien Chaffraix - PST wrote: > width and height are not overriden I removed them when I noticed it doesn't make any difference, which I think is because I'm testing the computed style and not the specified style I'd really like to test. I've added them back. https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/style-defaults-iframe.html:49: assert_equals(style.flex, "0 0 auto"); On 2015/03/04 01:07:37, Julien Chaffraix - PST wrote: > Nor is flex Done. https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/style-defaults-iframe.html:50: assert_equals(style.transform, "none"); On 2015/03/04 01:07:37, Julien Chaffraix - PST wrote: > Nor transform. Done. https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... File LayoutTests/fullscreen/rendering/style-defaults-root.html (right): https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/style-defaults-root.html:27: // No properties should match the UA style sheet. On 2015/03/04 01:07:37, Julien Chaffraix - PST wrote: > I have no clue what this test is trying to do, nor why the output is even > correct. It's to verify that the :not(:root) bit is in the UA style sheet, as it previously wasn't. Too paranoid? https://codereview.chromium.org/974783002/diff/20001/Source/core/css/fullscre... File Source/core/css/fullscreen.css (left): https://codereview.chromium.org/974783002/diff/20001/Source/core/css/fullscre... Source/core/css/fullscreen.css:2: background-color: white; On 2015/03/04 01:07:38, Julien Chaffraix - PST wrote: > I don't agree with this change nor the justification above. You're making us > behave differently for no compelling reasons and it's better to keep our old > behavior until we can land the top layer bits. We are already taking a > compatibility risk by landing this change so I would rather stick to the minimal > amount of diff. > > The change in LayoutTests/fullscreen/resources/empty.html underlines this risk. OK, I'll revert the background changes. https://codereview.chromium.org/974783002/diff/20001/Source/core/css/fullscre... File Source/core/css/fullscreen.css (right): https://codereview.chromium.org/974783002/diff/20001/Source/core/css/fullscre... Source/core/css/fullscreen.css:6: * :fullscreen. crbug.com/402378 On 2015/03/04 01:07:38, Julien Chaffraix - PST wrote: > That's not a really actionable (btw the rest of the comment is great). I'll convert everything to FIXMEs instead. https://codereview.chromium.org/974783002/diff/20001/Source/core/css/fullscre... Source/core/css/fullscreen.css:37: * https://crbug.com/356282 On 2015/03/04 01:07:38, Julien Chaffraix - PST wrote: > The (apparently) semi-random selection of bugs you've picked up confused me. It > would be better to pick one or 2 that *are* relevant and we don't have to dig > through a lot of history. > > It doesn't seem to justify the flex change (see below). These were the bugs references when adding/tweaking transform and flex to this stylesheet, but I guess inlining git blame isn't that helpful. I'm going to leave just the spec bug here since there isn't actually something to fix here except wait for the spec bug. https://codereview.chromium.org/974783002/diff/20001/Source/core/css/fullscre... Source/core/css/fullscreen.css:40: flex: none !important; On 2015/03/04 01:07:38, Julien Chaffraix - PST wrote: > I would have liked a focused explanation on why it's correct to convert flex: 1 > !important to flex: none !important (it doesn't have to be here but can be done > in the description) Sorry, I should have declared my ignorance on this up front. In the spec bug you can see that I don't really know the difference, and I couldn't get anyone else to clue me in. It was removed in your review so I figured you didn't find a bug that required it. It seems to me that per spec, the size of the fullscreen element is fixed in every way possible, there's no available space to fill into. If the rule is removed, full-screen-with-flex-item.html still passes. Do you know of any situation where it would matter? If not, I'll ask that it isn't added to the spec and remove it here instead. https://codereview.chromium.org/974783002/diff/20001/Source/core/css/fullscre... Source/core/css/fullscreen.css:43: On 2015/03/04 01:07:38, Julien Chaffraix - PST wrote: > I would love to keep the separation between spec and our implementation. Done.
https://codereview.chromium.org/974783002/diff/40001/Source/core/css/fullscre... File Source/core/css/fullscreen.css (right): https://codereview.chromium.org/974783002/diff/40001/Source/core/css/fullscre... Source/core/css/fullscreen.css:27: border: none; This is a change from before, it used to be !important. The change is visible in the full-screen-iframe-zIndex.html test. I'll note this in the description.
https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... File LayoutTests/fullscreen/rendering/style-defaults-iframe.html (right): https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/style-defaults-iframe.html:2: <title>User-agent level style sheet defaults for an iframe</title> On 2015/03/04 07:15:28, philipj_UTC7 wrote: > On 2015/03/04 01:07:37, Julien Chaffraix - PST wrote: > > Using <title> is useless in test. If it is meaningful, it should be dumped in > > the final output. If not, it should be removed. > > With testharness.js the <title> becomes the test title, equivalent to putting it > after the function in async_test. It is included in the test expectations ("PASS > User-agent level style sheet defaults for an iframe") but those don't need to be > checked in for all-passing testharness.js tests. Can you include the output of these tests? I think I understand what you said but I would like to see the output for myself before making up my mind. (Also I would argue it's a bad decision from the W3C harness to have this magical behavior for <title> but that's another debate). > > Also the description doesn't match what you're trying to do. You're not > testing > > the default at all, you're making sure they can't be overridden. > > I've renamed the tests to ua-style-*.html, does that work? I think it makes > sense to test the defaults and overriding them in the same test, because trying > to override them is how we verify that !important is included. I didn't have a gripe with the previous naming but the new name is better indeed. I meant that the content of <title> doesn't match what you're doing. You're not testing the defaults, you're testing that they cannot be overridden. Those are 2 different tests and the description matches the former when you're actually testing the latter. https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... File LayoutTests/fullscreen/rendering/style-defaults-root.html (right): https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/style-defaults-root.html:27: // No properties should match the UA style sheet. On 2015/03/04 at 07:15:28, philipj_UTC7 wrote: > On 2015/03/04 01:07:37, Julien Chaffraix - PST wrote: > > I have no clue what this test is trying to do, nor why the output is even > > correct. > > It's to verify that the :not(:root) bit is in the UA style sheet, as it previously wasn't. Too paranoid? Probably. I don't have a strong opinion on that apart that the description of the test should explain clearly what is tested. https://codereview.chromium.org/974783002/diff/20001/Source/core/css/fullscre... File Source/core/css/fullscreen.css (right): https://codereview.chromium.org/974783002/diff/20001/Source/core/css/fullscre... Source/core/css/fullscreen.css:40: flex: none !important; On 2015/03/04 at 07:15:28, philipj_UTC7 wrote: > On 2015/03/04 01:07:38, Julien Chaffraix - PST wrote: > > I would have liked a focused explanation on why it's correct to convert flex: 1 > > !important to flex: none !important (it doesn't have to be here but can be done > > in the description) > > Sorry, I should have declared my ignorance on this up front. In the spec bug you can see that I don't really know the difference, and I couldn't get anyone else to clue me in. It was removed in your review so I figured you didn't find a bug that required it. > > It seems to me that per spec, the size of the fullscreen element is fixed in every way possible, there's no available space to fill into. If the rule is removed, full-screen-with-flex-item.html still passes. Do you know of any situation where it would matter? If not, I'll ask that it isn't added to the spec and remove it here instead. It was probably not needed before actually but we were conservative as we wrap the fullscreen layout object in a flexbox. https://codereview.chromium.org/974783002/diff/40001/LayoutTests/fullscreen/r... File LayoutTests/fullscreen/rendering/ua-style-iframe.html (right): https://codereview.chromium.org/974783002/diff/40001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/ua-style-iframe.html:55: assert_equals(style.height, window.innerHeight + "px"); You forgot to remove these 2 per your comment on patch #2. https://codereview.chromium.org/974783002/diff/40001/LayoutTests/fullscreen/r... File LayoutTests/fullscreen/rendering/ua-style-root.html (right): https://codereview.chromium.org/974783002/diff/40001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/ua-style-root.html:10: * which would otherwise (incidentally) match the UA style sheet. */ This should be in the dumped description (same gripe as using <title> if it's important to understand the test it should be dumped). https://codereview.chromium.org/974783002/diff/40001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/ua-style-root.html:10: * which would otherwise (incidentally) match the UA style sheet. */ This should be in the dumped description (same gripe as using <title> if it's important to understand the test it should be dumped). https://codereview.chromium.org/974783002/diff/40001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/ua-style-root.html:42: assert_equals(style.height, window.innerHeight / 2 + "px"); You forgot to remove these 2 per your comment on patch #2. https://codereview.chromium.org/974783002/diff/40001/Source/core/css/fullscre... File Source/core/css/fullscreen.css (right): https://codereview.chromium.org/974783002/diff/40001/Source/core/css/fullscre... Source/core/css/fullscreen.css:27: border: none; On 2015/03/04 at 07:20:50, philipj_UTC7 wrote: > This is a change from before, it used to be !important. The change is visible in the full-screen-iframe-zIndex.html test. I'll note this in the description. ACK!
drop flex rule and tweak tests
Thanks again Julien, I hope you can sift through the back-and-forth changes and that everything is in order now, or soon :) https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... File LayoutTests/fullscreen/rendering/style-defaults-iframe.html (right): https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/style-defaults-iframe.html:2: <title>User-agent level style sheet defaults for an iframe</title> On 2015/03/04 17:50:22, Julien Chaffraix - PST wrote: > On 2015/03/04 07:15:28, philipj_UTC7 wrote: > > On 2015/03/04 01:07:37, Julien Chaffraix - PST wrote: > > > Using <title> is useless in test. If it is meaningful, it should be dumped > in > > > the final output. If not, it should be removed. > > > > With testharness.js the <title> becomes the test title, equivalent to putting > it > > after the function in async_test. It is included in the test expectations > ("PASS > > User-agent level style sheet defaults for an iframe") but those don't need to > be > > checked in for all-passing testharness.js tests. > > Can you include the output of these tests? I think I understand what you said > but I would like to see the output for myself before making up my mind. There's a hook to prevent all-passing testharness.js expectations from being checked in, but see below for what they look like after some more renaming. > (Also I would argue it's a bad decision from the W3C harness to have this > magical behavior for <title> but that's another debate). > > > > Also the description doesn't match what you're trying to do. You're not > > testing > > > the default at all, you're making sure they can't be overridden. > > > > I've renamed the tests to ua-style-*.html, does that work? I think it makes > > sense to test the defaults and overriding them in the same test, because > trying > > to override them is how we verify that !important is included. > > I didn't have a gripe with the previous naming but the new name is better > indeed. > > I meant that the content of <title> doesn't match what you're doing. You're not > testing the defaults, you're testing that they cannot be overridden. Those are 2 > different tests and the description matches the former when you're actually > testing the latter. OK, just name of the section in the spec wasn't clear. It was hard to come up with a title for the override+border test, so I've split those. The three tests now have these test expectations: This is a testharness.js-based test. PASS Fullscreen UA style sheet removes iframe border Harness: the test ran to completion. This is a testharness.js-based test. PASS Fullscreen UA style sheet cannot be overriden by author style Harness: the test ran to completion. This is a testharness.js-based test. PASS Fullscreen UA style sheet does not apply to the :root element Harness: the test ran to completion. https://codereview.chromium.org/974783002/diff/20001/Source/core/css/fullscre... File Source/core/css/fullscreen.css (right): https://codereview.chromium.org/974783002/diff/20001/Source/core/css/fullscre... Source/core/css/fullscreen.css:40: flex: none !important; On 2015/03/04 17:50:22, Julien Chaffraix - PST wrote: > On 2015/03/04 at 07:15:28, philipj_UTC7 wrote: > > On 2015/03/04 01:07:38, Julien Chaffraix - PST wrote: > > > I would have liked a focused explanation on why it's correct to convert > flex: 1 > > > !important to flex: none !important (it doesn't have to be here but can be > done > > > in the description) > > > > Sorry, I should have declared my ignorance on this up front. In the spec bug > you can see that I don't really know the difference, and I couldn't get anyone > else to clue me in. It was removed in your review so I figured you didn't find a > bug that required it. > > > > It seems to me that per spec, the size of the fullscreen element is fixed in > every way possible, there's no available space to fill into. If the rule is > removed, full-screen-with-flex-item.html still passes. Do you know of any > situation where it would matter? If not, I'll ask that it isn't added to the > spec and remove it here instead. > > It was probably not needed before actually but we were conservative as we wrap > the fullscreen layout object in a flexbox. I've played around a bit with flexbox and the Fullscreen UA style sheet rules and have satisfied myself that nothing you do with the flex property will affect the size or position of the fullscreen element. I've updated the description, removed flex from the stylesheet, and will comment on the spec bug. https://codereview.chromium.org/974783002/diff/40001/LayoutTests/fullscreen/r... File LayoutTests/fullscreen/rendering/ua-style-iframe.html (right): https://codereview.chromium.org/974783002/diff/40001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/ua-style-iframe.html:55: assert_equals(style.height, window.innerHeight + "px"); On 2015/03/04 17:50:22, Julien Chaffraix - PST wrote: > You forgot to remove these 2 per your comment on patch #2. I meant that I added back with width and height overrides above and kept these lines. https://codereview.chromium.org/974783002/diff/40001/LayoutTests/fullscreen/r... File LayoutTests/fullscreen/rendering/ua-style-root.html (right): https://codereview.chromium.org/974783002/diff/40001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/ua-style-root.html:10: * which would otherwise (incidentally) match the UA style sheet. */ On 2015/03/04 17:50:23, Julien Chaffraix - PST wrote: > This should be in the dumped description (same gripe as using <title> if it's > important to understand the test it should be dumped). New test title is "Fullscreen UA style sheet does not apply to the :root element" which I think captures the purpose of the test.
lgtm https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... File LayoutTests/fullscreen/rendering/style-defaults-iframe.html (right): https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/style-defaults-iframe.html:2: <title>User-agent level style sheet defaults for an iframe</title> On 2015/03/05 04:05:06, philipj_UTC7 wrote: > On 2015/03/04 17:50:22, Julien Chaffraix - PST wrote: > > On 2015/03/04 07:15:28, philipj_UTC7 wrote: > > > On 2015/03/04 01:07:37, Julien Chaffraix - PST wrote: > > > > Using <title> is useless in test. If it is meaningful, it should be dumped > > in > > > > the final output. If not, it should be removed. > > > > > > With testharness.js the <title> becomes the test title, equivalent to > putting > > it > > > after the function in async_test. It is included in the test expectations > > ("PASS > > > User-agent level style sheet defaults for an iframe") but those don't need > to > > be > > > checked in for all-passing testharness.js tests. > > > > Can you include the output of these tests? I think I understand what you said > > but I would like to see the output for myself before making up my mind. > > There's a hook to prevent all-passing testharness.js expectations from being > checked in, but see below for what they look like after some more renaming. > > > (Also I would argue it's a bad decision from the W3C harness to have this > > magical behavior for <title> but that's another debate). > > > > > > Also the description doesn't match what you're trying to do. You're not > > > testing > > > > the default at all, you're making sure they can't be overridden. > > > > > > I've renamed the tests to ua-style-*.html, does that work? I think it makes > > > sense to test the defaults and overriding them in the same test, because > > trying > > > to override them is how we verify that !important is included. > > > > I didn't have a gripe with the previous naming but the new name is better > > indeed. > > > > I meant that the content of <title> doesn't match what you're doing. You're > not > > testing the defaults, you're testing that they cannot be overridden. Those are > 2 > > different tests and the description matches the former when you're actually > > testing the latter. > > OK, just name of the section in the spec wasn't clear. It was hard to come up > with a title for the override+border test, so I've split those. The three tests > now have these test expectations: > > This is a testharness.js-based test. > PASS Fullscreen UA style sheet removes iframe border > Harness: the test ran to completion. > > This is a testharness.js-based test. > PASS Fullscreen UA style sheet cannot be overriden by author style > Harness: the test ran to completion. > > This is a testharness.js-based test. > PASS Fullscreen UA style sheet does not apply to the :root element > Harness: the test ran to completion. OK, I think that's OK. It's kinda weird to dump the description after PASS. https://codereview.chromium.org/974783002/diff/60001/LayoutTests/fullscreen/r... File LayoutTests/fullscreen/rendering/ua-style-root.html (right): https://codereview.chromium.org/974783002/diff/60001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/ua-style-root.html:29: // No properties should match the UA style sheet. You probably meant the UA fullscreen style sheet. I think these comments should be in the test output as they are giving more details about the condition for passing.
disambiguate Fullscreen UA style sheet
Thanks Julien, landing now and bracing for regressions :) https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... File LayoutTests/fullscreen/rendering/style-defaults-iframe.html (right): https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/style-defaults-iframe.html:2: <title>User-agent level style sheet defaults for an iframe</title> On 2015/03/05 18:19:03, Julien Chaffraix - PST wrote: > On 2015/03/05 04:05:06, philipj_UTC7 wrote: > > On 2015/03/04 17:50:22, Julien Chaffraix - PST wrote: > > > On 2015/03/04 07:15:28, philipj_UTC7 wrote: > > > > On 2015/03/04 01:07:37, Julien Chaffraix - PST wrote: > > > > > Using <title> is useless in test. If it is meaningful, it should be > dumped > > > in > > > > > the final output. If not, it should be removed. > > > > > > > > With testharness.js the <title> becomes the test title, equivalent to > > putting > > > it > > > > after the function in async_test. It is included in the test expectations > > > ("PASS > > > > User-agent level style sheet defaults for an iframe") but those don't need > > to > > > be > > > > checked in for all-passing testharness.js tests. > > > > > > Can you include the output of these tests? I think I understand what you > said > > > but I would like to see the output for myself before making up my mind. > > > > There's a hook to prevent all-passing testharness.js expectations from being > > checked in, but see below for what they look like after some more renaming. > > > > > (Also I would argue it's a bad decision from the W3C harness to have this > > > magical behavior for <title> but that's another debate). > > > > > > > > Also the description doesn't match what you're trying to do. You're not > > > > testing > > > > > the default at all, you're making sure they can't be overridden. > > > > > > > > I've renamed the tests to ua-style-*.html, does that work? I think it > makes > > > > sense to test the defaults and overriding them in the same test, because > > > trying > > > > to override them is how we verify that !important is included. > > > > > > I didn't have a gripe with the previous naming but the new name is better > > > indeed. > > > > > > I meant that the content of <title> doesn't match what you're doing. You're > > not > > > testing the defaults, you're testing that they cannot be overridden. Those > are > > 2 > > > different tests and the description matches the former when you're actually > > > testing the latter. > > > > OK, just name of the section in the spec wasn't clear. It was hard to come up > > with a title for the override+border test, so I've split those. The three > tests > > now have these test expectations: > > > > This is a testharness.js-based test. > > PASS Fullscreen UA style sheet removes iframe border > > Harness: the test ran to completion. > > > > This is a testharness.js-based test. > > PASS Fullscreen UA style sheet cannot be overriden by author style > > Harness: the test ran to completion. > > > > This is a testharness.js-based test. > > PASS Fullscreen UA style sheet does not apply to the :root element > > Harness: the test ran to completion. > > OK, I think that's OK. It's kinda weird to dump the description after PASS. This is because it's possible to have multiple tests in a single file, and in that case you get one PASS (hopefully) with the test title on each line. https://codereview.chromium.org/974783002/diff/60001/LayoutTests/fullscreen/r... File LayoutTests/fullscreen/rendering/ua-style-root.html (right): https://codereview.chromium.org/974783002/diff/60001/LayoutTests/fullscreen/r... LayoutTests/fullscreen/rendering/ua-style-root.html:29: // No properties should match the UA style sheet. On 2015/03/05 18:19:03, Julien Chaffraix - PST wrote: > You probably meant the UA fullscreen style sheet. I've disambiguated "UA style sheet" in all of the tests now to be clear. > I think these comments should be in the test output as they are giving more > details about the condition for passing. There's a bit of a difference in convention between testharness.js and our dumpAsText() tests, in that the test expectations aren't really part of a testharness.js test, and looking at it for a failing test will only tell you which assertion has failed. This has its downsides, like a failure sometimes being harder to pinpoint. But as you can see I still prefer testharness.js on balance, mostly because it makes it easier to share the test suite with web-platform-tests.
The CQ bit was checked by philipj@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from jchaffraix@chromium.org Link to the patchset: https://codereview.chromium.org/974783002/#ps80001 (title: "disambiguate Fullscreen UA style sheet")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/974783002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=191409 |
