Issue 974783002: Match the Fullscreen spec's CSS as far as currently practical

Issue 974783002: Match the Fullscreen spec's CSS as far as currently practical (Closed)

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.

More Reviews

Description

Match 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

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+199 lines, -50 lines)			Patch
M	LayoutTests/fullscreen/enter-exit-full-screen-hover.html	View		2 chunks	+9 lines, -1 line	0 comments	Download
M	LayoutTests/fullscreen/full-screen-iframe-zIndex.html	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/fullscreen/full-screen-iframe-zIndex-expected.html	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	LayoutTests/fullscreen/full-screen-render-inline-expected.html	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	LayoutTests/fullscreen/full-screen-zIndex-expected.html	View		1 chunk	+5 lines, -6 lines	0 comments	Download
M	LayoutTests/fullscreen/parent-flow-inline-with-block-child-expected.html	View		1 chunk	+2 lines, -2 lines	0 comments	Download
A	LayoutTests/fullscreen/rendering/ua-style-iframe-border.html	View	1 2 3	1 chunk	+25 lines, -0 lines	0 comments	Download
A	LayoutTests/fullscreen/rendering/ua-style-override.html	View	1 2 3 4	1 chunk	+59 lines, -0 lines	0 comments	Download
A	LayoutTests/fullscreen/rendering/ua-style-root.html	View	1 2 3 4	1 chunk	+49 lines, -0 lines	0 comments	Download
M	Source/core/css/fullscreen.css	View	1 2 3	2 chunks	+47 lines, -38 lines	0 comments	Download

Messages

Total messages: 17 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Julien - ping for review

https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/full-screen-render-inline-expected.html File LayoutTests/fullscreen/full-screen-render-inline-expected.html (left): https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/full-screen-render-inline-expected.html#oldcode2 LayoutTests/fullscreen/full-screen-render-inline-expected.html:2: <!-- FIXME: the <br> is needed until http://crbug.com/246077 is ...

5 years, 9 months ago (2015-03-04 01:07:38 UTC) #4

philipj_slow

Thanks for the detailed feedback, Julien! There are a few issues left to discuss in ...

5 years, 9 months ago (2015-03-04 07:15:29 UTC) #6

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.

philipj_slow

https://codereview.chromium.org/974783002/diff/40001/Source/core/css/fullscreen.css File Source/core/css/fullscreen.css (right): https://codereview.chromium.org/974783002/diff/40001/Source/core/css/fullscreen.css#newcode27 Source/core/css/fullscreen.css:27: border: none; This is a change from before, it ...

5 years, 9 months ago (2015-03-04 07:20:51 UTC) #7

Julien - ping for review

https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/rendering/style-defaults-iframe.html File LayoutTests/fullscreen/rendering/style-defaults-iframe.html (right): https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/rendering/style-defaults-iframe.html#newcode2 LayoutTests/fullscreen/rendering/style-defaults-iframe.html:2: <title>User-agent level style sheet defaults for an iframe</title> On ...

5 years, 9 months ago (2015-03-04 17:50:23 UTC) #8

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!

philipj_slow

Thanks again Julien, I hope you can sift through the back-and-forth changes and that everything ...

5 years, 9 months ago (2015-03-05 04:05:06 UTC) #10

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.

Julien - ping for review

lgtm https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/rendering/style-defaults-iframe.html File LayoutTests/fullscreen/rendering/style-defaults-iframe.html (right): https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/rendering/style-defaults-iframe.html#newcode2 LayoutTests/fullscreen/rendering/style-defaults-iframe.html:2: <title>User-agent level style sheet defaults for an iframe</title> ...

5 years, 9 months ago (2015-03-05 18:19:03 UTC) #11

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.

philipj_slow

Thanks Julien, landing now and bracing for regressions :) https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/rendering/style-defaults-iframe.html File LayoutTests/fullscreen/rendering/style-defaults-iframe.html (right): https://codereview.chromium.org/974783002/diff/20001/LayoutTests/fullscreen/rendering/style-defaults-iframe.html#newcode2 LayoutTests/fullscreen/rendering/style-defaults-iframe.html:2: ...

5 years, 9 months ago (2015-03-06 02:43:40 UTC) #13

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.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/974783002/80001

5 years, 9 months ago (2015-03-06 02:44:18 UTC) #16

commit-bot: I haz the power

5 years, 9 months ago (2015-03-06 04:20:06 UTC) #17

Message was sent while issue was closed.

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191409

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages