Closed Bug 1435692 Opened 6 years ago Closed 6 years ago

Implement the font-optical-sizing property

Categories

(Core :: Layout: Text and Fonts, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 7 obsolete files)

21.87 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
4.09 KB, patch
Details | Diff | Splinter Review
1.75 KB, patch
Details | Diff | Splinter Review
9.49 KB, patch
emilio
: review+
Details | Diff | Splinter Review
172.73 KB, patch
Details | Diff | Splinter Review
CSS Fonts 4 introduces font-optical-sizing[1], with initial value 'auto'.

For fonts that have an Optical Size variation axis (e.g. see [2], [3]), we should use font-size to determine the appropriate setting of the opsz axis.

The 'none' value of font-optical-sizing would disable this behavior, leaving the font resource using its default opsz instance.

(If 'opsz' is specified via the low-level font-variation-settings property, it would still override this.)


[1] https://www.w3.org/TR/css-fonts-4/#font-optical-sizing-def
[2] https://www.axis-praxis.org/specimens/voto-serif
[3] https://www.axis-praxis.org/specimens/zeitung-pro
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Note that this only adds the property to the old Gecko style system, not to Stylo. So there's still another piece to come.
Attachment #8948513 - Flags: review?(jwatt)
This passes for me locally, when stylo is DISabled. We won't actually be able to run it in automation on most platforms yet, as our test systems (at least macOS and Windows) aren't running recent enough OS versions to have full variation-font support, so test coverage will initially be poor.
Attachment #8948514 - Flags: review?(jwatt)
Here's an initial attempt to add the property to stylo. This seems to work, but I'm guessing (from looking at the other font-* properties) that it should really be using helpers.single_keyword_system ... but I didn't find any docs about how to make that work. Also, it presumably first needs to land in servo, and then get merged to m-c, rather than landing directly here. Any chance you could take on this part and do it properly?
Attachment #8948516 - Flags: feedback?(emilio)
(In reply to Jonathan Kew (:jfkthame) from comment #4)
> Created attachment 8948516 [details] [diff] [review]
> (WiP) - Add font-optical-sizing property to stylo
> 
> Here's an initial attempt to add the property to stylo. This seems to work,
> but I'm guessing (from looking at the other font-* properties) that it
> should really be using helpers.single_keyword_system ... but I didn't find
> any docs about how to make that work. Also, it presumably first needs to
> land in servo, and then get merged to m-c, rather than landing directly
> here. Any chance you could take on this part and do it properly?

That works. How is this supposed to work with all the system font stuff? That's the only thing that make most of the font properties kinda awkward. I assume since you also don't have any particular handling on Gecko, this is just fine?

Happy to do / land the Servo-side patch tomorrow if you want, yeah :).
Flags: needinfo?(jfkthame)
Attachment #8948516 - Flags: feedback?(emilio) → feedback+
Comment on attachment 8948513 [details] [diff] [review]
patch 2 - Implement the font-optical-sizing property in the Gecko style system, to control whether optical size is automatically applied

Cancel review on this for now, it needs an update before it's ready.
Attachment #8948513 - Flags: review?(jwatt)
Now including proper connection to the font shorthand, which was missing from the original patch.
Attachment #8948631 - Flags: review?(jwatt)
Attachment #8948513 - Attachment is obsolete: true
(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> That works. How is this supposed to work with all the system font stuff?
> That's the only thing that make most of the font properties kinda awkward. I
> assume since you also don't have any particular handling on Gecko, this is
> just fine?

Actually, AFAICT from checking the spec (and also just because it makes sense) I think font-optical-sizing should be reset by the system font keywords, and by the font shorthand in general. The Gecko patch above failed to handle that, so I've updated it; and the Servo patch will need some kind of update as well (I'm guessing that's where single_keyword_system comes in?), but I'm not sure how that works...
Pushed a try run with these patches at https://treeherder.mozilla.org/#/jobs?repo=try&revision=73ba191734af2e63ef3e1cc0f97b611494bbe63a; I'm expecting this to fail when stylo is enabled, because of the incompleteness of the Servo patch here.
Flags: needinfo?(jfkthame)
Here's my attempt to update this, by copying how font-kerning is handled; seems to be working in basic tests, at least. Could you take it over from here? Thanks.
Attachment #8948634 - Flags: feedback?(emilio)
Attachment #8948516 - Attachment is obsolete: true
And a further try run, with the updated stylo patch, to see how this goes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=50947df8928f78089de28c4ac5cf3e1db2f1f9ce.
Comment on attachment 8948631 [details] [diff] [review]
patch 2 - Implement the font-optical-sizing property in the Gecko style system, to control whether optical size is automatically applied

Hmm, the try run is hitting crashes (even with stylo disabled), so this is clearly broken in some way.
Attachment #8948631 - Flags: review?(jwatt)
Comment on attachment 8948634 [details] [diff] [review]
(WiP) - Add font-optical-sizing property to stylo

Review of attachment 8948634 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the lag on this one. This looks good off-hand :)

::: servo/components/style/properties/longhand/font.mako.rs
@@ +181,5 @@
> +                                gecko_ffi_name="mFont.opticalSizing",
> +                                gecko_constant_prefix="NS_FONT_OPTICAL_SIZING",
> +                                animation_value_type="none",
> +                                flags="APPLIES_TO_FIRST_LETTER APPLIES_TO_FIRST_LINE APPLIES_TO_PLACEHOLDER",
> +                                spec="https://www.w3.org/TR/css-fonts-4/#font-optical-sizing-def")}

nit: https://drafts.csswg.org/css-fonts-4/
Attachment #8948634 - Flags: feedback?(emilio) → feedback+
OK, I think this finally handles the new property OK. It gets a little messy because of this being behind a pref, which then also affects the font shorthand, so eventually when that pref is removed we'll be able to clean up a bit.
Attachment #8948961 - Flags: review?(jwatt)
Attachment #8948631 - Attachment is obsolete: true
To keep devtools tests happy, we also need to refresh their property db.
Attachment #8948962 - Flags: review?(jwatt)
Comment on attachment 8948512 [details] [diff] [review]
patch 1 - Add support in gfx for automatic application of the optical size axis in variation fonts that support it

Review of attachment 8948512 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/src/nsFont.h
@@ +90,5 @@
>  
> +  // Whether automatic optical sizing should be applied to variation fonts
> +  // that include an 'opsz' axis
> +  uint8_t opticalSizing = NS_FONT_OPTICAL_SIZING_AUTO;
> + 

Trim white space on blank line.
Attachment #8948512 - Flags: review?(jwatt) → review+
Attachment #8948514 - Flags: review?(jwatt) → review+
Attachment #8948961 - Flags: review?(jwatt) → review+
Comment on attachment 8948962 [details] [diff] [review]
patch 3 - (Auto-generated) Run "mach devtools-css-db" to update devtools property database

Review of attachment 8948962 [details] [diff] [review]:
-----------------------------------------------------------------

Tsk, tsk. Someone has been forgetting to do this I see. :)
Attachment #8948962 - Flags: review?(jwatt) → review+
Attachment #8948512 - Attachment is obsolete: true
There's one remaining issue on the Stylo side here: the current patches fail the tests for animating the `font` shorthand (e.g. when running dom/smil/test/test_smilCSSFromTo.xhtml; there's also a reftest that fails as a result of this).

I believe this is somehow related to the `font-optical-sizing` property not being animatable (i.e. the spec says "Animatable: no", which I translated to animation_value_type="none" in components/style/properties/longhand/font.mako.rs); yet this property is connected to the `font` shorthand, in that `font` resets `font-optical-sizing` to its initial value. This causes stylo to fail when it tries to animate from "font: 10px serif" to "font: 30px serif", and similar cases. (I guess it expands the shorthand to all its longhands, then tries to animate each of those; but `font-optical-sizing` can't be animated and so it fails?)

This doesn't happen with the old style system; running the test with --setpref layout.css.servo.enabled=false, it all passes.

I confirmed that changing the setting for `font-optical-sizing` to animation_value_type="discrete" in longhand/font.mako.rs prevents the failure; but this would be incorrect according to the spec.

:emilio, can you take a look at this? I don't really know where to look or how to go about fixing this.
Flags: needinfo?(emilio)
(In reply to Jonathan Kew (:jfkthame) from comment #19)
> There's one remaining issue on the Stylo side here: the current patches fail
> the tests for animating the `font` shorthand (e.g. when running
> dom/smil/test/test_smilCSSFromTo.xhtml; there's also a reftest that fails as
> a result of this).
> 
> I believe this is somehow related to the `font-optical-sizing` property not
> being animatable (i.e. the spec says "Animatable: no", which I translated to
> animation_value_type="none" in
> components/style/properties/longhand/font.mako.rs); yet this property is
> connected to the `font` shorthand, in that `font` resets
> `font-optical-sizing` to its initial value. This causes stylo to fail when
> it tries to animate from "font: 10px serif" to "font: 30px serif", and
> similar cases. (I guess it expands the shorthand to all its longhands, then
> tries to animate each of those; but `font-optical-sizing` can't be animated
> and so it fails?)

Hmm, that's weird. I suspect what the old style system is doing is effectively discrete animation... But anyway I cannot reproduce what you say, if I switch font-family to use animation_value_type="none" (and we probably should per spec), then a test-case like the following:

<!doctype html>
<style>
@keyframes foo {
  from { font: 10px sans; }
  to { font: 20px serif; }
}
</style>
<div style="animation: 1s foo infinite">Wow fonts</div>

Still animates the font-size, which I think is the expected behavior. I'm doing a build with your patches now to see the failure.
So I took a look at the SMIL test issue. I think it's fine to land with discrete animation for now.

Unfortunately I don't know enough about our SMIL code. I know the style code is producing the expected results: ParseProperty is returning the correct set of declarations, and GetAnimationValues does generate 16 animation values (instead of 17 if we use discrete).

The test is doing <animate from="10px serif" to="30px serif">, then inspects the computed value of the animated element.

Brian, any chance you could take a quick look at this, or point where to look at? See comment 19 and comment 20, and:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=928718de43f5a2406ddad23459ec4f5b9c3241b5&selectedJob=161749095
Flags: needinfo?(emilio) → needinfo?(bbirtles)
(In reply to Jonathan Kew (:jfkthame) from comment #19)
> There's one remaining issue on the Stylo side here: the current patches fail
> the tests for animating the `font` shorthand (e.g. when running
> dom/smil/test/test_smilCSSFromTo.xhtml; there's also a reftest that fails as
> a result of this).
> 
> I believe this is somehow related to the `font-optical-sizing` property not
> being animatable (i.e. the spec says "Animatable: no", which I translated to
> animation_value_type="none" in
> components/style/properties/longhand/font.mako.rs);

The spec is wrong here. It should be discretely animatable. Basically, everything is discretely animatable. The only things that are not are animation-related properties (to avoid awkward recursive behavior) and 'display' (again, recursive behavior), although we also turn off animation for a few Moz-only internal properties.

> I confirmed that changing the setting for `font-optical-sizing` to
> animation_value_type="discrete" in longhand/font.mako.rs prevents the
> failure; but this would be incorrect according to the spec.

It sounds like the fix here is just to fix the spec (and I think we can land without waiting on the spec change since there is a CSSWG resolution that all properties are discretely animatable by default).
Flags: needinfo?(bbirtles)
OK, thanks. So in the light of https://www.w3.org/TR/css-transitions-1/#animatable-properties and in particular "issue 1" there, and the proposal to update CSS specs to say "Animatable: discrete" or similar in place of "no", I guess this is primarily a misunderstanding on my part. (And the spec update needed is part of a global update to CSS specs in the light of that resolution).

I'll give this another try run with this adjustment, and see how things look then.
Ugh... I think I still need some help here, after changing the animation_value_type; we still have some oranges to kill. :(

See for example the failure in test_system_font_serialization.html at [1]. This test passes when run with stylo disabled; and it passes when run with stylo enabled and font-variations enabled; but with stylo enabled and font-variations disabled (i.e. the current default configuration), it fails.

The failure shows

got
"font-family: Helvetica; font-feature-settings: inherit; font-kerning: inherit; font-language-override: inherit; font-optical-sizing: inherit; font-size-adjust: inherit; font-size: inherit; font-stretch: inherit; font-style: inherit; font-variant: inherit; font-weight: inherit; line-height: inherit;"

expected
"font-family: Helvetica; font-feature-settings: inherit; font-kerning: inherit; font-language-override: inherit; font-size-adjust: inherit; font-size: inherit; font-stretch: inherit; font-style: inherit; font-variant: inherit; font-weight: inherit; line-height: inherit;"

with the difference being that stylo has included font-optical-sizing:inherit in the list, even though when font-variations are disabled, that property is supposed to be preffed off.

So it looks to me like it fails to appropriately exclude the preffed-off longhand property from the serialization of the system font shorthand. I guess this needs some kind of additional checking of the pref, probably in properties/shorthand/font.mako.rs, but I'm not sure how to go about it properly.

(I'm guessing that a fix for this may also resolve the failures in test_bug377947.html and test_value_storage.html. Again, these pass with stylo disabled, or with font-variations enabled, and relate to serialization of the font shorthand.)

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=a785479024a4a4eaad732715b22a2435a575c42a&selectedJob=162116265
Flags: needinfo?(emilio)
You should be able to cherry-pick https://hg.mozilla.org/try/rev/11ed5568d69f2484daf222f679015a5e848a833f to fix that. I'll submit that later today when try is green :).
Flags: needinfo?(emilio)
Hmm, needs a couple more tweaks, I think, for variables and CSS wide keywords.
Yes, this doesn't entirely seem to fix things: I still get test failures with the optical-sizing patches when stylo is enabled (and font-variations are not), and font-optical-sizing is showing up in the system font serialization. :(
(In reply to Jonathan Kew (:jfkthame) from comment #27)
> Yes, this doesn't entirely seem to fix things: I still get test failures
> with the optical-sizing patches when stylo is enabled (and font-variations
> are not), and font-optical-sizing is showing up in the system font
> serialization. :(

So, there's good and bad news... The good news is that I know what the problem is, and I know how to fix it. The bad news is that fixing it is non-trivial at all, and probably requires discussing a bit with other people that know the parsing and serialization code.

The issue is that Stylo assumes that if a given shorthand is available, all its longhands are. That seems a reasonable assumption off-hand, but it has terrible implications, specially in presence of the `all` shorthand.

In particular, if you go with nightly with stylo enabled and do something like the following in the web console:

> document.body.style.all = "inherit";
> document.body.style.fontFamily = "10px"; // Doesn't matter, just any property so that we need to expand "all" to its longhands.
> document.body.style.cssText

You'll see disabled properties in there, including, for example, font-variation-settings.

Now, regarding how to fix this... I thought it'd be enough to check whether the longhands are available for content at parse time, and avoid expanding those longhands. That'd fix that particular test...

But that is obviously not enough, because then, when serializing, we stash all the longhands in a struct, and call ToCss on that... Which means we'll never serialize the longhand!

Now, fixing it requires a fair amount of thought, specially wrt how to rework the serialization so that it allows it.

I'll file another bug for this to avoid this bug derailing a lot.
See Also: → 1438234
(In reply to Jonathan Kew (:jfkthame) from comment #23)
> OK, thanks. So in the light of
> https://www.w3.org/TR/css-transitions-1/#animatable-properties and in
> particular "issue 1" there, and the proposal to update CSS specs to say
> "Animatable: discrete" or similar in place of "no", I guess this is
> primarily a misunderstanding on my part. (And the spec update needed is part
> of a global update to CSS specs in the light of that resolution).

Right, there's a resolution to make all properties transitionable apparently (I wasn't there when it was made) but I'm fairly confident that would break far too much content and will never be actioned. However, for CSS *animations* and for other types of declarative animations (e.g. Web animations) we do make all properties animatable by default.

The editing to make specs say: "Animation type: discrete" is something I need to do. I tried to do it once but I couldn't work out how to edit CSS Values & Units correctly (so that we have somewhere to link to for the animation types). I plan to follow up on this at the next CSS WG F2F.
Attachment #8948962 - Attachment is obsolete: true
I think this should be the final version of the stylo patch here; you've basically seen it (or written parts of it) already. As we'll need to coordinate landing the gecko and servo patches here, can you take care of that once you're happy with the servo side of it?
Attachment #8955575 - Flags: review?(emilio)
Attachment #8948634 - Attachment is obsolete: true
Updated reftest patch with annotations for the various cases where things don't yet pass; carrying over r=jwatt. Let's see if tryserver is happy with all this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca76c4569dabbd330d150349bd0938c945c22775.
Attachment #8948514 - Attachment is obsolete: true
Comment on attachment 8955575 [details] [diff] [review]
patch 4 [with lots of input from Emilio] - Add font-optical-sizing property to stylo

r=me, though maybe Xidorn could take a look at the parts that were suggested by me :). They're mostly trivial so probably doesn't matter.

Sorry again that this got blocked for so long!
Attachment #8955575 - Flags: review?(emilio) → review+
(In reply to Jonathan Kew (:jfkthame) from comment #31)
> Created attachment 8955575 [details] [diff] [review]
> patch 4 [with lots of input from Emilio] - Add font-optical-sizing property
> to stylo
> 
> I think this should be the final version of the stylo patch here; you've
> basically seen it (or written parts of it) already. As we'll need to
> coordinate landing the gecko and servo patches here, can you take care of
> that once you're happy with the servo side of it?

Sure, will take care of that :)
Flags: needinfo?(emilio)
Tryserver looks OK: as per discussion in #developers this evening, the TV orange is spurious and can be ignored. The other oranges on the try push look like unrelated intermittents.
https://github.com/servo/servo/pull/20187 for the servo bits, will land the Gecko ones as soon as those land.
Flags: needinfo?(emilio)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/65a8e5654ef5
patch 1 - Add support in gfx for automatic application of the optical size axis in variation fonts that support it. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/93cd740dde96
patch 2 - Implement the font-optical-sizing property in the Gecko style system, to control whether optical size is automatically applied. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/5116edeac259
patch 3 - (Auto-generated) Run "mach devtools-css-db" to update devtools property database. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/eb917fd39e66
Add a reftest for font-optical-sizing with OpenType Variation fonts. r=jwatt
Depends on: 1457417
Depends on: 1458004
No longer depends on: 1458004
I have documented this property:

New page added: https://developer.mozilla.org/en-US/docs/Web/CSS/font-optical-sizing
Property data added to our data repo: https://github.com/mdn/data/pull/227
Browser support info added to BCD repo: https://github.com/mdn/browser-compat-data/pull/2002
Note added to Fx60 rel notes: https://developer.mozilla.org/en-US/Firefox/Releases/60#CSS

Can you have a look and let me know if this looks OK to you? Thanks.

I did have a question as well. I've not managed to write a working example for this property. I tried writing something simple that used the same font file you were using in your reftest — VotoSerifGX.latin1.ttf. But I found a huge problem — every time I try to do anything with this file, the Finder app on my Mac immediately crashes!

I tried everything, and eventually gave up.

Have you have any such troubles with this font? Do you know of another example font I could use?
Flags: needinfo?(jfkthame)
Hmm, I've not had that problem, AFAIK. What macOS version are you on?

There are a number of fonts in the gallery at https://v-fonts.com/ that include an optical-size axis, but most are commercial. The only other free-licensed one I noticed was Amstelvar (see https://v-fonts.com/P25/#amstelvar); maybe that would be useful?

(There's also Sitka, used for the optical-sizing example on Microsoft's demo page https://developer.microsoft.com/en-us/microsoft-edge/testdrive/demos/variable-fonts/, but again, it's not under a free license.)

One other thing to note is that currently we have an issue (bug 1457417) with optical-size on macOS (only; should be OK on other platforms) whereby fonts that have an optical size axis may render incorrectly when the axis is not explicitly set. When I view the Amstelvar example on v-fonts.com, the rendering is incorrect (manifested as erratic glyph spacing) _until_ I touch the optical size slider, which applies a specific 'opsz' value (overriding the automatic behavior); then it looks correct.

Similarly, the Sitka example on the microsoft demo page renders the "font-optical-sizing:none" sample incorrectly for me.

So you may want to hold off on trying to create examples until that bug is resolved.

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #39)
> I have documented this property:
> 
> New page added:
> https://developer.mozilla.org/en-US/docs/Web/CSS/font-optical-sizing
> Property data added to our data repo: https://github.com/mdn/data/pull/227
> Browser support info added to BCD repo:
> https://github.com/mdn/browser-compat-data/pull/2002
> Note added to Fx60 rel notes:
> https://developer.mozilla.org/en-US/Firefox/Releases/60#CSS

Note that font-optical-sizing is behind the layout.css.font-variations.enabled pref, so it is not enabled by default until Firefox 62.
Flags: needinfo?(jfkthame)
You need to log in before you can comment on or make changes to this bug.