1280162 - Add OpenType features to Font Inspector

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Sebastian Zartner [:sebo]]

As of bug 1247723 comment 0 the Font Inspector should expose UI for OpenType features like ligatures and smallcaps.

Sebastian

Comment 1

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 2

[Patrick Brosset <:pbro>]

Gabriel (:gl) and I discussed about this recently and agreed to start looking into it more soon.

To me, this Font Inspector enhancement would be a way for web authors to see which features exist in the font face they're currently using, and would also allow to toggle them on and off. I can basically see a list of checkboxes you'd be able to check and see the result live in the page. Behind the scenes, this would set the right CSS properties to the element: font-feature-settings, font-variant-ligatures, font-variant-caps, font-variant-east-asian, font-variant-alternates, font-variant-numeric or font-variant-position.

Jonathan, would a platform API be needed for DevTools to retrieve the list of features available?

Comment 3

[Jonathan Kew [:jfkthame]]

Yes, I imagine we'd want to provide a platform API for this. In principle, for a webfont I guess you could just grab the raw font data and parse it (IIRC, there are existing OpenType-parsing JS libs) to find all this stuff, but that's a bunch of work and there's already code in the platform to do it. And anyhow, for installed fonts on the platform, I don't think there'd be any way to get at the raw font data yourself.

So I think it'd make more sense to expose this via nsIDOMFontFace, similarly to the proposal for variations in bug 1323743. Then DevTools can just get the info from there, regardless of whether it's dealing with a platform font or a webfont resource.

What's actually present in an OpenType font is rather more complex than just a list of features: they are organized under a hierarchy of Script / Language System / Feature, so that a font may implement different features for different scripts. E.g. a font that supports both Latin and Arabic might implement 'smcp' (Small Caps) for Latin, but not for Arabic; meanwhile, for Arabic script it probably supports 'rlig' (Required Ligatures), which is unlikely to be present in Latin. A font may also have multiple "language systems" within any given script, so that (for example) within Latin script, Turkish may have different features from English.

Whether to directly expose this potentially-extensive hierarchy in DevTools is another matter; it's probably not helpful at the level of web authoring. But if we make nsIDOMFontFace expose the OpenType model, you can always "flatten" that to just create a single list of "all the features this font implements", which is most likely what you want to show the user.

The question I have is whether it's more sensible to do this as a tree of objects that corresponds to the OpenType structure, such as

  nsIDOMFontFace
    scripts: list of nsIDOMFontScript

  nsIDOMFontScript:
    tag: string
    langSystems: list of nsIDOMFontLangSystem

  nsIDOMFontLangSystem:
    tag: string
    features: list of nsIDOMFontFeature (or just of feature tags as strings?)

  nsIDOMFontFeature:
    tag: string
    // anything else?

or to invert this to something more like

  nsIDOMFontFace
    features: list of nsIDOMFontFeature

  nsIDOMFontFeature
    tag: string
    scripts: list of nsIDOMFontFeatureScript for which this feature is defined

  nsIDOMFontFeatureScript
    tag: string
    langSystems: list of langSystem tags implemented by the script (may be only "default")

which might simplify things on the DevTools side, if the primary presentation is as a unified list of available features.

Comment 4

[Jonathan Kew [:jfkthame]]

I started looking at this a bit more, and have an initial set of patches for consideration. In the end, I decided to go with a relatively simple API, just adding a `features` attribute to nsIDOMFontFace, which will be a list of nsIDOMFontFeature. Each feature in turn has 3 attributes, featureTag, scriptTag, and languageSystemTag, each of which will be a 4-character string as found in the OpenType registry.[1]

(This API is of course open to revision if some other way of exposing the data seems more useful.)

Note that this means any given feature such as "Common Ligatures" (tag: 'liga') is likely to occur multiple times in the list, for each combination of script and language system the font supports; e.g. I have a testcase using DejaVu Sans, where the resulting list includes

  ["liga", "arab", "KUR "],
  ["liga", "arab", "SND "],
  ["liga", "arab", "URD "],
  ["liga", "arab", "dflt"],
  ["liga", "latn", "ISM "],
  ["liga", "latn", "KSM "],
  ["liga", "latn", "LSM "],
  ["liga", "latn", "MOL "],
  ["liga", "latn", "NSM "],
  ["liga", "latn", "ROM "],
  ["liga", "latn", "SKS "],
  ["liga", "latn", "SSM "],
  ["liga", "latn", "dflt"],

as this font supports ligatures in both Latin and Arabic script, and has several language-specific variants. I imagine that for an initial Font Inspector UI, we might want to filter this down to a single "liga" item, but I thought that is best left to the front end to decide, rather than hard-coded into the API. A secondary UI (maybe a tooltip for each feature?) might want to expose more detail, such as the script(s) for which the feature is present.

[1] https://www.microsoft.com/typography/otspec/ttoreg.htm

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: patch 1 - Add a 'features' attribute to nsIDOMFontFace to expose the OpenType features supported by a font

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: patch 2 - Implement gfxFontEntry method to query font feature info for OpenType fonts

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: patch 3 - Testcase for layout inspector API to examine font features

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 8936802 [details] [diff] [review]
patch 1 - Add a 'features' attribute to nsIDOMFontFace to expose the OpenType features supported by a font

Patrick, does this seem like it would provide what you need from the DevTools side, at least as a starting point? Anything you'd like to see differently?

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8936802 [details] [diff] [review]
patch 1 - Add a 'features' attribute to nsIDOMFontFace to expose the OpenType features supported by a font

This is great Jonathan.
I agree with approach (2) from comment 3. It makes it easy for us to start prototyping a simple UI that just lists the features at first.
It'll be easy enough for us to loop through the features and get the list of scripts and language systems from there if we do decide to show this information at a later point.
So, I'm definitely F+ on this patch.

One thing I wonder about though is the opentype registry. We'll definitely want to display user-friendly strings in the UI, so for instance Common Ligatures instead of liga.
How do you think we'll want to do the mapping. We could make a copy of the data in DevTools source code of course, but then we'd have to be careful if the registry ever gets updated. How likely is that?
In fact, I think we will want to localize these as well anyway, so they'll have to be in DevTools.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: patch 1 - Implement gfxFontEntry method to query font feature info for OpenType fonts

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: patch 2 - Add a getFeatures() method to InspectorFontFace to expose the OpenType features supported by a font

This is like bug 1432552, except for OpenType "features" -- i.e. controlling things like ligatures, swashes, kerning, alternate glyphs, etc -- rather than "variations" (design axes like width and weight). Together, these two bugs should provide the basis for a much-enhanced Font Inspector panel.

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: patch 3 - Testcase for layout inspector API to examine font features

Comment 13

[Jonathan Kew [:jfkthame]]

(In reply to Patrick Brosset <:pbro> from comment #9)
> One thing I wonder about though is the opentype registry. We'll definitely
> want to display user-friendly strings in the UI, so for instance Common
> Ligatures instead of liga.
> How do you think we'll want to do the mapping. We could make a copy of the
> data in DevTools source code of course, but then we'd have to be careful if
> the registry ever gets updated. How likely is that?

The registry is pretty stable, but does still get occasional additions. However, I think for DevTools purposes what's much more important than user-friendly (and potentially localizable) strings for each individual feature is the mapping between low-level feature tags and higher-level CSS properties (the font-variant-* properties from the CSS Fonts spec). For this, see the descriptions of the values for the font-variant-* subproperties in CSS Fonts 3, where corresponding OpenType features for each CSS value are mentioned.

Authors who want to deal with obscure/non-standard features for which no corresponding high-level font-variant-* property exists will almost certainly be aware of feature tags, and have to deal with them in other contexts (font development, technical documentation of fonts that support such rare features, etc) anyway. While user-friendly names are nice when available, we don't need to be afraid of falling back to displaying 4-character tags for features (or variation axes) as a last resort.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8946141 [details] [diff] [review]
patch 1 - Implement gfxFontEntry method to query font feature info for OpenType fonts

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

Partial review for part 1:

::: gfx/thebes/gfxFontEntry.cpp
@@ +951,5 @@
> +        unsigned int featCount = 0;
> +        featCount =
> +            hb_ot_layout_language_get_feature_tags(face, aTableTag, aScript,
> +                                                   aLang, 0, &featCount,
> +                                                   nullptr);

I suspect you don't really want to be using featCount twice here (firstly as an outparam, and secondly to capture the return value)...

This API is new to me, but I suspect you really want to pass nullptr for that arg, for this first call (the call where we're asking for the count)... At least, I found a call like that here (which passes NULL, NULL as the last 2 args):
https://lists.freedesktop.org/archives/harfbuzz/2012-November/002675.html

...and if I drill down into the API (a few layers in), it looks like a non-null value signals that you want data copied into the other outparam (which you don't want, because you're passing nullptr for that arg):
https://searchfox.org/mozilla-central/source/gfx/harfbuzz/src/hb-ot-layout-common-private.hh#174,177,181

I think this arg (when non-null) is meant to represent "here's the number of entries you can write into the output buffer", and then the function reuses it as an outparam to indicate the number that it actually did write?  (I can't actually find any documentation on this harfbuzz API - my googling is just finding declarations/usages without documentation -- so I'm kind of guessing. Do you know if there's documentation anywhere?)

Anyway, given that you aren't providing an output buffer here, I *think* the arg wants to be nullptr...

@@ +957,5 @@
> +        featTags.SetLength(featCount);
> +        hb_ot_layout_language_get_feature_tags(face, aTableTag, aScript,
> +                                               aLang, 0, &featCount,
> +                                               featTags.Elements());
> +        for (hb_tag_t t : featTags) {

Do you know if we're 100% justified in assuming that this HarfBuzz will exactly fill our featTags array, in edge-casey / broken-font scenarios?

(Perhaps we should assert that featCount <= featTags.Length(), and then call SetLength(featCount) again *after* this function-call, to truncate away any possibly-uninitialized array entries at the end of the array, now that we know how many entries were *actually copied*?)

@@ +973,5 @@
> +                       HB_TAG('d','f','l','t'));
> +        unsigned int langCount = 0;
> +        langCount =
> +            hb_ot_layout_script_get_language_tags(face, aTableTag, aScript, 0,
> +                                                  &langCount, nullptr);

Similarly, here you probably want to pass "nullptr" instead of &langCount?

@@ +978,5 @@
> +        AutoTArray<hb_tag_t,32> langTags;
> +        langTags.SetLength(langCount);
> +        hb_ot_layout_script_get_language_tags(face, aTableTag, aScript, 0,
> +                                              &langCount, langTags.Elements());
> +        for (unsigned int lang = 0; lang < langCount; ++lang) {

(and as above, maybe we should assert that langCount <= langTags.Length(), and then call langTags.SetLength(langCount) just in case it happens to be fewer than we expected?)

Comment 15

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> I suspect you don't really want to be using featCount twice here (firstly as
> an outparam, and secondly to capture the return value)...
> 
> This API is new to me, but I suspect you really want to pass nullptr for
> that arg, for this first call (the call where we're asking for the count)...
> At least, I found a call like that here (which passes NULL, NULL as the last
> 2 args):
> https://lists.freedesktop.org/archives/harfbuzz/2012-November/002675.html
> 
> ...and if I drill down into the API (a few layers in), it looks like a
> non-null value signals that you want data copied into the other outparam
> (which you don't want, because you're passing nullptr for that arg):
> https://searchfox.org/mozilla-central/source/gfx/harfbuzz/src/hb-ot-layout-
> common-private.hh#174,177,181
> 
> I think this arg (when non-null) is meant to represent "here's the number of
> entries you can write into the output buffer", and then the function reuses
> it as an outparam to indicate the number that it actually did write?  (I
> can't actually find any documentation on this harfbuzz API - my googling is
> just finding declarations/usages without documentation -- so I'm kind of
> guessing. Do you know if there's documentation anywhere?)
> 
> Anyway, given that you aren't providing an output buffer here, I *think* the
> arg wants to be nullptr...

I believe it actually works OK as-is, because featCount has been initialized to zero and therefore the API won't try to actually write anything to the (nullptr) output array; but you're right that it seems confusing, and I think passing nullptr for both the args would be clearer/simpler. (And marginally more efficient, even!) So I'll update the patch accordingly; new version coming shortly.

> Do you know if we're 100% justified in assuming that this HarfBuzz will exactly
> fill our featTags array, in edge-casey / broken-font scenarios?

I think so, as harfbuzz validates these tables when it loads the font, but adding your suggested assertions and explicitly re-setting the length seems like a good belt-and-braces precaution.

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: patch 1 - Implement gfxFontEntry method to query font feature info for OpenType fonts

Updated/improved as per suggestions above.

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> Do you know if there's documentation anywhere?)

Not really, at present; there's a current project to get it written, but for now the code is the documentation (mostly).

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8946272 [details] [diff] [review]
patch 1 - Implement gfxFontEntry method to query font feature info for OpenType fonts

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

r=me

::: gfx/thebes/gfxFontEntry.cpp
@@ +956,5 @@
> +        hb_ot_layout_language_get_feature_tags(face, aTableTag, aScript,
> +                                               aLang, 0, &featCount,
> +                                               featTags.Elements());
> +        MOZ_ASSERT(featCount <= featTags.Length());
> +        featTags.SetLength(featCount);

For the benefit of confused code-readers / archeologists, probably worth annotating this superficially-redundant second SetLength() call with a brief explanation, like:

	// Just in case HB didn't fill featTags (i.e. in case it returned fewer
        // tags than it promised), we truncate at the length it says it filled:

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8946142 [details] [diff] [review]
patch 2 - Add a getFeatures() method to InspectorFontFace to expose the OpenType features supported by a font

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

r=me

(For bz's reference: the AppendTagAsASCII() api used here hasn't made it to mozilla-central yet, but it's at https://hg.mozilla.org/integration/mozilla-inbound/rev/a7ff43ee429b#l4.30 )

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8946143 [details] [diff] [review]
patch 3 - Testcase for layout inspector API to examine font features

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

r=me

(I'm assuming we're OK to add the DejaVuSans.ttf file, since we already include the exact same file elsewhere in the tree, in layout/reftests/fonts/dejavu-sans)

::: layout/inspector/tests/chrome/test_fontFeaturesAPI.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 -->

Drop this boilerplate comment ^^

@@ +13,5 @@
> +
> +SimpleTest.waitForExplicitFinish();
> +
> +// Expected result (sorted) from the DejaVu Sans font we're testing with
> +var dejaVuFeatures = [

(maybe add a ":" at the end of this comment - otherwise it feels like it might be trailing off midsentence)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8946142 [details] [diff] [review]
patch 2 - Add a getFeatures() method to InspectorFontFace to expose the OpenType features supported by a font

>+  for (auto f : features) {

Is there a reason this is not "auto& f: features"?  That is, a reason we're copying all those structs?

r=me with that fixed or documented.

Comment 22

[Jonathan Kew [:jfkthame]]

No, you're right - we should use a reference there. Will fix, thanks.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8290a5ff7354
patch 1 - Implement gfxFontEntry method to query font feature info for OpenType fonts. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/f100b8eb60a6
patch 2 - Add a getFeatures() method to InspectorFontFace to expose the OpenType features supported by a font. r=dholbert,bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd3079e9390c
patch 3 - Testcase for layout inspector API to examine font features. r=dholbert

Comment 24

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8290a5ff7354
https://hg.mozilla.org/mozilla-central/rev/f100b8eb60a6
https://hg.mozilla.org/mozilla-central/rev/bd3079e9390c