1323743 - Add an API for DevTools to get the list of OpenType Font Variations

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P3)

Description

[Patrick Brosset <:pbro>]

Bug 1302685 implements OpenType Font Variations.

This is cool and very useful. It might be even more useful if DevTools users could see the list of variations (are these called axes?) that a font supports.

This way we could implement a new piece of UI in the inspector's Fonts panel to help people explore fonts.

Therefore, we would like a new API to be implemented on the platform side that lists these variations for a given font.

This was discussed on the dev-platform mailing list: https://groups.google.com/forum/#!topic/mozilla.dev.platform/_FacI6Aw2BQ

Comment 1

[Jonathan Kew [:jfkthame]]

(In reply to Patrick Brosset <:pbro> from comment #0)
> Bug 1302685 implements OpenType Font Variations.
> 
> This is cool and very useful. It might be even more useful if DevTools users
> could see the list of variations (are these called axes?) that a font
> supports.

Yes, they're "variation axes".

Along with the list of variation axes (each of which will have a tag, name, and min/default/max values), we should also expose a list of defined instances. An "instance", in the context of OpenType Variations, is a predefined, named set of variation settings. So, for example, the Skia font has "weight" and "width" axes, which can be varied arbitrarily; but it also has a collection of predefined instances ("Light", "Bold", "Light Extended", "Light Condensed", etc.) that refer to specific settings of the two axes.

I presume we can do this by extending the existing nsIDOMFontFace interface with 'variations' and 'instances' attributes for DevTools to examine.

Comment 2

[Jonathan Kew [:jfkthame]]

To implement this, we'll need to add methods in gfx (probably on gfxFontEntry) to get the variations from a font face. For macOS, we could do this using Core Text APIs, but in due course we should have a platform-independent version based on harfbuzz, so it may not be worth moving forward until that's ready.

Comment 3

[Patrick Brosset <:pbro>]

Jonathan and I recently discussed about the API in an email. Here's an excerpt:

I'd guess maybe you'll want some additional properties exposed on the layout inspector's nsIDOMFontFace interface, to be able to determine the variations available in a given font (and their supported range of values). Does that sound like the kind of thing that would help?
(Offhand, I'm picturing something like an nsIDOMFontVariation interface that has properties like axisName, minValue, defaultValue, maxValue; and nsIDOMFontFace would have a variations property that returns a sequence of this new thing).

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: patch 1 - Add a variationAxes attribute to the nsIDOMFontFace objects exposed to devtools by the layout inspector

Here's a possible way forward: this extends the nsIDOMFontFace interface that the layout/inspector code exposes for DevTools to have a .variationAxes attribute on each face. This will be null if it's not a variation font, but if variations are present it'll be a list of nsIDOMFontVariation objects, each of which has a tag (i.e. the 4-character OpenType tag such as 'wght', which would be used in font-variation-settings to control it), name (human-readable, e.g. 'Weight'), and min/max/default values. ISTM this should provide the info needed for the DevTools Inspector to present a set of sliders or something like that. Obviously, this is just a proposal at this point, and we can refine the API if necessary according to needs.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: patch 2 - Implement support for exposing variationAxes in the macOS font backend

This adds support in the macOS font back-end so that nsIDOMFontFace.variationAxes will actually return something useful on macOS when variation font support is enabled.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: patch 3 - Simple test for the nsIDOMFontFace.variationAxes attribute

And here's a basic chrome mochitest to check that it's working (on macOS only, for now).

Comment 7

[Jonathan Kew [:jfkthame]]

Try build with the above patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=30cbf27a892301e9ab26b5c5ddacdd07496b3742

Comment 8

[Patrick Brosset <:pbro>]

Yay! Thank you Jonathan. This will make it possible for us to start experimenting.

Comment 9

[Brad Werth [:bradwerth]]

I didn't realize Jonathan was able to move this forward. Taking myself off the bug for now.

Comment 10

[Jonathan Kew [:jfkthame]]

The patches here need re-working since bug 1427419 recently landed (but should be significantly simpler as a result -- thanks, Cameron!)

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: patch 1 - Add an API to gfxFontEntry to allow querying the available variation axes in a font face. (No actual implementation yet.)

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: patch 2 - Add a getVariationAxes() method to InspectorFontFace for potential devtools use

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: patch 3 - macOS implementation of the gfxFontEntry::GetVariationAxes method

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: patch 4 - Implement gfxFontEntry::GetVariationAxes for the Linux (fontconfig) backend

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: Add simple chrome mochitest for InspectorFontFace.getVariationAxes()

Comment 16

[Jonathan Kew [:jfkthame]]

(Note that the mochitest is expected to fail on platforms where we don't yet have a variable fonts implementation; we'll need to annotate it appropriately to skip those platforms if we start landing stuff here before all back-ends have been implemented.)

Comment 17

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08a8362a6aad8624f349cf18aec18c3d7220f824

Comment 18

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c42377401ed0e57499b7d0fb6688e1b54ffff68a

Comment 19

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d47d1aa5e29105d991d7d90e1228b9773a64d2c6

Comment 20

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5563086e6e5fe47c8e75f4d07788190ddcc091dc

Comment 21

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c9af78d8cfca93f866d2cea2184d01613aa1e12

Comment 22

[Jonathan Kew [:jfkthame]]

Attachment: patch 5.1 - Factor out a GetEnglishOrFirstName helper to simplify DirectWrite font-name code

This doesn't actually change functionality, it is just to clean up the existing code a bit and make the following patch simpler.

Comment 23

[Jonathan Kew [:jfkthame]]

Attachment: patch 5.2 - Implement gfxFontEntry::GetVariationAxes for the DirectWrite backend

And here's the actual implementation.

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 8942535 [details] [diff] [review]
patch 1 - Add an API to gfxFontEntry to allow querying the available variation axes in a font face. (No actual implementation yet.)

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

::: gfx/thebes/gfxFontEntry.h
@@ +346,5 @@
>      bool SupportsScriptInGSUB(const hb_tag_t* aScriptTags);
>  
> +    // For variation font support, not yet implemented on all platforms
> +    virtual bool HasVariations()
> +    {

Consider rephrasing with "which" to make the meaning clearer (assuming I'm interpreting correctly):
    // For variation font support, which isn't implemented on all platforms yet

Otherwise (with the current wording) it's unclear whether this comment is saying that *just this specific method* isn't implemented on all platforms, vs. whether "variation font support" in general isn't yet implemented on all platforms.  (I'm guessing you mean the latter, because it looks like this specific method gets implemented for all desktop platforms at least by the end of this patch queue.)

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 8942536 [details] [diff] [review]
patch 2 - Add a getVariationAxes() method to InspectorFontFace for potential devtools use

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

r=me, but I think this also requires review from a DOM peer, since it's got a change to a .webidl file.... (I think there's a hg.m.o hook to enforce this, too.)

::: layout/inspector/InspectorFontFace.cpp
@@ +201,5 @@
> +    InspectorVariationAxis axis;
> +    axis.mMinValue.Construct(a.mMinValue);
> +    axis.mMaxValue.Construct(a.mMaxValue);
> +    axis.mDefaultValue.Construct(a.mDefaultValue);
> +    axis.mName.Construct(a.mAxisName);

Nit: it might be better to do these assignments in the same order as the class declarations, so that it's easier to verify that we are indeed assigning each member-var?

(i.e. consider setting axis.mTag first, and then axis.mName, then min/max/defaultValue)

*shrug*, up to you.

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 8942536 [details] [diff] [review]
patch 2 - Add a getVariationAxes() method to InspectorFontFace for potential devtools use

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

::: layout/inspector/InspectorFontFace.cpp
@@ +201,5 @@
> +    InspectorVariationAxis axis;
> +    axis.mMinValue.Construct(a.mMinValue);
> +    axis.mMaxValue.Construct(a.mMaxValue);
> +    axis.mDefaultValue.Construct(a.mDefaultValue);
> +    axis.mName.Construct(a.mAxisName);

One more nit -- can you rename either "mName" or "mAxisName" (in one or the other struct) so that they're consistent?

All the other member-vars have the same names between the two structs, so it's confusing that just this one has an arbitrarily-different name.

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 8942536 [details] [diff] [review]
patch 2 - Add a getVariationAxes() method to InspectorFontFace for potential devtools use

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

::: layout/inspector/InspectorFontFace.cpp
@@ +195,5 @@
> +  if (!mFontEntry->HasVariations()) {
> +    return;
> +  }
> +  AutoTArray<gfxFontVariationAxis,4> axes;
> +  mFontEntry->GetVariationAxes(axes);

Perhaps it'd be worth adding an assertion (or several) here to enforce that HasVariations() and GetVariationAxes() are consistent with each other?   (i.e. the former returns true IFF the latter produces a nonempty array)

(That might mean we can't have the early-return, since we need to call both APIs in order to compare their results. Not sure if that's unfortunate from a perf perspective -- I'd imagine this won't get called a ton, and GetVariationAxes will be pretty fast for a font that has no variations, so it probably doesn't matter?)

Alternately, we could add internal-consistency assertions to every GetVariationAxes() implementation internally -- but it seems cleaner to kill all the birds with one stone at the callsite.

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 8942537 [details] [diff] [review]
patch 3 - macOS implementation of the gfxFontEntry::GetVariationAxes method

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

r=me

(I didn't bother looking up documentation on the various mac APIs that you're using here, but this LGTM assuming they do what they seem to do, and given that I know that you know this stuff)

Comment 29

[Daniel Holbert [:dholbert]]

Comment on attachment 8942538 [details] [diff] [review]
patch 4 - Implement gfxFontEntry::GetVariationAxes for the Linux (fontconfig) backend

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

This seems fine. I reviewed this by comparing it to the similar code in gfxFT2FontBase::SetupVarCoords, basically.

::: gfx/thebes/gfxFcPlatformFontList.cpp
@@ +1049,5 @@
> +        axis.mAxisName.Assign(NS_ConvertUTF8toUTF16(a.name));
> +        aAxes.AppendElement(axis);
> +    }
> +    free(mmVar);
> +//    FT_Done_MM_Var(face->glyph->library, mmVar);

Does this commented-out line want to be removed?

Comment 30

[Daniel Holbert [:dholbert]]

Comment on attachment 8942539 [details] [diff] [review]
Add simple chrome mochitest for InspectorFontFace.getVariationAxes()

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

::: layout/inspector/tests/chrome/test_fontVariationsAPI.xul
@@ +6,5 @@
> +        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +        onload="RunTest();">
> +  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"/>
> +
> +  <!-- test code goes here -->

This ^^ line wants to be removed, I think. (IIRC it's part of the boilerplate produced by the "generate mochitest" script, and it's meant to be replaced with the actual test code. :))

@@ +31,5 @@
> +      var elem, fonts, f;
> +
> +      // We allow for some floating-point "fuzz" on the axis values
> +      var approx = function (a, b) {
> +        return (Math.abs(a - b) < 0.001);

I think you can use the following:
   const epsilon = 0.001;
   [...]
   isfuzzy(a, b, epsilon, "message goes here");

...instead of rolling your own API for this.  See isfuzzy definition here:
https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#315

That's nicer in practice (when there are failures), since it'll automatically print out the expected & actual values when it detects a failure.

@@ +63,5 @@
> +          ok(approx(v.maxValue, 200), "maximum weight");
> +          ok(approx(v.defaultValue, 40), "default weight");
> +          break;
> +        default:
> +          ok(false, "unexpected axis tag");

Two nits:
 (1) It's not the tag that's unexpected, but rather the whole axis
 (2) It would be convenient to actually print the unexpected tag as part of the error message here.

How about:
  "unexpected axis, with tag: '" + v.tag + "'");

@@ +95,5 @@
> +            ok(approx(v.maxValue, 1.30), "maximum width");
> +            ok(approx(v.defaultValue, 1.0), "default width");
> +            break;
> +          default:
> +            ok(false, "unexpected axis tag");

ditto

Comment 31

[Jonathan Kew [:jfkthame]]

Comment on attachment 8942536 [details] [diff] [review]
patch 2 - Add a getVariationAxes() method to InspectorFontFace for potential devtools use

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

r? to :bz for the webidl changes here. It's only extending a [ChromeOnly] interface, so hopefully not too controversial...

Comment 32

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #29)
> ::: gfx/thebes/gfxFcPlatformFontList.cpp
> @@ +1049,5 @@
> > +        axis.mAxisName.Assign(NS_ConvertUTF8toUTF16(a.name));
> > +        aAxes.AppendElement(axis);
> > +    }
> > +    free(mmVar);
> > +//    FT_Done_MM_Var(face->glyph->library, mmVar);
> 
> Does this commented-out line want to be removed?

Oops -- that's been fixed locally, but I failed to upload a refreshed patch. We need to look up FT_Done_MM_Var with dlsym, and fall back to plain ol' free if it's not available (it's a very new addition to the freetype API).

Comment 33

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Kew (:jfkthame) from comment #32)
> Oops -- that's been fixed locally, but I failed to upload a refreshed patch.
> We need to look up FT_Done_MM_Var with dlsym, and fall back to plain ol'
> free if it's not available (it's a very new addition to the freetype API).

We need probably want to do that in the existing (very-similar) gfxFT2FontBase::SetupVarCoords code as well then, right?  (probably in a separate bug)

Comment 34

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #33)
> (In reply to Jonathan Kew (:jfkthame) from comment #32)
> > Oops -- that's been fixed locally, but I failed to upload a refreshed patch.
> > We need to look up FT_Done_MM_Var with dlsym, and fall back to plain ol'
> > free if it's not available (it's a very new addition to the freetype API).
> 
> We need probably want to do that in the existing (very-similar)
> gfxFT2FontBase::SetupVarCoords code as well then, right?  (probably in a
> separate bug)

Right. I have a trivial patch locally that does this; will post it shortly.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8942536 [details] [diff] [review]
patch 2 - Add a getVariationAxes() method to InspectorFontFace for potential devtools use

If the dictionary will always have all the members specified, you could mark them as "required" and not have to deal with the Construct() bits.  So then you would do something like:

  InspectorVariationAxis& axis = *aResult.AppendElement();
  axis.mMinValue = a.mMinValue;

etc.  Just slightly nicer code, I think.

r=me either way.

Comment 36

[Jonathan Kew [:jfkthame]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #35)
> If the dictionary will always have all the members specified, you could mark
> them as "required" and not have to deal with the Construct() bits.  So then
> you would do something like:
> 
>   InspectorVariationAxis& axis = *aResult.AppendElement();
>   axis.mMinValue = a.mMinValue;
> 
> etc.  Just slightly nicer code, I think.

Thanks, I've gone with this suggestion.

(In reply to Jonathan Kew (:jfkthame) from comment #34)
> (In reply to Daniel Holbert [:dholbert] from comment #33)
> > (In reply to Jonathan Kew (:jfkthame) from comment #32)
> > > Oops -- that's been fixed locally, but I failed to upload a refreshed patch.
> > > We need to look up FT_Done_MM_Var with dlsym, and fall back to plain ol'
> > > free if it's not available (it's a very new addition to the freetype API).
> > 
> > We need probably want to do that in the existing (very-similar)
> > gfxFT2FontBase::SetupVarCoords code as well then, right?  (probably in a
> > separate bug)
> 
> Right. I have a trivial patch locally that does this; will post it shortly.

Just FTR, that fixup is now bug 1433402.

Comment 37

[Jonathan Kew [:jfkthame]]

Note that on Win10, the test here passes for me locally (modulo the axis names being missing until bug 1433098 is fixed), but it fails on tryserver (no variation axes are found). I believe this is because we're not running the latest Win10 update, see bug 1433438; as a result, I'm annotating the test to be skipped on Windows for the time being. (Checking navigator.oscpu does not give sufficiently precise version information for the test to be able to tell whether to expect variations to work.)

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/625b879e0a93
patch 1 - Add an API to gfxFontEntry to allow querying the available variation axes in a font face. (No actual implementation yet.) r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a2c3bc3eb23
patch 2 - Add a getVariationAxes() method to InspectorFontFace for potential devtools use. r=dholbert,bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7374076bd97
patch 3 - macOS implementation of the gfxFontEntry::GetVariationAxes method. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4a0e7c496c6
patch 4 - Implement gfxFontEntry::GetVariationAxes for the Linux (fontconfig) backend. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/09c06c7df0d1
patch 5.1 - Factor out a GetEnglishOrFirstName helper to simplify DirectWrite font-name code. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe3a9f1031a7
patch 5.2 - Implement gfxFontEntry::GetVariationAxes for the DirectWrite backend. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/49309c53740f
Add simple chrome mochitest for InspectorFontFace.getVariationAxes(). r=dholbert

Comment 39

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/625b879e0a93
https://hg.mozilla.org/mozilla-central/rev/4a2c3bc3eb23
https://hg.mozilla.org/mozilla-central/rev/e7374076bd97
https://hg.mozilla.org/mozilla-central/rev/a4a0e7c496c6
https://hg.mozilla.org/mozilla-central/rev/09c06c7df0d1
https://hg.mozilla.org/mozilla-central/rev/fe3a9f1031a7
https://hg.mozilla.org/mozilla-central/rev/49309c53740f

Comment 40

[Patrick Brosset <:pbro>]

Hey Jonathan, I've tested this API (and getVariationInstances). It works great!
I'm wondering whether we support custom axes though. The API does not seem to return axes when they are custom. I'm testing on https://www.axis-praxis.org/playground/horse/ which has a font with an axis called TIME. But I can't get to it with getVariationAxes().
Do we have support for custom axes in Firefox yet?

Comment 41

[Jonathan Kew [:jfkthame]]

Hmm, that's a bit surprising - I would have expected it to work. Will investigate. Which platform are you using, and have you tried others?

Comment 42

[Daniel Holbert [:dholbert]]

Patrick is on Mac -- though, to be clear, this isn't specific for this devtools API -- it seems like we maybe don't support custom axes *at all*?

On chrome (I tested Linux and Mac), https://www.axis-praxis.org/playground/horse/ moves the horse, via a CSS animation on this custom axis. On Firefox (I tested Linux and Mac), the horse does not move.

Comment 43

[Jonathan Kew [:jfkthame]]

That's odd. The horse moves for me (with the variation-font prefs set, obviously) on all platforms: Mac (Sierra, 10.12), Linux (Ubuntu 16.04), Win10 (Insider) and Android.

Comment 44

[Daniel Holbert [:dholbert]]

Never mind, we sorted it out in IRC - we hadn't set all the necessary prefs. Thanks!