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/css-fonts/matching/ tests refresh to pass on FF, Safari? #22355

Open
drott opened this issue Mar 20, 2020 · 13 comments
Open

css/css-fonts/matching/ tests refresh to pass on FF, Safari? #22355

drott opened this issue Mar 20, 2020 · 13 comments
Labels

Comments

@drott
Copy link
Contributor

@drott drott commented Mar 20, 2020

Judging from the results pages css/css-fonts/matching/ is only passing on Chromium based browsers.

In the process of fixing the issue raised in https://arrowtype.github.io/vf-slnt-test/ (application of slnt values needs sign inversion from CSS to OpenType values, Chrome bug, FF bug, WebKit bug) I am now updating the test font so that its slnt axis is turned around and the bar for the test glyph is short at 90, and long at -90 axis values, which means it's short at -90 deg CSS value, and long at 90 deg CSS value.

The test setup is perhaps a little complicated, but I still find it useful to test font matching and application of variable style values, and I would expect Safari and FF to be compatible with the principles, but somehow the test does not pass for those.

@jfkthame, @litherum - would you be ready to help by reviewing the test and providing some insights? Is there something we can do for this test to work better in your systems? Or are there perhaps issues with the test, or would you have other improvement suggestions? Thanks in advance.

@jfkthame
Copy link
Contributor

@jfkthame jfkthame commented Mar 20, 2020

Hi @drott - I'll try to take a look and figure out why this isn't working in Firefox, at least, but it may take a bit of time to get to the bottom of it. I thought we pretty much had this stuff working, so definitely want to understand what the issue is here.

Meanwhile, I do see one minor issue with the variabletest_matching.ttf test font: I get warnings in the Firefox web console such as

downloadable font: fvar: Axis nameID out of range (font-family: "variable_axes" style:normal weight:700..800 stretch:100 src index:0) source: ...

because the font has axis name IDs 200, 201, etc., but the spec says that

The name ID must be greater than 255 and less than 32768.

I don't believe that would affect the behavior we're seeing here, but it'd still be nice to correct the font so that it strictly follows the spec here.

@jfkthame
Copy link
Contributor

@jfkthame jfkthame commented Mar 22, 2020

Another issue with the test font is that it lacks a 'STAT' table, which the Microsoft spec says is "required in all variable fonts".

Again, I don't currently have any evidence this is causing the problem here, but it'd be good to fix it for spec compliance anyway.

@jfkthame
Copy link
Contributor

@jfkthame jfkthame commented Mar 22, 2020

@drott: OK, looking into Firefox's behavior here, it seems to be loading the correct font face, but it's not applying the expected variation-axis values to it. The Fonts panel in the developer tools reveals that we're not recognizing the presence of axes in the font at all.

Turns out this is because the 'fvar' table in the font doesn't define any instances (instanceCount in the 'fvar' header is zero), and we don't handle that properly. I guess most "real" fonts out there do have at least a "default" instance defined, and so we hadn't had this reported as a bug, but it is in fact valid to have no instance records. (There's still an implicit "default instance", fwiw, but it doesn't need to be recorded in the table.) So I'll file a Gecko bug on this.

With that fixed, the font appears to work as expected in Firefox (based on cursory testing at this point), but the testcase as currently found in WPT will fail because the reference renderings are wrong: they don't account for the sign inversion between CSS oblique and OpenType slnt. I assume you're intending to update the font and/or test files to account for this.

If you could add an instance record to the 'fvar' table, I believe the font would then work properly in Firefox. But in any case we'll be fixing that bug in Gecko shortly.

@jfkthame
Copy link
Contributor

@jfkthame jfkthame commented Mar 22, 2020

FTR, I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1624225 to fix the Firefox bug here.

@drott
Copy link
Contributor Author

@drott drott commented Mar 23, 2020

@jfkthame - thanks for looking into it and sending your feedback. I'll fix the STAT and name ID issues that you found - thank you! I can also try to add an instance record.

The oblique/sign inversion was indeed the trigger for me to update this font and look closer at this test. The font should be fixed in this regard after c98c762 - can you confirm that the test passes in Firefox after this change in the font file?

@jfkthame
Copy link
Contributor

@jfkthame jfkthame commented Mar 23, 2020

@drott Thanks for updating the font. Yes, with that fix it passes -- almost! It's clear that the font-matching behavior and application of variation-axis values is working as expected.

There's one remaining issue: synthetic bold. When the requested font-weight property is a bold value (600 or more), but the selected face is not bold, Firefox applies a synthetic-bold effect to the glyphs, resulting in slightly longer "bars" in the test rendering compared to the reference in those cases.

It seems Chrome doesn't apply synthetic-bolding to @font-face fonts, or at least not in this case. I don't think the spec gives any clear ruling on this -- synthetic-bold is something UAs are allowed but not required to do. So to avoid this issue interfering with the test, I'd suggest adding font-synthesis: none; to the stylesheet; then Firefox won't attempt to embolden anything, and the test passes.

@drott
Copy link
Contributor Author

@drott drott commented Mar 23, 2020

@jfkthame A set of changes are in #22392 - added and instance, a STAT table, and moved the name IDs, added encoding headers and added font-synthesis: none; - thanks in advance for taking a look.

In FF nightly, I get: "Error in parsing value for ‘font’. Declaration dropped. 4 style-ranges-over-weight-direction.html" for example. Is this intentional? Am I still missing some syntax changes?

@jfkthame
Copy link
Contributor

@jfkthame jfkthame commented Mar 23, 2020

In FF nightly, I get: "Error in parsing value for ‘font’. Declaration dropped. 4 style-ranges-over-weight-direction.html" for example. Is this intentional? Am I still missing some syntax changes?

I'm puzzled by this, I don't see anything like that in Nightly here, and I don't see anything in the test that seems like it could cause it.

What platform(s) are you trying it on? What exactly are you doing -- opening the test file manually, or using the wpt test runner, or....?

@jfkthame
Copy link
Contributor

@jfkthame jfkthame commented Mar 23, 2020

Oh, one other thing you might want to look into: I notice that if I change the order of a range descriptor, replacing for example

font-style: oblique -67.5deg -45deg;

with

font-style: oblique -45deg -67.5deg;

the test no longer passes in Chrome canary, while it does pass in Firefox. In other words, Chrome doesn't allow the range to be expressed in decreasing rather than increasing order. (I haven't tested in detail to see whether it rejects the descriptor entirely, or just collapses the intended range to one of the ends.)

However, the spec explicitly says that

User agents must swap the computed value of the startpoint and endpoint of the range in order to forbid decreasing ranges.

so such a range should actually be accepted. (Although this phrasing seems a little awkward.)

(I guess this is probably motivated primarily by the possibility of negative slant, where writing the range arithmetically "backwards" as oblique -10deg -20deg could be quite a natural thing to do -- intuitively, it's increasing the amount of slant, even though the value is getting smaller.)

@drott
Copy link
Contributor Author

@drott drott commented Mar 23, 2020

I'm puzzled by this, I don't see anything like that in Nightly here, and I don't see anything in the test that seems like it could cause it.

What platform(s) are you trying it on? What exactly are you doing -- opening the test file manually, or using the wpt test runner, or....?

That was when opening the test file from disk when using Nightly on Mac - but now I saw an update indicator on the Nightly and after restarting for the update I no longer see this issue.

@drott
Copy link
Contributor Author

@drott drott commented Mar 23, 2020

[...] test no longer passes in Chrome canary, while it does pass in Firefox. In other words, Chrome doesn't allow the range to be expressed in decreasing rather than increasing order.

Thanks for the heads-up, filed as Chromium bug #1063867.

@drott drott closed this in #22392 Mar 24, 2020
drott added a commit that referenced this issue Mar 24, 2020
Concluding from the discussion in #22355 name ids should have values
greater than 255, and the font should have a STAT table. The name id
issue is something the Firefox prints out as an error on the command
line. Add a STAT table, add a named instance for the regular instance,
and move name ids in this change. Also add encoding headers in tests and
references, and suppress font-synthesis as this is needed to help the
test pass on Firefox.

Fixes #22355 for Firefox.
@drott drott reopened this Mar 24, 2020
@drott
Copy link
Contributor Author

@drott drott commented Mar 24, 2020

I reopened this as it still does not seem to pass in Safari - if you have any feedback, @litherum - I'd be happy to hear what we can do in this regard.

@drott
Copy link
Contributor Author

@drott drott commented Mar 25, 2020

[...] test no longer passes in Chrome canary, while it does pass in Firefox. In other words, Chrome doesn't allow the range to be expressed in decreasing rather than increasing order.

Thanks for the heads-up, filed as Chromium bug #1063867.

And this is now fixed.

@foolip foolip added the css-fonts label May 13, 2020
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.

3 participants