Closed Bug 1258916 Opened 4 years ago Closed 3 years ago

Import CSS Writing Modes tests from CSSWG's test repo

Categories

(Testing :: Reftest, defect)

defect
Not set

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(5 files, 12 obsolete files)

58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
No description provided.
It seems many of the reference pages for this spec lack the "ahem" flag. Gérard, could you please fix them?
Flags: needinfo?(bugzilla)
Xidorn,

I am not sure I understand here... Please give me a few examples of reference files that lack the ahem flag. 

And, of course, I will fix them.

Gérard
e.g.
css-writing-modes-3/block-flow-direction-001-ref.xht
css-writing-modes-3/block-flow-direction-002-ref.xht

It seems to me the reference files show PASS graph lack the "ahem" flag.
Xidorn,

You are correct! I missed the "ahem" flag in those reference files!

I have corrected
css-writing-modes-3/block-flow-direction-001-ref.xht 
css-writing-modes-3/block-flow-direction-002-ref.xht 
css-writing-modes-3/block-flow-direction-043-ref.xht

http://hg.csswg.org/test/rev/492f5be68732



 block-flow-direction-043-ref.xht  2016-03-25 00:23  5.5K  CSS Reftest Reference
 block-flow-direction-002-ref.xht  2016-03-25 00:23  5.5K  CSS Reftest Reference
 block-flow-direction-001-ref.xht  2016-03-25 00:23  5.3K  CSS Reftest Reference

I will check some more reference files right now... 

> It seems many of the reference pages for this spec lack the "ahem" flag.

Do you know of other reference files missing the "ahem" flag?

Gérard
Xidorn,

One more reference file:

http://hg.csswg.org/test/rev/c6d8b262affd

I have examined all the reference files and I think they are correct. If you find other reference files missing the "ahem" flag or any flag, please let me know and I will make the necessary corrections immediately.

And thank you for reporting this.

Gérard
Thanks for fixing that.
Flags: needinfo?(bugzilla)
Ahhh... Lots of fuzzy in the reftests on OS X and Windows... I wonder if it's really worth importing them... I believe the time I spent on trying importing them has been much longer than writing tests of text-combine-upright by myself...
Although I'm not going to import the tests at this moment, I think the preparation patches are still worth landing.
Keywords: leave-open
Review commit: https://reviewboard.mozilla.org/r/42957/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42957/
Attachment #8735823 - Flags: review?(dbaron)
Attachment #8735824 - Flags: review?(dbaron)
Attachment #8735825 - Flags: review?(dbaron)
Attachment #8735826 - Flags: review?(dbaron)
Attachment #8735827 - Flags: review?(dbaron)
Attachment #8735828 - Flags: review?(dbaron)
Attachment #8735829 - Flags: review?(dbaron)
Attachment #8735830 - Flags: review?(dbaron)
No longer blocks: 1097499
Attachment #8735823 - Flags: review?(dbaron) → review+
Comment on attachment 8735823 [details]
MozReview Request: Bug 1258916 part 1 - Ensure files are written with unix line ending. r=dbaron

https://reviewboard.mozilla.org/r/42957/#review42771
Attachment #8735824 - Flags: review?(dbaron) → review+
Comment on attachment 8735825 [details]
MozReview Request: Bug 1258916 part 3 - Add suffix for reference files if neither suffix nor prefix is present.

https://reviewboard.mozilla.org/r/42961/#review42775

I'm a bit hesitant about this.  I'd rather fix name conflicts by making what's in our repo be *more* like what's in the upstream repo rather than by making it *less* like what's in the upstream repo.

What collision did you get that required this?
Comment on attachment 8735826 [details]
MozReview Request: Bug 1258916 part 3 - Skip rather than throw when replacing a file with an identical one. r=dbaron

https://reviewboard.mozilla.org/r/42963/#review42777
Attachment #8735826 - Flags: review?(dbaron) → review+
Comment on attachment 8735827 [details]
MozReview Request: Bug 1258916 part 4 - Simplify code for skipping dir when populating test files. r=dbaron

https://reviewboard.mozilla.org/r/42965/#review42779
Attachment #8735827 - Flags: review?(dbaron) → review+
Comment on attachment 8735828 [details]
MozReview Request: Bug 1258916 part 5 - Make failures.list more general (include <failure-type> and allow path pattern), and merge skip.list into it. r=dbaron

https://reviewboard.mozilla.org/r/42967/#review42785

::: layout/reftests/w3c-css/import-tests.py:295
(Diff revision 1)
> -    gSkipList = [x for x in [x.lstrip().rstrip() for x in skipListFile]
> -                 if bool(x) and not x.startswith("#")]
> -    skipListFile.close()
> +            item = line.split()
> +            pat = re.compile(fnmatch.translate(item.pop()))
> +            gFailList.append((pat, item))

Please use the variable name items rather than item.

::: layout/reftests/w3c-css/import-tests.py:340
(Diff revision 1)
> +        # We may mark a reftest fail for "match", but in that case there
> +        # is a good chance we do not fail the "mismatch" references.
> +        if "fails" in fail and testType == "!=":
> +            fail = ["skip"]

Was this actually a problem?  I'd prefer to avoid doing this unless it was -- and if it was, maybe come up with something narrower?
Attachment #8735828 - Flags: review?(dbaron) → review+
Comment on attachment 8735829 [details]
MozReview Request: Bug 1258916 part 7 - Optimize PNG files when importing them.

https://reviewboard.mozilla.org/r/42969/#review42791

Please don't do this.

The test repository is probably a likely place to find interesting images where we don't want to optimize things away.
Attachment #8735830 - Flags: review?(dbaron) → review+
Comment on attachment 8735830 [details]
MozReview Request: Bug 1258916 part 6 - Do not add CDATA wrap for ahem font style for html files. r=dbaron

https://reviewboard.mozilla.org/r/42971/#review42793

I'm a little surprised that this code is unused.
Comment on attachment 8735831 [details]
MozReview Request: Bug 1258916 part 9 - Import CSS Writing Modes test from CSSWG test repo.

https://reviewboard.mozilla.org/r/42973/#review42795

::: layout/reftests/w3c-css/failures.list:39
(Diff revision 1)
>  skip css-values-3/vh-support-transform-translate.html
> +
> +
> +#### CSS Writing Modes 3 #############################################
> +
> +# Too many fuzzies on OS X and Windows to add fuzzy-if indivisually...

"individually"

::: layout/reftests/w3c-css/failures.list:39
(Diff revision 1)
> +# Too many fuzzies on OS X and Windows to add fuzzy-if indivisually...
> +# Just disable all of them together!
> +skip-if(OSX||winWidget) css-writing-modes-3/*.xht

This is a lot of tests marked fuzzy.  Please file a followup bug on investigating this.

::: layout/reftests/w3c-css/failures.list:44
(Diff revision 1)
> +# Bug 1167911
> +skip css-writing-modes-3/abs-pos-non-replaced-icb-vlr-021.xht
> +skip css-writing-modes-3/abs-pos-non-replaced-icb-vrl-020.xht

Why are these skip rather than fails?
Attachment #8735831 - Flags: review+
Comment on attachment 8735832 [details]
MozReview Request: Bug 1258916 part 9b - The imported files.

https://reviewboard.mozilla.org/r/42975/#review42799

I presume you intend to merge this into the previous changeset?
Attachment #8735832 - Flags: review+
https://reviewboard.mozilla.org/r/42961/#review42775

There are some reference pages have the same name with their test pages but in a different directory in tests for CSS Writing Modes. Should I rename those files directly in the upstream repo?
Comment on attachment 8735823 [details]
MozReview Request: Bug 1258916 part 1 - Ensure files are written with unix line ending. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42957/diff/1-2/
Attachment #8735823 - Attachment description: MozReview Request: Bug 1258916 part 1 - Ensure files are written with unix line ending. → MozReview Request: Bug 1258916 part 1 - Ensure files are written with unix line ending. r=dbaron
Attachment #8735824 - Attachment description: MozReview Request: Bug 1258916 part 2 - Update existing tests. → MozReview Request: Bug 1258916 part 2 - Update existing tests. r=dbaron
Attachment #8735826 - Attachment description: MozReview Request: Bug 1258916 part 4 - Skip rather than throw when replacing a file with an identical one. → MozReview Request: Bug 1258916 part 3 - Skip rather than throw when replacing a file with an identical one. r=dbaron
Attachment #8735827 - Attachment description: MozReview Request: Bug 1258916 part 5 - Simplify code for skipping dir when populating test files. → MozReview Request: Bug 1258916 part 4 - Simplify code for skipping dir when populating test files. r=dbaron
Attachment #8735828 - Attachment description: MozReview Request: Bug 1258916 part 6 - Make failures.list more general (include <failure-type> and allow path pattern), and merge skip.list into it. → MozReview Request: Bug 1258916 part 5 - Make failures.list more general (include <failure-type> and allow path pattern), and merge skip.list into it. r=dbaron
Attachment #8735830 - Attachment description: MozReview Request: Bug 1258916 part 8 - Do not add CDATA wrap for ahem font style for html files. → MozReview Request: Bug 1258916 part 6 - Do not add CDATA wrap for ahem font style for html files. r=dbaron
Comment on attachment 8735824 [details]
MozReview Request: Bug 1258916 part 2 - Update existing tests. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42959/diff/1-2/
Comment on attachment 8735826 [details]
MozReview Request: Bug 1258916 part 3 - Skip rather than throw when replacing a file with an identical one. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42963/diff/1-2/
Comment on attachment 8735827 [details]
MozReview Request: Bug 1258916 part 4 - Simplify code for skipping dir when populating test files. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42965/diff/1-2/
Comment on attachment 8735828 [details]
MozReview Request: Bug 1258916 part 5 - Make failures.list more general (include <failure-type> and allow path pattern), and merge skip.list into it. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42967/diff/1-2/
Comment on attachment 8735830 [details]
MozReview Request: Bug 1258916 part 6 - Do not add CDATA wrap for ahem font style for html files. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42971/diff/1-2/
Attachment #8735825 - Attachment is obsolete: true
Attachment #8735829 - Attachment is obsolete: true
Attachment #8735831 - Attachment is obsolete: true
Attachment #8735832 - Attachment is obsolete: true
I guess what is doable here is, we can import all of the tests, but skip them initially. Then we can enable them individually later.
OK, all green on Linux, I guess it's good to go now...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4f0197e5036
Assignee: nobody → bugzilla
Keywords: leave-open
Attachment #8735823 - Attachment is obsolete: true
Attachment #8735824 - Attachment is obsolete: true
Attachment #8735826 - Attachment is obsolete: true
Attachment #8735827 - Attachment is obsolete: true
Attachment #8735828 - Attachment is obsolete: true
Attachment #8735830 - Attachment is obsolete: true
Hmmm, it seems it causes a permafail on Linux debug R-e10s1 that the test takes longer than 2hrs... The tests are slow, probably because they require HTTP due to Ahem... Probably better fix bug 1261279 as soon as possible.
There's one (set) of really really slow test, namely text-orientation-script-001[a-o]?. You may want to skip it and see how long the rests will take.
(In reply to Koji Ishii from comment #45)
> There's one (set) of really really slow test, namely
> text-orientation-script-001[a-o]?. You may want to skip it and see how long
> the rests will take.

We import reftests only, so they are not included.
It also seems that we have already imported part of the Writing Modes test suite. I think we want to dedup somehow.
Attachment #8741781 - Attachment description: MozReview Request: Bug 1258916 part 7 - Use the directory structure from upstream repo for test and reference files. r?dbaron → Bug 1258916 part 7 - Use the directory structure from upstream repo for test and reference files.
Attachment #8741782 - Attachment description: MozReview Request: Bug 1258916 part 8 - Import CSS Writing Modes test from CSSWG test repo. r?dbaron → Bug 1258916 part 8 - Import CSS Writing Modes test from CSSWG test repo.
Attachment #8741783 - Attachment description: MozReview Request: Bug 1258916 part 8b - The imported files. → Bug 1258916 part 8b - The imported files.
Attachment #8741781 - Flags: review?(dbaron)
I just requested review for part 7 for keeping the directory structure for reftests, so that we may be able to recursively add support files for bug 1256580.

You don't need to review part 8 and part 8b because I'm not going to land them at this moment. (And you've actually reviewed those patches before... They were not landed because I haven't figured out how to work around the timeout failure.)
Flags: needinfo?(dbaron)
See Also: → 1256580
Comment on attachment 8741781 [details]
Bug 1258916 part 7 - Use the directory structure from upstream repo for test and reference files.

https://reviewboard.mozilla.org/r/46747/#review88240

Sorry for delying so long on this.

Hopefully merging with Neerja's changes (which I think have landed!) shouldn't be too hard.

::: layout/reftests/w3c-css/import-tests.py
(Diff revision 1)
> -        if filecmp.cmp(srcfile, destfile):
> -            print "Warning: duplicate file {}".format(destname)
> -            return

Are you really sure this isn't an issue?  Seems like it may as well stay, for safety.  (This was designed to catch future mistakes...)

::: layout/reftests/w3c-css/import-tests.py
(Diff revision 1)
> -        if filecmp.cmp(srcfile, destfile):
> -            print "Warning: duplicate file {}".format(destname)
> -            return

Are you really sure this isn't an issue?  Seems like it may as well stay, for safety.  (This was designed to catch future mistakes...)

::: layout/reftests/w3c-css/import-tests.py:166
(Diff revision 1)
> -    load_flags_for(fn, spec)
> +    load_flags_for(fn, destname)
>      copy_file(destname, fn, destname, False)
> -    copy_support_files(destname, os.path.dirname(fn), spec)
> +    copy_support_files(destname, os.path.dirname(fn))
>      return destname
>  
> -def load_flags_for(fn, spec):
> +def load_flags_for(fn, destname):

I wonder if it would be worth writing a patch on top of this one to change most uses of "fn" to "srcname" or similar.  That might make this a bit clearer.

::: layout/reftests/w3c-css/import-tests.py:246
(Diff revision 1)
>  
>          if not isSupportFile and not ahemFontAdded and 'ahem' in gTestFlags[test] and re.search(searchRegex, line):
>              # First put our ahem font declation before the first <style>
>              # element
> -            newFile.write(AHEM_DECL_HTML if is_html(aDestFileName) else AHEM_DECL_XML)
> +            template = AHEM_DECL_HTML if is_html(aDestFileName) else AHEM_DECL_XML
> +            path = os.path.relpath(AHEM_FONT_PATH, os.path.dirname(aDestFileName))

maybe call this ahem_path rather than just path?
Attachment #8741781 - Flags: review?(dbaron) → review+
Flags: needinfo?(dbaron)
Comment on attachment 8741781 [details]
Bug 1258916 part 7 - Use the directory structure from upstream repo for test and reference files.

https://reviewboard.mozilla.org/r/46747/#review88240

> Are you really sure this isn't an issue?  Seems like it may as well stay, for safety.  (This was designed to catch future mistakes...)

The main idea of this commit is to copy the whole directory structure from csswg repo. If the directory structure is copied, why could there still be any duplicate file?

> Are you really sure this isn't an issue?  Seems like it may as well stay, for safety.  (This was designed to catch future mistakes...)

I wonder why this issue is duplicated...
Keywords: leave-open
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f33bcb35609a
part 7 - Use the directory structure from upstream repo for test and reference files. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/498f5669f48d
part 8 - Change most uses of 'fn' to 'srcname' to make it clearer. rs=dbaron
Attachment #8741781 - Attachment is obsolete: true
Attachment #8741783 - Attachment is obsolete: true
It seems to be failing on Windows 7 debug build now... Can we split the reftest tasks on Windows 7 debug build?
Depends on: 1317261
Not sure if there are more existing imported reftests we can remove to mitigate the hit of test time.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee051ed6ab12

Probably we can also disable the whole bunch of the newly imported tests on Windows...
There's an awful lot of fuzziness on a bunch of these tests. :( A lot of it looks like it arises because of tests that compare glyph rendering (Ahem boxes) with PNG images; this tends to be problematic because rendering systems apply antialiasing to the glyphs.

Gérard, would it be reasonable to modify reference files such as block-flow-direction-*-ref.xht to use the Ahem font instead of PNG images for their blocks? ISTM that could still test the layout issues here equally well, and I'd expect it to avoid a lot of the spurious issues due to antialiasing at the edges. Annotating whole batches of tests as having thousands of mismatched "fuzzy" pixels is unfortunate...
Flags: needinfo?(bugzilla)
Comment on attachment 8810296 [details]
Bug 1258916 part 9 - Optimize re.sub usage in import script.

https://reviewboard.mozilla.org/r/92638/#review92754
Attachment #8810296 - Flags: review?(dbaron) → review+
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #50)

> The main idea of this commit is to copy the whole directory structure from
> csswg repo. If the directory structure is copied, why could there still be
> any duplicate file?

I know there will be several of my tests being duplicated. I often created draft tests and they are used in bug reports. The submitted ones to the CSSWG test repository are filename-renamed.

Eg
http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s71-abs-pos-non-replaced-vrl-040.xht

is roughly the same as

https://dxr.mozilla.org/mozilla-central/source/layout/reftests/writing-mode/abspos/s71-abs-pos-non-replaced-vrl-040.xht
(click Raw link to view test)

which is roughly the same as

[CSSWG repository]
http://test.csswg.org/source/css-writing-modes-3/abs-pos-non-replaced-vrl-040.xht

The CSSWG repository tests that I authored are generally - if not always - more polished (better test assert, more or better comments) than the ones I authored in mozilla-central/layout/reftests/writing-mode/ .


- - - - - - -


(In reply to Jonathan Kew (:jfkthame) from comment #63)
> There's an awful lot of fuzziness on a bunch of these tests. :( A lot of it
> looks like it arises because of tests that compare glyph rendering (Ahem
> boxes) with PNG images; this tends to be problematic because rendering
> systems apply antialiasing to the glyphs.

I thought you guys configured operating system antialiasing to minimize fuzziness at edges.
I have been using swatch-* images or PNG images for many years now when creating reference files...

> Gérard, would it be reasonable to modify reference files such as
> block-flow-direction-*-ref.xht to use the Ahem font instead of PNG images
> for their blocks? 

It is reasonable. I will use the Ahem font instead of PNG images for 

http://test.csswg.org/source/css-writing-modes-3/block-flow-direction-001-ref.xht

http://test.csswg.org/source/css-writing-modes-3/block-flow-direction-002-ref.xht

http://test.csswg.org/source/css-writing-modes-3/block-flow-direction-043-ref.xht

> ISTM that could still test the layout issues here equally
> well

Yes, it should still test the layout issues

> , and I'd expect it to avoid a lot of the spurious issues due to
> antialiasing at the edges. Annotating whole batches of tests as having
> thousands of mismatched "fuzzy" pixels is unfortunate...

I use Mercurial and I do not know for sure right now if the CSSWG test server has now been modified...
Flags: needinfo?(bugzilla)
(In reply to Jonathan Kew (:jfkthame) from comment #63)
> There's an awful lot of fuzziness on a bunch of these tests. :( A lot of it
> looks like it arises because of tests that compare glyph rendering (Ahem
> boxes) with PNG images; this tends to be problematic because rendering
> systems apply antialiasing to the glyphs.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #7)
> Ahhh... Lots of fuzzy in the reftests on OS X and Windows... 

I do not have OS X and do not use OS X; I do not have Windows and do not use Windows. So, when creating reference files, I may not see the fuzziness that you see. I wish we could find a solution I could apply to reference files from now on...
I guess making block-flow-direction-{001,002,043}-ref.xht using glyphs should reduce or eliminate fuzziness on block-flow-direction-*, line-box-direction-*, row-progression-*, table-column-order-*, which are the tests with largest difference numbers.

We still have several other tests with fuzzy, though...

I wonder whether it is possible to simply disable AA on Ahem font, so that we can resolve all kinds of fuzzy needed for this...
> block-flow-direction-*,
> line-box-direction-*, row-progression-*, table-column-order-*, which are the
> tests with largest difference numbers.

(aside comment)
For many reasons, it is important, whenever possible and convenient, to reuse already created and available reference files.
>> why could there still be any duplicate file?
> 
> I know there will be several of my tests being duplicated. 

A bunch of tests which use this image

https://dxr.mozilla.org/mozilla-central/source/layout/reftests/writing-mode/blue-yellow-206w-165h.png

or

https://dxr.mozilla.org/mozilla-central/source/layout/reftests/writing-mode/1172774-percent-horizontal-ref.html

as reference will be duplicated. I believe they have not been removed.
Unfortunately there are still fuzzy, although much smaller. I think the ultimate way is to disable AA on Ahem, but have no idea how to.

See for example: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca0759804ee627553801222d808880d7733398ac&selectedJob=31057100
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #72)
> Unfortunately there are still fuzzy, although much smaller. I think the
> ultimate way is to disable AA on Ahem, but have no idea how to.

In theory, we should be able to do that by adjusting the 'gasp' table in the Ahem font. I don't know whether our various back-ends will correctly respect that, though. I've just pushed a try job remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c7459c9e8fdf6dad2e84d37f8a1217a84107852 with a modified Ahem.ttf to see what happens.... if it does help, perhaps we could suggest this as a modification to the canonical Ahem font.

> 
> See for example:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ca0759804ee627553801222d808880d7733398ac&selectedJob=3
> 1057100

Well, it's an improvement over the thousands of fuzzy pixels we had previously, at least.
Almost there: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a97607fc14a159c00ad59023f03646be2671d0b all the oranges are timeout exceeding 120mins.

It seems not using http on imported tests for ahem font makes tests run significantly faster (no timeout anymore! 80+min vs. >120min for Windows debug builds, ~60min vs. 90+min for OS X debug build). And the tests run almost fine, except some intermittent TEST-UNEXPECTED-PASS(?!) which I may need to investigate a bit further: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e20e45bf6c9d44c6c05a5f21600990afc7104b3b&selectedJob=31323779

If it turns out that http is really not necessary for accessing font outside the directory, we may be able to remove many http directives elsewhere, which would potentially make reftests run much faster.
Not using HTTP works locally as well... I want to understand why it works, and why it sometimes doesn't work. Also given that significant speed-up, if it eventually turns out non-HTTP accessing to ancestor directory is not going to work, we may want a different strategy for handling ahem: we may just duplicate the ahem font in their directories so that the tests can load it without needing HTTP.
dbaron, do you have any idea why the tests can work mostly fine without having HTTP() directive?
Flags: needinfo?(dbaron)
No longer depends on: 1317261
Probably because of https://hg.mozilla.org/mozilla-central/rev/d6e2267744e3 (which now lives in reftest-preferences.js).
Flags: needinfo?(dbaron)
I filed bug 1318526 on removing the HTTP(..) where not needed.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #79)
> It seems not using http on imported tests for ahem font makes tests run
> significantly faster (no timeout anymore! 80+min vs. >120min for Windows
> debug builds, ~60min vs. 90+min for OS X debug build). And the tests run
> almost fine, except some intermittent TEST-UNEXPECTED-PASS(?!) which I may
> need to investigate a bit further:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e20e45bf6c9d44c6c05a5f21600990afc7104b3b

All unexpected pass items are from one of line-box-direction-{slr-056,srl-055,vlr-016,vrl-015}.xht, which is currently marked as fails-if(!Android) because of bug 1227616. I guess the rendering of these tests depends on some timing, which could be affected by whether http is used, which means that is not necessary an issue of loading font.

I guess we can just mark those tests random then...

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #82)
> Probably because of https://hg.mozilla.org/mozilla-central/rev/d6e2267744e3
> (which now lives in reftest-preferences.js).

Great. I think we can now remove the http directive for ahem now :)
Comment on attachment 8812021 [details]
Bug 1258916 part 10 - Not attach HTTP directive to tests which need ahem font.

https://reviewboard.mozilla.org/r/93912/#review94060

::: layout/tools/reftest/reftest-preferences.js:32
(Diff revision 1)
> -// Setting this pref makes tests run much faster there.
> +// Setting this pref makes tests run much faster there. Reftests also
> +// rely on this to load resource files from outside their directory.

I think I'd be more specific, since the same origin policy doesn't affect most cases of loading.  So instead of "load resource files" I'd say "load downloadable fonts (which are restricted to same origin by default)"
Attachment #8812021 - Flags: review?(dbaron) → review+
Comment on attachment 8741782 [details]
Bug 1258916 part 11 - Import CSS Writing Modes test from CSSWG test repo.

https://reviewboard.mozilla.org/r/46749/#review94102

::: layout/reftests/w3c-css/failures.list:108
(Diff revision 5)
> +fails-if(!Android) css-writing-modes-3/line-box-direction-slr-056.xht
> +fails-if(!Android) css-writing-modes-3/line-box-direction-srl-055.xht
> +fails-if(!Android) css-writing-modes-3/line-box-direction-vlr-016.xht
> +fails-if(!Android) css-writing-modes-3/line-box-direction-vrl-015.xht

It seems I forgot to mark them random before last push... I thought I did, but hmm...
Keywords: leave-open
Since jfkthame doesn't seem to get here soonish, I'm going to land part 9 and part 10 first, to reduce the risk of conflict, and accelerate reftest tasks by removing HTTP directive from 100+ existing imported tests.
Keywords: leave-open
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8f9cdefb1f8
part 9 - Optimize re.sub usage in import script. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/f89eef33b118
part 10 - Not attach HTTP directive to tests which need ahem font. r=dbaron
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #92)
> All green:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=a8e178a5da13bd4a9c786033d2e0cf053b1376e7

Looks like this wasn't quite all green, actually; there are several unexpected-pass oranges on Linux x64 opt. Maybe they can just have failure annotations removed, if they really are passing now? (Do we understand what happened to affect them?)

And the Linux (32) failure on svg-glyph-objectopacity.svg looks to be pretty consistent, not an intermittent issue. I guess it can just be annotated with fuzzy-if.
(In reply to Jonathan Kew (:jfkthame) from comment #100)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #92)
> > All green:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=a8e178a5da13bd4a9c786033d2e0cf053b1376e7
> 
> Looks like this wasn't quite all green, actually; there are several
> unexpected-pass oranges on Linux x64 opt. Maybe they can just have failure
> annotations removed, if they really are passing now? (Do we understand what
> happened to affect them?)
> 
> And the Linux (32) failure on svg-glyph-objectopacity.svg looks to be pretty
> consistent, not an intermittent issue. I guess it can just be annotated with
> fuzzy-if.

Those are not tests imported here. I suppose they are unrelated. But I'll do a new try push to see whether they are blockers of landing patches here.
I don't see any failure on reftests https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c9533d9bec2ddc827becf44e848c45869e0e5ff&exclusion_profile=false

The failing tests in comment 92 seem no longer around? I only see tc-R(), no R() there at all.
Comment on attachment 8741782 [details]
Bug 1258916 part 11 - Import CSS Writing Modes test from CSSWG test repo.

https://reviewboard.mozilla.org/r/46749/#review98570

OK, let's get this landed, thanks. Sorry for being slow here.
Attachment #8741782 - Flags: review?(jfkthame) → review+
Comment on attachment 8810298 [details]
Bug 1258916 part 12 - Remove some of duplicate writing modes tests in tree.

https://reviewboard.mozilla.org/r/92642/#review98572
Attachment #8810298 - Flags: review?(jfkthame) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b04b434494d
part 11 - Import CSS Writing Modes test from CSSWG test repo. r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c1a2c6d4ad2
part 12 - Remove some of duplicate writing modes tests in tree. r=jfkthame
Keywords: leave-open
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddcab08c663e
part 12A - increase known fuzzyness for ua-style-sheet-checkbox-radio-1.html to fix Android test failure. r=test-fix
Had to increase the fuzzyness for a test on Android which started to fail after your changes landed, see comment 106.
Flags: needinfo?(xidorn+moz)
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #107)
> Had to increase the fuzzyness for a test on Android which started to fail
> after your changes landed, see comment 106.

Thanks for doing that. I think the fix is fine.

It's interesting that it happens on a test which seems to be unrelated (not part of imported test).
Flags: needinfo?(xidorn+moz)
I have about 100 additional writing-modes tests in good but still draft state and another 100 tests that I am planning to created and then submitted to CSSWG's test repo. Will you reactivate this bug when those 200-ish tests are submitted ... or is this importation of CSS Writing Modes tests from CSSWG test repository done on a regular automated manner?
Flags: needinfo?(xidorn+moz)
(In reply to Gérard Talbot from comment #110)
> I have about 100 additional writing-modes tests in good but still draft
> state and another 100 tests that I am planning to created and then submitted
> to CSSWG's test repo. Will you reactivate this bug when those 200-ish tests
> are submitted ... or is this importation of CSS Writing Modes tests from
> CSSWG test repository done on a regular automated manner?

That's a good question. I guess we should open new bug for importing the new tests.

Unfortunately, the importing here is not done on a regular mannar, unlike what we do with WPT. But I guess it would be good if we can do that regularly.

There is a script for importing tests in various directories from CSSWG's test repo, but it needs manual work to handle additional failures introduced from each import. I can probably volunteer to do this work regularly.
Flags: needinfo?(xidorn+moz)
(In reply to Jonathan Kew (:jfkthame) from comment #63)
> There's an awful lot of fuzziness on a bunch of these tests. :( A lot of it
> looks like it arises because of tests that compare glyph rendering (Ahem
> boxes) with PNG images; this tends to be problematic because rendering
> systems apply antialiasing to the glyphs.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #69)
> I wonder whether it is possible to simply disable AA on Ahem font, so that
> we can resolve all kinds of fuzzy needed for this...

Over at webkit, I noticed that they declare 
'-webkit-font-smoothing: none;'
alongside 
'font-family: Ahem;'
in tests. Mozilla apparently supports the non-standard
'font-smooth: never;'
https://developer.mozilla.org/en/docs/Web/CSS/font-smooth
and also
'-moz-osx-font-smoothing' 
(no MDN documentation on this though)

Have you (Jonathan or Xidorn) tried to see how the fuzziness issue is or becomes if tests use 'font-smooth: never'? This can be checked by you guys. Keep in mind that I do not have OS X and do not use OS X; I do not have Windows and do not use Windows. So, when creating reference files, I may not see the fuzziness that you see and I can control antialiasing on my Linux system.
gfx.use_text_smoothing_setting
{
True

Honor the “Turn off text smoothing for font sizes ___ and smaller” setting in OS X’s appearance system preferences when deciding whether to anti-alias rendered text. 
}
http://kb.mozillazine.org/Gfx.use_text_smoothing_setting#True
I think I've tried them before... but I can test them again later. ni? myself for now.
Flags: needinfo?(xidorn+moz)
Filed bug 1330831 to add pref for disabling anti-aliasing for testing.
Flags: needinfo?(xidorn+moz)
(In reply to Jonathan Kew (:jfkthame) from comment #63)
> There's an awful lot of fuzziness on a bunch of these tests. :( A lot of it
> looks like it arises because of tests that compare glyph rendering (Ahem
> boxes) with PNG images; this tends to be problematic because rendering
> systems apply antialiasing to the glyphs.
> 
> Gérard, would it be reasonable to modify reference files such as
> block-flow-direction-*-ref.xht to use the Ahem font instead of PNG images
> for their blocks? ISTM that could still test the layout issues here equally
> well, and I'd expect it to avoid a lot of the spurious issues due to
> antialiasing at the edges. Annotating whole batches of tests as having
> thousands of mismatched "fuzzy" pixels is unfortunate...

It's notable that <span style="font: 80px/1 Ahem;">X</span> renders differently depending on the value of writing-mode, despite it being rotating a 80x80 square by 90°. If you give it a background and color, like red and green respectively, you see some red in the anti-aliasing at the bottom despite it being entirely obscured by the ink area of the glyph.

I don't know whether it makes sense to consider that a bug in Gecko (I don't see similar in Chrome or Safari), but fixing that would do a lot to make it easier to right accurate references for writing-mode.
jfkthame, any thought on comment 116?
Flags: needinfo?(jfkthame)
> It's notable that <span style="font: 80px/1 Ahem;">X</span> renders differently
> depending on the value of writing-mode, despite it being rotating a 80x80
> square by 90°. If you give it a background and color, like red and green
> respectively, you see some red in the anti-aliasing at the bottom despite it
> being entirely obscured by the ink area of the glyph.

This may depend, in part at least, on whether we're dealing with a setup where CSS pixels correspond to an integer multiple of device pixels (which is often _not_ the case on Windows, for example).

Example:
data:text/html,<span style="background:red;color:green;font: 80px/1 Ahem;">X<br><br>
               <span style="writing-mode:vertical-rl">X

In Firefox on macOS with a retina screen I do not see any red at the bottom of either square; and they seem to be antialiased exactly the same. But if I set layout.css.devPixelsPerPx to 1.5, suddenly the vertical-rl square (but not the horizontal one) does have a line of pixels at the bottom where the red background shows through.

(In general, if the glyph edges do not correspond exactly to device-pixel edges on all sides, some background show-through is likely to occur, because the antialiasing pixels at the edges will be semi-transparent.)

I can imagine writing-mode affecting this in part because IIRC we try to snap text baselines to integer device pixels; for horizontal mode, that means we're snapping to device pixels in the vertical direction (but potentially using fractional-pixel positioning horizontally), and vice versa for vertical mode. So what is theoretically "the same" square glyph may be positioned slightly differently with respect to the device-pixel grid depending on the writing mode, and will be antialiased differently as a result.

In principle, I think the right thing to do here would be to adjust the 'gasp' table[1] in the Ahem font such that it specifies that smoothing (antialiasing) should _not_ be applied (though probably grid-fitting should be used); and then ensure that Gecko respects this (which I don't think it currently does in all cases/platforms/backends, but that may need further investigation). This should allow Ahem-based tests to render without antialiasing artifacts across all browsers (provided they respect the font's 'gasp' table properly) and without requiring custom prefs in the test harness, etc.

[1] https://www.microsoft.com/typography/otspec/gasp.htm
Flags: needinfo?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #118)

> But if I
> set layout.css.devPixelsPerPx to 1.5, suddenly the vertical-rl square (but
> not the horizontal one) does have a line of pixels at the bottom where the
> red background shows through.

Why (for what purpose, realistically speaking) would a tester or a browser manufacturer want to change the ratio of the resolution in physical pixels to the resolution in CSS pixels of current display device (window.devicePixelRatio) to a value different from 1 for testing CSS tests, when testing CSS tests? 
I always assumed and always assume that such ratio was and is 1 . 

> I think the right thing to do here would be to adjust the
> 'gasp' table[1] in the Ahem font such that it specifies that smoothing
> (antialiasing) should _not_ be applied (though probably grid-fitting should
> be used); and then ensure that Gecko respects this (which I don't think it
> currently does in all cases/platforms/backends, but that may need further
> investigation). This should allow Ahem-based tests to render without
> antialiasing artifacts across all browsers

The code used to generate variants of Ahem font is at https://github.com/litherum/AhemMaker and there is a func gaspTable() .

> [1] https://www.microsoft.com/typography/otspec/gasp.htm

http://lists.w3.org/Archives/Public/public-css-testsuite/2016Aug/0002.html
> > (...) would be to adjust the
> > 'gasp' table[1] in the Ahem font such that it specifies that smoothing
> > (antialiasing) should _not_ be applied (though probably grid-fitting should
> > be used); and then ensure that Gecko respects this (which I don't think it
> > currently does in all cases/platforms/backends, but that may need further
> > investigation). This should allow Ahem-based tests to render without
> > antialiasing artifacts across all browsers
> 
> The code used to generate variants of Ahem font is at
> https://github.com/litherum/AhemMaker and there is a func gaspTable() .

https://github.com/litherum/AhemMaker/blob/master/AhemMaker/main.swift#L308

https://github.com/litherum/AhemMaker/blob/master/AhemMaker/main.swift#L314

 append(result, value: UInt16(2)) // Greyscale

This apparently sets the anti-aliasing method to greyscale anti-aliasing.

 append(result, value: UInt16(3)) // Gridfit and greyscale

- - - - - - -

Xidorn,

Assuming you are using Windows 8 or Windows 10, have you tried

https://addons.mozilla.org/en-US/firefox/addon/anti-aliasing-tuner/

I have also found an abundant documentation about anti-aliasing in Windows and in Linux: eg

superuser.com/questions/267726/how-do-i-disable-font-smoothing-hinting-anti-aliasing-on-firefox-4
You need to log in before you can comment on or make changes to this bug.