convert css3-background / css3-values tests to CSSWG format

ASSIGNED
Assigned to

Status

()

Core
Layout: Misc Code
ASSIGNED
5 years ago
4 years ago

People

(Reporter: fantasai, Assigned: Gérard Talbot)

Tracking

(Depends on: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
This bug is to track patches for exporting the css3-background and css3-values tests to the CSSWG repository. See http://wiki.csswg.org/test/css2.1/format and bug 691950 for background info.
(Assignee)

Comment 1

5 years ago
Created attachment 706818 [details]
patch-values-and-backgrounds.diff is a patch for /css-valuesandunits/ tests and /backgrounds/ tests


/css-valuesandunits/ tests
*************************

2 mozilla-central/layout/reftests/css-valuesandunits/ tests which should
*NOT* be shifted over

http://mxr.mozilla.org/mozilla-central/source/layout/reftests/css-valuesandunits/unit-vh-vw-zoom.html

http://mxr.mozilla.org/mozilla-central/source/layout/reftests/css-valuesandunits/unit-vh-vw-zoom-ref.html


Same list of 2 tests (which should *not* be shifted over) but as filenames only:

unit-vh-vw-zoom.html
unit-vh-vw-zoom-ref.html


Still in /css-valuesandunits/ tests, 5 tests use unneedlessly a red border-bottom :
unit-rem-div-fontsize.html
unit-rem-div-width-inner.html
unit-rem-div-width-outer.html
unit-rem-root-width.html
unit-rem-ref.html


--------------

/backgrounds/ tests
*******************

List of 27 URLs of filenames in lexicographical order which should *not*
be shifted over (images not included):
-----------------------------------

http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/background-moz-default-background-color-ref.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/background-moz-default-background-color.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/background-redraw-237766-ref.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/background-redraw-237766.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/background-referrer-ref.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/background-referrer.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/background-referrer.sjs
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/background-size-continuous.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/background-size-cover-bounding-box.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/background-size-cover-continuous.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/background-size-cover-each-box.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/background-size-each-box.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/background-size-zoom-no-repeat-ref.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/background-size-zoom-no-repeat.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/background-size-zoom-repeat-ref.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/background-size-zoom-repeat.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/body-background-ref.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/body-background.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/delay-image-response.sjs
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/div-background-ref.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/div-background.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/really-big-background-ref.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/really-big-background.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/root-background-1.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/root-background-ref.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/root-element-display-none-1.html
http://mxr.mozilla.org/mozilla-central/layout/reftests/backgrounds/root-element-display-none-ref.html


Same list of 27 tests (which should *not* be shifted over) but as filenames only:
-----------------------------------------------------------------------
 background-moz-default-background-color-ref.html
 background-moz-default-background-color.html
 background-redraw-237766-ref.html
 background-redraw-237766.html
 background-referrer-ref.html
 background-referrer.html
 background-referrer.sjs
 background-size-continuous.html
 background-size-cover-bounding-box.html
 background-size-cover-continuous.html
 background-size-cover-each-box.html
 background-size-each-box.html
 background-size-zoom-no-repeat-ref.html
 background-size-zoom-no-repeat.html
 background-size-zoom-repeat-ref.html
 background-size-zoom-repeat.html
 body-background-ref.html
 body-background.html
 delay-image-response.sjs
 div-background-ref.html
 div-background.html
 really-big-background-ref.html
 really-big-background.html
 root-background-1.html
 root-background-ref.html
 root-element-display-none-1.html
 root-element-display-none-ref.html


12 tests which would better replace red color with a more neutral color:
------------------------------------------------------------------------

10 tests from Timothy Nikkel with the substring "translucent" in their
filenames use red color:
translucent-color-1.html
translucent-color-2.html
translucent-color-3.html
translucent-color-ref.html
iframe-translucent-color-1.html
iframe-translucent-color-ref.html
viewport-translucent-color-1.html
viewport-translucent-color-2.html
viewport-translucent-color-3.html
viewport-translucent-color-ref.html
<!--
"Red Means Failure
    Don't use the color red other than to indicate a failure."
http://wiki.csswg.org/test/format#design-requirements
red color is pre-considered as a test failure;
best would be to replace red with another
more neutral color.
Gérard Talbot
-->
was added to those tests
and 2 tests from Kyle Huey use red color:
table-background.html
table-background-ref.html
also were added a similar <!-- comment -->

Finally, Boris Zbarsky added 2 reftests (January 19th) 5 days after I cloned (January 14th) locally the repository. So, this will need to be adjusted manually.

 	continuous-inline-1ab-ref.html 	440 	Jan 19 20:01 	 
	 
	continuous-inline-1cd-ref.html 	460 	Jan 19 20:01 	  

while my patch only has, only mentions continuous-inline-1-ref.html
(Assignee)

Comment 2

5 years ago
Note that Jeff Walden's SVG tests which are in

http://mxr.mozilla.org/mozilla-central/source/layout/reftests/backgrounds/vector/

have already been copied to, submitted in

http://test.csswg.org/source/contributors/mozilla/submitted/css3-background/background-size/vector/

and a long + multi-steps process to convert Jeff Walden's SVG tests to CSSWG format has started.

So, strictly speaking, this task by itself is more or less independent of this bug report and bug 795022.

--------------

What more/else needs to be done in /backgrounds/ tests and /css-valuesandunits/ tests to fully complete the CSSWG conversion?

A) replace red color in the beforementioned tests with a neutral, appropriate color: blue or orange should be okay.

B) merge div#outer declarations with div#inner declarations into 1 single div; this will bring many benefits to tests: 
fewer declarations, fewer rules, fewer elements involved, cleaner and clearer tests.
Attachment #706818 - Flags: feedback?(dbaron)
(Assignee)

Comment 3

5 years ago
One observation in my notes I forgot to mention with regards to background tests.

Test is:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/backgrounds/background-size-body-cover-not-fixed.html

Reftest is:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/backgrounds/background-size-body-cover-ref.html

Reftest list at line 71 
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/backgrounds/reftest.list#70
states:
!= background-size-body-cover-not-fixed.html background-size-body-cover-ref.html

and so every browser, including Firefox 18+, I tried fails this background-size-body-cover-not-fixed.html test according to such mismatching statement.

I believe this "!=" should be re-examined.

Gérard
Status: NEW → ASSIGNED
QA Contact: bugzilla
(Assignee)

Updated

5 years ago
QA Contact: bugzilla
If it's in the reftest.list without any failure annotations, then Firefox is passing it hundreds of times per day.  (It's running it at 800x1000, though; there's an annotation saying it fails at 600x600.)
...which means it presumably needs to be converted to the new standard size for reftests.
(Assignee)

Comment 6

5 years ago
> there's an annotation saying it fails at 600x600.)
> ...which means it presumably needs to be converted to the new standard size
> for reftests.

Adding, inserting  
  html
  {
  height: 300px;
  width: 300px;  
  }
would fix this. Eg.

http://www.gtalbot.org/BrowserBugsSection/CSS3Backgrounds/background-size-body-cover-not-fixed-new.html
Sorry for taking so long to look at this; I tend to ignore review requests from myself, which probably isn't the best idea.

This would have been a lot easier to review if the changes had been in separate patches, for example, one patch to move the images to support/ and fix up the tests, another patch to add the metadata bits, separate patches for reindentation if that was actually necessary (though it really would have been better to avoid), and other patches for the other logically-separate changes.

The cases where you changed tests to end with "\No newline at end of file" are bad.  There should be a newline at end of file, following Unix convention (where newline is a line terminator) rather than Windows convention (where it's a line separator).

I'm skeptical of tests named things like "Pixels-to-twips conversion" being appropriate for the test suite -- or at least for the test names being appropriate.

Adding snarky comments to the tests like:
>+<!--
>+"Red Means Failure
>+    Don't use the color red other than to indicate a failure."
>+http://wiki.csswg.org/test/format#design-requirements    
>+red color is pre-considered as a test failure;
>+best would be to replace red with another
>+more neutral color.
>+Gérard Talbot
>+-->
isn't helpful.  If you think it needs to be fixed, please fix it, but in a separate patch.

Otherwise I think the patch looks fine, and we should take it with those things revised if it still applies.  (Though I admit I didn't review all that carefully.)

You shouldn't use http://www.mozilla.com/ as the href for all these various authors, though.  Can you use the email from the changesets instead, as a mailto: URL?  I think that's been the normal convention.  (Or is that wrong?)

In layout/reftests/backgrounds/background-size-percent-percent-stretch-ref.html this change:
>-</html>
>+<s/html>
is wrong.

>+    <link rel="match" href="continuous-inline-1-ref.html">
>+    <!--
>+    Newer reftest is
>+    http://mxr.mozilla.org/mozilla-central/source/layout/reftests/backgrounds/continuous-inline-1ab-ref.html
>+    -->

This (repeated 4 times, though two with cd rather than ab) is wrong; it's the second URL that's the reference, and the first doesn't exist.

In layers-layer-count-cascade-1.xhtml and layers-layer-count-cascade-2.xhtml and layers-layer-count-inheritance-1.xhtml tests the link rel="match" is mis-indented.

In the unit-rem-* tests, why did you add quotes around all the id attributes?  They should be fine as-is.  (And it's, in general, good to have variety in tests.)

A few questions:

 * did you gather the 'author' metadata from 'hg log'?  I didn't check it, but if you gathered it some other way it probably needs to be rechecked.

 * did you check that the support files you moved weren't referenced by any other tests?

 * are you able to produce an updated patch, or does one of us need to?

Before we contribute these tests, I (but not you) should:

 * ask Keith Rarick for permission to contribute his tests to the CSS test suite?  He doesn't work for Mozilla.

 * send a courtesy email to all the other test authors involved (since I believe they worked for Mozilla at the time they wrote the tests, but to at least give them an opportunity to say otherwise)
Flags: needinfo?(bugzilla)
Attachment #706818 - Flags: feedback?(dbaron)
(Assignee)

Comment 8

4 years ago
> This would have been a lot easier to review if the changes had been in
> separate patches

This is something I wanted to do at one point. Create 1 patch for css-valuesandunits and then 1 patch for css3backgrounds  ... and then even possibly split css3backgrounds into 1 for backgrounds only and 1 for borders only. But I thought this was not required, not suitable because not requested.

>, for example, one patch to move the images to support/ and
> fix up the tests, another patch to add the metadata bits, separate patches
> for reindentation if that was actually necessary (though it really would
> have been better to avoid), and other patches for the other
> logically-separate changes.

I never thought of doing separate patches of this nature. For many reasons.

> The cases where you changed tests to end with "\No newline at end of file"
> are bad.  There should be a newline at end of file, following Unix
> convention (where newline is a line terminator) rather than Windows
> convention (where it's a line separator).

I was not aware of such convention and what it implies, its importance, etc. In my mind, files all have or use end-of-file control characters, which are added when saving, writing on local disk. But now I see that several editors have "End of line" settings..

> I'm skeptical of tests named things like "Pixels-to-twips conversion" being
> appropriate for the test suite -- or at least for the test names being
> appropriate.
> 
> Adding snarky comments to the tests like:
> >+<!--
> >+"Red Means Failure
> >+    Don't use the color red other than to indicate a failure."
> >+http://wiki.csswg.org/test/format#design-requirements    
> >+red color is pre-considered as a test failure;
> >+best would be to replace red with another
> >+more neutral color.
> >+Gérard Talbot
> >+-->
> isn't helpful. 

I've searched for understanding what "snarky comment" is/means. 
The comment I wrote was intended to be useful and helpful. In any case, it wanted to be a reminder. But, yes, I could have fix myself the color issue instead.

> If you think it needs to be fixed, please fix it, but in a
> separate patch.
> 
> Otherwise I think the patch looks fine, and we should take it with those
> things revised if it still applies.  (Though I admit I didn't review all
> that carefully.)
> 
> You shouldn't use http://www.mozilla.com/ as the href for all these various
> authors, though.  Can you use the email from the changesets instead, as a
> mailto: URL?  I think that's been the normal convention.  (Or is that wrong?)

I remember I tried to reach a few of these authors and some I couldn't (I reached Chris Lord, William Chen, Jeff Walden). If an author's email address was not listed in his/her original test, then was it okay for me to add, to disclose it? Spammers and spamming is a reality. If it was listed but possibly no longer valid, was it okay to add it anyway?
I remember hesitating on these questions and then thought the safe course was to add http://www.mozilla.com/ which is some sort of a "no-call". 
In the past and to a few persons submitting tests, I have recommended to test authors to _not_ edit their email address in their tests because of spammers, otherwise to obfuscate it a bit to prevent harvesting.

> In
> layout/reftests/backgrounds/background-size-percent-percent-stretch-ref.html
> this change:
> >-</html>
> >+<s/html>
> is wrong.
> 
> >+    <link rel="match" href="continuous-inline-1-ref.html">
> >+    <!--
> >+    Newer reftest is
> >+    http://mxr.mozilla.org/mozilla-central/source/layout/reftests/backgrounds/continuous-inline-1ab-ref.html
> >+    -->
> 
> This (repeated 4 times, though two with cd rather than ab) is wrong; it's
> the second URL that's the reference, and the first doesn't exist.

The first URL (continuous-inline-1-ref.html) - indeed - does not exist (but it did on january 14th 2013). The second URL is the correct reference: yes. I explained why in the last 5 lines of comment #1

> In layers-layer-count-cascade-1.xhtml and layers-layer-count-cascade-2.xhtml
> and layers-layer-count-inheritance-1.xhtml tests the link rel="match" is
> mis-indented.

Yes, the link rel="match" in those 3 files is mis-indented. And it shouldn't be.

> 
> In the unit-rem-* tests, why did you add quotes around all the id
> attributes? 

It is an habit adopted from reading the W3C specs, http://www.cs.tut.fi/~jkorpela/qattr.html
and from using advanced text editors and WYSIWYG HTML editors. Advanced text editors (auto-completion) and WYSIWYG HTML editors will surround attribute values with double quotes. Color syntax will sometimes not work if attribute value are not quoted. Generally speaking, there is very little to gain from not quoting.
Also, test source re-serialization is supposed to enclose/wrap  attribute values with quotes anyway.
http://wiki.csswg.org/test/format#acceptable-test-formats

> They should be fine as-is.  (And it's, in general, good to have
> variety in tests.)
> 
> A few questions:
> 
>  * did you gather the 'author' metadata from 'hg log'? 

Yes, I gathered the 'author' metadata from the hg log.

> I didn't check it,
> but if you gathered it some other way it probably needs to be rechecked.
> 
>  * did you check that the support files you moved weren't referenced by any
> other tests?

I'm not 100% sure. It's been a while. I believe I checked that the support files weren't referenced by other tests. I would have to check again.

From the patch, I see 16 pairs of "rename from" "rename to" for .png images support and 1 pair of "copy from" "copy to" .png image support.

>  * are you able to produce an updated patch, or does one of us need to?

If the patch needs to be corrected (smaller patches of logical nature, no reindentation, leaving untouched newline at end of file, fixing red color usage, adding mailto:author-email-address, fixing typos, continuous-inline-1-ref.html, 3 mis-indentations, etc), then I will correct, fix the patch. It may take me 3-4 weeks, starting from september 10th though.
I don't think it's possible to improve the patch as it is, right? I have to start all over again, right?

Gérard
You don't need to start over on this patch, but I'm asking you to split things into logical patches in the future.
(Assignee)

Comment 10

4 years ago
David, 

I will create smaller patches of logical nature in the future. I will need some guidance on how to do this.

Unfortunately, the reindentation, missing newline at end of file, red color usage, missing mailto:author-email-address things are also in attached patches of bug 835098, bug 837355, bug 837562.

Gérard
Flags: needinfo?(bugzilla)
You need to log in before you can comment on or make changes to this bug.