Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite css21_dev/html4/{font-family,fonts}-013.htm to use a free font #8053

Open
SimonSapin opened this issue Oct 16, 2015 · 17 comments
Open

Rewrite css21_dev/html4/{font-family,fonts}-013.htm to use a free font #8053

SimonSapin opened this issue Oct 16, 2015 · 17 comments
Labels

Comments

@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Oct 16, 2015

These tests rely on some fonts from Microsoft whose license does not allow redistribution. Getting these fonts on our CI servers is proving troublesome: #7625 (comment)

They should be modified to use different fonts whose license is not as problematic. To do so:

  • Make sure the msttcorefonts package (or similar) is installed.
  • Run ./mach test-css tests/wpt/css-tests/css21_dev/html4/fonts-013.htm tests/wpt/css-tests/css21_dev/html4/font-family-013.htm. The tests should pass.
  • Uninstall the package.
  • Run the tests again. They should fail.
  • In the test files, replace "Times New Roman", "Arial", and "Courrier New" with serif, sans-serif, and monospace (without quotes) respectively.
  • Run the tests again, with the package still uninstalled. Hopefully they should start passing.
  • Run ./mach run tests/wpt/css-tests/css21_dev/html4/font-family-013.htm and ./mach run tests/wpt/css-tests/css21_dev/html4/fonts-013.htm Check that each file renders shows the word "Ţęşţ" (Test with some diacritics on each letter) six times, and not for example just a square for each letter or nothing at all.
  • When everything looks good, submit a pull request to the upstream repository at https://github.com/w3c/csswg-test/tree/master/css21/fonts , not to the Servo repository. (Any local change to Servo’s copy of W3C tests would be overwritten at the next synchronization.)
@SimonSapin SimonSapin changed the title Rewrite css21_dev/html4/{font-family,font}-013.htm to use a free font Rewrite css21_dev/html4/{font-family,fonts}-013.htm to use a free font Oct 16, 2015
@metajack
Copy link
Contributor

@metajack metajack commented Oct 16, 2015

Isn't this really a bug that should be filed upstream?

cc @Ms2ger @jgraham

@SimonSapin
Copy link
Member Author

@SimonSapin SimonSapin commented Oct 16, 2015

If the generic names serif, sans-serif or monspace don’t work, we’ll need to find some fonts that contain these characters and are available on all of our CI platforms.

@gsnedders
Copy link
Contributor

@gsnedders gsnedders commented Oct 16, 2015

@metajack yes, probably.

@SimonSapin
Copy link
Member Author

@SimonSapin SimonSapin commented Oct 16, 2015

I don’t know if this is really a bug upstream. When microsoft fonts are not available other browsers seem to pick other, similar fonts. The tests doesn’t really require non-free fonts, it’s just the easiest way we know to make it pass unmodified in our code. There’s probably more stuff we can improve in our font selection code.

(That said, if generic names work, there is no harm in making that change upstream too.)

@gsnedders
Copy link
Contributor

@gsnedders gsnedders commented Oct 16, 2015

Other browsers have magic to fallback for some super-common fonts. The test suite almost certainly shouldn't rely on non-free fonts or unspecified behaviour, IMO. I'm happy to champion (or help to) upstream (and I don't think there will be any disagreement).

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Oct 17, 2015

This change cannot be made here; it will be overwritten automatically.

@SimonSapin
Copy link
Member Author

@SimonSapin SimonSapin commented Oct 17, 2015

Good point, I’ve added a step to the instructions in the original message.

@0X1A
Copy link
Contributor

@0X1A 0X1A commented Oct 20, 2015

Willing to take this issue

@SimonSapin
Copy link
Member Author

@SimonSapin SimonSapin commented Oct 21, 2015

@0X1A, great! Feel free to ask questions here or on IRC (#servo on irc.mozilla.org).

@0X1A
Copy link
Contributor

@0X1A 0X1A commented Oct 21, 2015

@SimonSapin So, changing the fonts to their generic counterparts does not solve the tests and I get a fail for both.

Changing reference/font-family-013-ref.html to match the changes in html4/font-family-013.html gets the test to pass but font-013.htm still fails. Any suggestions? I'm guessing the ref's are upstream files?

@gsnedders
Copy link
Contributor

@gsnedders gsnedders commented Oct 26, 2015

@0X1A Do you have your changes anywhere?

@0X1A
Copy link
Contributor

@0X1A 0X1A commented Oct 26, 2015

@gsnedders Yes, the changes are available here

@gsnedders
Copy link
Contributor

@gsnedders gsnedders commented Oct 27, 2015

That passes for me locally, FWIW. Oh, ignore me, I have the fonts so that's totally expected.

@gsnedders
Copy link
Contributor

@gsnedders gsnedders commented Oct 27, 2015

As I just commented in #7625 I'm leaning towards there being some bug in the font-matching code?

frewsxcv added a commit that referenced this issue Feb 19, 2016
Fixes #8053
bors-servo added a commit that referenced this issue Feb 19, 2016
Rewrite CSS tests to use free fonts.

Fixes #8053
@frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Feb 19, 2016

As per #9697, the switch to generic fonts fails on Linux, but passes on OSX

@frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Feb 19, 2016

Nevermind, I guess it fails on OSX on the builders too, despite passing locally on my OSX machine.

@jdm
Copy link
Member

@jdm jdm commented Mar 23, 2016

Turns out this wasn't easy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.