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

[css-text] Test for text-tranform:full-size-kana #13461

Merged
merged 1 commit into from
Dec 26, 2018

Conversation

frivoal
Copy link
Contributor

@frivoal frivoal commented Oct 11, 2018

@r12a
Copy link
Contributor

r12a commented Oct 11, 2018

This is just a suggestion: I changed the format for these kinds of tests recently to make it easier for manual reviewers to do the comparisons (and slightly easier to create the ref tests). You can see an example at https://w3c.github.io/i18n-tests/run?base=css-text&batch=text-transform&test=text-transform/text-transform-fullwidth-005.html Basically, results are superimposed such that divergeances appear in red. Makes it very quick to spot fails. Just mentioning it in case you think it's a good idea, since your tests are similar.

Fwiw, here's an example that will probably contain failures on Firefox: https://w3c.github.io/i18n-tests/run?base=css-text&batch=text-transform&test=text-transform/text-transform-fullwidth-005.html

@frivoal
Copy link
Contributor Author

frivoal commented Oct 11, 2018

I considered doing something like this, and decided against because it's really hard to decide if the test is passing of failing when you have sub-pixel or single pixel differences, and just a tiny little bit of red showing. In the full size kana case, a real failure would involve a lot more red than that, but the reviewer does not know that, so when font rendering artifact show a little bit of red, it's not clear.

Case in point, with the test you shared, on my machine, chrome is a clear failure:
screenshot 2018-10-11 20 51 19

Firefox, on the other hand, is maybe a failure, maybe not. I think it is meant to be a pass, but I do see some red:
screenshot 2018-10-11 20 52 56

A human reviewer would be unsure, and an automated ref checker would fail it, even though it is fine.

@r12a
Copy link
Contributor

r12a commented Oct 11, 2018

Yes. I thought a lot about that too, but came to the opposite conclusion ;-). I didn't think the subpixel shadow would be picked up on the ref test - i could be wrong. The instructions on some of the other tests i did like this actually mention that subpixel shadows don't count.

@frivoal
Copy link
Contributor Author

frivoal commented Oct 14, 2018

To be rebased on top of #13504, as discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1498148#c41

@frivoal
Copy link
Contributor Author

frivoal commented Oct 15, 2018

@dpino Rebased on top of your tests from #13504 to try and get the best of both worlds:

  • kept your warning about missing glyphs
  • kept the test checking that already full-size kana don't get changed
  • used by format to make it easier to review by just looking at it
  • split into multiple files to fit a single screen
  • simplified the markup a bit
  • made the test that check that nothing happens on already full-size kana only apply the (ineffective) transform to one character in each pair, as otherwise it isn't really testing anything.

Review appreciated.

@frivoal
Copy link
Contributor Author

frivoal commented Oct 15, 2018

<side-discussion>
@r12a, when it comes to red showing at the edge of glyphs due to anti aliasing issues, this is actually causing problems. See this new PR I made in response to the this kind of problem: #13510
</side-discussion>

Rework the tests from web-platform-tests#13504,
so that:
* They fit within 600px × 600px (as generally required for reftests)
* They are easier for a human reader to review: being asked whether two
  characters are the same when the only difference expected is size is
  much easier if you have a reference for the right and wrong size.
  Only doing this in tests 001 to 004,
  as for the rest there's not (always) a corresponding small character.
* The test that check that nothing happens on already full-size kana
  only applies the (ineffective) transform to one character in each pair
  as otherwise, it isn't really testing anything
Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frivoal frivoal merged commit a546e84 into web-platform-tests:master Dec 26, 2018
@frivoal frivoal deleted the text-3143 branch December 26, 2018 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants