511339 - implement experimental support for -moz-font-feature-settings

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[John Daggett (:jtd)]

Jonathan and I put together a proposal for supporting OpenType font features in CSS:

  http://lists.w3.org/Archives/Public/www-style/2009Jun/0506.html
  
In a nutshell, font features affect glyph selection and/or positioning
without changing the font used.  The proposal above is for a set of
font-variant-xxx properties that are turned into OpenType/AAT features
within text rendering code.

As a simple way of experimenting with OpenType features directly, add an
experimental CSS property -moz-font-feature-opentype with the syntax
below:

  -moz-font-feature-opentype: auto | <feature-string> ;
  
Here, 'auto' means use font defaults and is the initial value.

The <feature-string> would be a sequence of tags and values:

  -moz-font-feature-opentype: "liga=0, salt=2, xyz1=1";

Each of the 4-character tags are OpenType feature tags, as defined here:

http://www.microsoft.com/opentype/otspec/featurelist.htm

The order of the tags would not affect rendering since OT features are processed in the order they occur within font data.

I'm assuming that the string is opaque to style code and gets passed
down within a gfxFontStyle object associated with a run of text. We can
then hook this up to harfbuzz (bug 449292) or rig a quick hack on
Vista/Win7 to render using the Uniscribe calls that allow OT features
to be specified explicitly.

ScriptShapeOpenType
http://msdn.microsoft.com/en-us/library/dd368565%28VS.85%29.aspx

Note: I'm not advocating this as a general CSS property, although some would see utility in it.  This is just a convenient way of experimenting with OT features within the browser.

Comment 1

[John Daggett (:jtd)]

Attachment: patch, initial implementation of -moz-font-feature-opentype

First pass at implementing -moz-font-feature-opentype.  Parses the CSS and sticks the value into nsCSSFont, which uses it to set the value in nsFont, which puts it into the gfxFontStyle.  From gfxFontStyle, font groups can parse the OpenType feature list and pass the values to harfbuzz/Uniscribe/whatever.  Not the ideal way to do this but it's enough for experimentation.

I've added logging code in gfxAtsuiFontGroup to dump the opentype feature list when text run logging is enabled:

export NSPR_LOG_MODULES=atsuiTextRun:5

Comment 2

[John Daggett (:jtd)]

Attachment: testcase using -moz-font-feature-opentype

Set logging, run FF, open testcase, output below will appear:

InitTextRun 1d358b10 fontgroup 1d358aa0 (serif) lang: x-western len 10 opentype: liga=0, salt=2, xyz1=1 TEXTRUN "Great fun!" ENDTEXTRUN

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 395544 [details] [diff] [review]
patch, initial implementation of -moz-font-feature-opentype

You'll also want to add to layout/style/test/property_database.js and then make sure all the mochitests in layout/style pass.

> PRBool
> CSSParserImpl::ParseFont()
...
>+  // hmmm, think i need to reset font-feature-opentype here, not sure how...

Just add an extra AppendValue call in the three obvious-looking places in that function.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: patch, connecting -moz-font-feature-opentype to the harfbuzz shaper

WIP/proof-of-concept. This patch enables CSS control of features through the HarfBuzz layout engine, using the patches from bug 449292 (which in turn depend on bug 502906).

Comment 5

[John Daggett (:jtd)]

Javascript get/set methods work but the syntax is a tad funky, I probably need to tweak something.  The setter requires an extra nested quote, I'm guessing this is because the setter is essentially passing the value to the parser and the parser in this case is looking for quotes.

  var el = document.getElementById("main");
  el.style.MozFontFeatureOpentype = "'burp=1'";

This will cause the string "burp=1" to end up in the opentypeFeatures field of gfxFontStyle.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: sample file for experimenting with font features

Cool .... yes, I think it was the quotes that were confusing me. Works fine now. I'm attaching a sample file that I'm using to experiment with feature settings.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - implement -moz-font-feature-settings and -moz-font-language-override in CSS

This based on John's original patch, but updated in several ways:

- rename the property to -moz-font-feature-settings (following the current CSS3 Fonts editor's draft)

- store features as an array of tag+value pairs rather than just passing the string down to gfx

- also implement -moz-font-language-override (see CSS3 Fonts draft)

This seems to pass the style mochitests successfully; however, I'm not sure whether something is still needed in CSSParserImpl::ParseFont (see comments there). I tried adding what seemed to me like the "obvious" extra AppendValue calls, but this led to lots of assertion spew.... clearly, I don't really understand this code.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - connect font feature properties to the harfbuzz shaper

This is dependent on the harfbuzz patches from bug 449292.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 442941 [details] [diff] [review]
part 1 - implement -moz-font-feature-settings and -moz-font-language-override in CSS

>diff --git a/dom/interfaces/css/nsIDOMNSCSS2Properties.idl b/dom/interfaces/css/nsIDOMNSCSS2Properties.idl
>--- a/dom/interfaces/css/nsIDOMNSCSS2Properties.idl
>+++ b/dom/interfaces/css/nsIDOMNSCSS2Properties.idl
>@@ -116,16 +116,22 @@ interface nsIDOMNSCSS2Properties : nsIDO

You need to rev the IID of nsIDOMNSCSS2Properties.

>diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp
>--- a/layout/style/nsCSSParser.cpp
>+++ b/layout/style/nsCSSParser.cpp

So, in order to help you with your question about the parser, the first thing I looked at was nsStyleStruct.cpp - the constructor of nsStyleFont - to see what the initial values of these new properties are.  And as far as I can tell, featureSettings's initial value is null (from the nsTArray constructor called by the nsFont constructor), which seems OK, but languageOverride looks like it's uninitialized in nsStyleFont's constructor (actually 2 constructors; the copy-constructor is ok, though).

I think you also need to change the two nsFont constructors that initialize other things to initialize languageSettings as well; I think you probably also want to add parameters to those constructors to catch places that need to pass them through.  (You fixed the copy-constructor, and there's one constructor that's intended to leave things uninitialized.)


In nsRuleNode.cpp, for both properties, you have handling for a 'none' value that you don't handle in the parsing code; you should just remove that (and handle just eCSSUnit_Initial and eCSSUnit_Normal).

Should there be a named constant to represent that 0 means no language override?  nsFont.h and gfxFont.h should probably say what values the languageOverride member takes (e.g., have a comment pointing to a header file with the appropriate comments).


In any case, now that I'm past the things I noticed trying to figure out the initial values of the property, back to your question:

>@@ -7441,28 +7445,34 @@ CSSParserImpl::ParseFont()
>         AppendValue(eCSSProperty_font_family, family);
>         AppendValue(eCSSProperty_font_style, family);
>         AppendValue(eCSSProperty_font_variant, family);
>         AppendValue(eCSSProperty_font_weight, family);
>         AppendValue(eCSSProperty_font_size, family);
>         AppendValue(eCSSProperty_line_height, family);
>         AppendValue(eCSSProperty_font_stretch, family);
>         AppendValue(eCSSProperty_font_size_adjust, family);
>+        // Do we need to do something here with these?
>+        //   eCSSProperty_font_feature_settings
>+        //   eCSSProperty_font_language_override

I think doing something here is desirable since I think these new properties probably should be reset by the 'font' shorthand.

Here you can just make the exact same AppendValue calls and it shouldn't cause any problems.  This is handling the case of 'font:inherit' or 'font:-moz-initial', which set all the subproperties of these properties to 0.

>       else {
>         AppendValue(eCSSProperty__x_system_font, family);
>         nsCSSValue systemFont(eCSSUnit_System_Font);
>         AppendValue(eCSSProperty_font_family, systemFont);
>         AppendValue(eCSSProperty_font_style, systemFont);
>         AppendValue(eCSSProperty_font_variant, systemFont);
>         AppendValue(eCSSProperty_font_weight, systemFont);
>         AppendValue(eCSSProperty_font_size, systemFont);
>         AppendValue(eCSSProperty_line_height, systemFont);
>         AppendValue(eCSSProperty_font_stretch, systemFont);
>         AppendValue(eCSSProperty_font_size_adjust, systemFont);
>+        // Do we need to do something here with these?
>+        //   eCSSProperty_font_feature_settings
>+        //   eCSSProperty_font_language_override

Here you have two options.  You could either:
 * append an eCSSUnit_Normal value, or
 * append an eCSSUnit_System_Font value

I think the latter is probably the better choice, but it means that you need to add handling for eCSSUnit_System_Font in nsRuleNode.cpp (which was probably the cause of the assertions).

The reason for these values is that system fonts are single values for the font shorthand, such as:
  font: menu;
but it's legal to write:
  font: menu;
  font-style: italic;
which means that you should use all aspects of the 'menu' system font, except override the font-style to make it italic.


As I said, I think making these new properties be reset by the 'font' shorthand does seem like the right thing to me.  However, to do it correctly, you also need to:
 * adjust gFontSubpropTable in nsCSSProps.cpp
 * adjust the serialization code for case eCSSProperty_font in nsCSSDeclaration::GetValue
 * adjust the list of subproperties for "font" in property_database.js

>@@ -7514,16 +7524,19 @@ CSSParserImpl::ParseFont()
>       AppendValue(eCSSProperty_font_style, values[0]);
>       AppendValue(eCSSProperty_font_variant, values[1]);
>       AppendValue(eCSSProperty_font_weight, values[2]);
>       AppendValue(eCSSProperty_font_size, size);
>       AppendValue(eCSSProperty_line_height, lineHeight);
>       AppendValue(eCSSProperty_font_stretch,
>                   nsCSSValue(NS_FONT_STRETCH_NORMAL, eCSSUnit_Enumerated));
>       AppendValue(eCSSProperty_font_size_adjust, nsCSSValue(eCSSUnit_None));
>+      // Do we need to do something here with these?
>+      //   eCSSProperty_font_feature_settings
>+      //   eCSSProperty_font_language_override

And here, to make these properties be reset by the font shorthand,  you'd want to append an eCSSUnit_Normal value for both properties.


I haven't looked through the whole patch yet; just the parts relevant to your question about the parser.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The main cause of the assertions was probably the mismatch between gFontSubpropTable and what the parsing code was doing; the lack of eCSSUnit_System_Font support in nsRuleNode might have been a cause of a few additional ones.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: part 1, v2 - implement -moz-font-feature-settings and -moz-font-language-override in CSS

Thanks for the comments, dbaron; very helpful. I think this is a lot closer to being correct now.

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: part 2, v2 - connect font feature properties to the harfbuzz shaper

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 445111 [details] [diff] [review]
part 1, v2 - implement -moz-font-feature-settings and -moz-font-language-override in CSS

Is it really safe to assume all language tags in this context are <= 4
characters?  Are there relevant rules that forbid using zh-Hans,
zh-Hant, etc.?


I'm not a big fan of default parameters in cases like the way you use
them in the nsFont constructor, since they tend to hide places that need
to be changed.  But I suppose they're ok in this case (especially since
those constructors already use them).

gfxFont.h:

>+inline PRBool
>+operator==(const gfxFontFeature& a, const gfxFontFeature& b)
>+{
>+    return (a.mTag == b.mTag) && (a.mValue == b.mValue);
>+}

I think you don't need to write this since the operator== implicitly
generated by the compiler is fine.  (If you want to leave a comment
that says
  // implicit operator==(const gfxFontFeature&, const gfxFontFeature&)
  // is correct
that's fine, though.


I think gfxFontStyle could perhaps use some comments as to how language
and languageOverride relate to each other.  Do we really need both?

nsCSSDeclaration.cpp:

>-        // The font-stretch and font-size-adjust
>+        // The font-stretch, font-size-adjust
>+        // -moz-font-feature-settings and -moz-font-language-override

commas after font-size-adjust and -moz-font-feature-settings, please.

nsCSSParser.cpp:

>+  case eCSSProperty_font_feature_settings:
>+    return ParseVariant(aValue, VARIANT_NORMAL | VARIANT_INHERIT | VARIANT_STRING, nsnull);
>+  case eCSSProperty_font_language_override:
>+    return ParseVariant(aValue, VARIANT_NORMAL | VARIANT_INHERIT | VARIANT_STRING, nsnull);

Please drop the first "return" line and just have both cases share
the code.

Also, please wrap at less than 80 characters (i.e., break after the
last | and align VARIANT_STRING with VARIANT_NORMAL).

nsComputedDOMStyle.cpp:

AppendTagToString isn't quite right in terms of escaping.  I think
should instead just append the four characters to a string without
any escaping, and its callers should then be responsible for passing
the result to nsStyleUtil::AppendEscapedCSSString, which does the
correct escaping for a CSS string (and also places the quote marks at
the start and end).  (AppendEscapedCSSString has to be done only once
per string, so you can't put it inside AppendTagToString; but see
paragraph after next.)

Please add tests to property_database.js for things that were broken
without this.

(I'm also a bit concerned about the feature settings property being a
single string rather than a comma-separated list of strings.  It just
feels odd to have a list all stored *inside* a string.)

nsRuleNode.cpp:

For both properties, it's probably better to use
nsCSSValue::GetStringBufferValue than nsCSSValue::GetStringValue.  If
you want to use string APIs on the value, wrap it in an
nsDependentString rather than copying.

For both properties, I think your case for eCSSUnit_SystemFont should be
separate from normal and initial:  it should assign the
featureSettings/languageOverride from
systemFont.featureSettings/languageOverride.  This would mean that if
any of the system font backends start filling them in (which I could
certainly see them doing for languageOverride), they'd get used.

+    if (!nsCRT::IsAscii(ch)) { // valid tags are pure ASCII
+      return NO_FONT_LANGUAGE_OVERRIDE;
+    }

This doesn't seem very CSS-ish.  If a value is invalid, it should be
dropped by the parser.  (This is part of CSS's forward-compatibility
mechanism:  it allows new values to fill the space unused by old ones in
a way that lets authors use multiple declarations as fallback for older
implementations.)

The way one would normally accomplish that is by having the parsing code
live in the parser (not in nsRuleNode), which turns the string into some
useful data structure that's stored in the rule (i.e., an nsCSSValue,
nsCSSValuePair, nsCSSValueList, or nsCSSValuePairList).  (This also
means nsCSSDeclaration needs to know how to serialize it.)

Same (but with more complications) for font feature settings.

I'm not sure whether I'd insist on this change, though...


property_database.js:

should probably have examples of invalid values, at least the same as
the valid one but unquoted.

I presume the nsChildView.mm changes and the nsTArray changes weren't
really intended to be part of this patch.


This looks good otherwise, though.

Comment 14

[Jonathan Kew [:jfkthame]]

(In reply to comment #13)
> (From update of attachment 445111 [details] [diff] [review])
> Is it really safe to assume all language tags in this context are <= 4
> characters?  Are there relevant rules that forbid using zh-Hans,
> zh-Hant, etc.?

For -moz-font-language-override, the tags are not ISO or BCP 47 language tags but OpenType language system tags. These are a separate collection without a simple or unambiguous mapping to standard language tags; they represent something more like "a particular set of orthographic conventions for a writing system". (And hence the need to sometimes override the default that can be inferred from the document or element's language.)

The OpenType language tags are defined to be <= 4 characters (they're treated as 4-byte integers within OpenType; they just happen to be integers that, when interpreted as 4 ASCII bytes in big-endian order, look like mnemonic language tags, and so they're normally written that way).

> I think gfxFontStyle could perhaps use some comments as to how language
> and languageOverride relate to each other.  Do we really need both?

Unfortunately, we do, because they're different things; I'll add comments to clarify this.

> (I'm also a bit concerned about the feature settings property being a
> single string rather than a comma-separated list of strings.  It just
> feels odd to have a list all stored *inside* a string.)

This would be a spec issue; the current CSS3 Fonts draft (http://dev.w3.org/csswg/css3-fonts/#font-feature-settings-prop) says it's a string, but perhaps we should consider allowing a list of strings. As far as CSS is concerned it's an opaque value whose exact contents and interpretation is dependent on the type of font being used.

John, any thoughts on the benefits or drawbacks of making this property a list of strings at the CSS level?

> I presume the nsChildView.mm changes and the nsTArray changes weren't
> really intended to be part of this patch.

The patch requires us to add nsTArray::Equals (used when testing equality of nsFont objects with arrays in the featureSettings field); I implemented nsTArray::Compare as well as Equals because it seemed logical in terms of the API, but that's not actually required here.

IIRC, the nsChildView.mm change (just moving the #include of prlog.h a bit earlier) was required because of a build issue that shows up otherwise, but I'll double-check what that was about.

Will work through the other issues mentioned. Thanks!

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'd note that the parsing issues are also a spec issue:  it's unusual (and bad for cascading) for a CSS spec to accept values at parse time that don't mean anything.  The spec ought to define which values are accepted at parse time, but in the end we ought to match it.

Comment 16

[John Daggett (:jtd)]

(In reply to comment #14)
> > I think gfxFontStyle could perhaps use some comments as to how language
> > and languageOverride relate to each other.  Do we really need both?
> 
> Unfortunately, we do, because they're different things; I'll add comments to
> clarify this.

The current editor's draft has an example of how font-language-override is used:

  http://dev.w3.org/csswg/css3-fonts/#font-language-override-prop

> > (I'm also a bit concerned about the feature settings property
> > being a single string rather than a comma-separated list of
> > strings.  It just feels odd to have a list all stored *inside* a
> > string.)
> 
> This would be a spec issue; the current CSS3 Fonts draft
> (http://dev.w3.org/csswg/css3-fonts/#font-feature-settings-prop)
> says it's a string, but perhaps we should consider allowing a list
> of strings. As far as CSS is concerned it's an opaque value whose
> exact contents and interpretation is dependent on the type of font
> being used.
> 
> John, any thoughts on the benefits or drawbacks of making this
> property a list of strings at the CSS level?

We could use a slightly different syntax, something like:

  font-feature-settings: <tag>[(<integer>)]? [<tag>[(<integer>)]? ]*

where <tag> is a four-character sequence and <integer> is zero or greater.

Example:

  font-feature-settings: salt(5) ss03 liga hlig(0);

This would be equivalent to:

  -moz-font-features-opentype: "salt=5,ss03=1,liga=1,hlig=0";

I'm not sure we can really go beyond that.  The whole point of this is
to effectively allow a pass-through to an OpenType layout engine,
which tag values have meaning is defined by OpenType.

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to comment #16)
> We could use a slightly different syntax, something like:
> 
>   font-feature-settings: <tag>[(<integer>)]? [<tag>[(<integer>)]? ]*
> 
> where <tag> is a four-character sequence and <integer> is zero or greater.

I'd be hesitant to do this, as it ties the CSS syntax too closely to the particulars of OpenType technology, and if/when some other font technology comes along that does not express font features as 4-char tags and integer values, we're in trouble.

Alternative:

    font-feature-settings: <string> [, <string>]*

which is simply a list of strings to be passed to the font-rendering system for interpretation.

> Example:
> 
>   font-feature-settings: salt(5) ss03 liga hlig(0);

This then becomes

    font-feature-settings: "salt=5", "ss03", "liga", "hlig=0";

> I'm not sure we can really go beyond that.  The whole point of this is
> to effectively allow a pass-through to an OpenType layout engine,
> which tag values have meaning is defined by OpenType.

I would put this differently: The whole point of this is to allow a pass-through of arbitrary parameters to a font layout engine, without attempting to define or interpret the values at the CSS level. (I.e., it should not be tied to the OpenType feature model, IMO.)

Even in today's world, there are at least two existing font technologies (AAT and Graphite) that would require something other than 4-character tags to control arbitrary features. This CSS property should leave room for that.

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

At the risk of overengineering: should font-feature-settings have a way to specify exactly which font the features apply to?  That is, given:

font-family: "Foo", "Bar", "Baz";

you may want "salt(5) ss03 liga hlig(0)" to only apply to the "Foo" family, not whatever fallback font the browser chose if the user doesn't have Foo available, because you have no idea what effect those features might have there (and might indeed be broken).

Comment 19

[John Daggett (:jtd)]

(In reply to comment #18)
> At the risk of overengineering: should font-feature-settings have a way to
> specify exactly which font the features apply to?  That is, given:
> 
> font-family: "Foo", "Bar", "Baz";
> 
> you may want "salt(5) ss03 liga hlig(0)" to only apply to the "Foo" family, not
> whatever fallback font the browser chose if the user doesn't have Foo
> available, because you have no idea what effect those features might have there
> (and might indeed be broken).

Yeah, lots of discussion on www-style related to this topic.  The current draft spec allows for font-feature-settings to be specified per-font in @font-face rules.  Combining this with src: local() use allows it to be applied to specific platform fonts:

http://dev.w3.org/csswg/css3-fonts/#rendering-considerations

Scroll down to view Example XXIII.  Not the most elegant solution but since feature use is most likely to be done in conjunction with downloadable fonts, it's not entirely crazy.

I don't think the "only for the first font" way works very well, it starts to break down when you have font stacks that are designed for multiple platforms rather than "missing glyph" fallback (e.g. font-family: Helvetica, Arial, sans-serif;).  In some cases you may want fallback (e.g. hlig(1)) in other cases not (e.g. ss03).

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

That makes sense -- sorry for the drive-by comment, and thanks for the explanation!

Comment 21

[Jonathan Kew [:jfkthame]]

In the light of review comments (thanks, dbaron!) and thinking about how the current CSS3 Fonts draft defines these properties, I think it's more logical to maintain them as simple strings at the CSS parser/rule level. As far as CSS is concerned, that's all they are; it has no concept of "valid" or "invalid" contents of these strings. Any interpretation of the string content is a matter for the font-rendering backend.

So I've modified this to keep featureSettings and languageOverride as uninterpreted strings at the CSS level; they only get parsed further (or ignored, if their content isn't meaningful) when they are handed to the thebes font system, which knows about arrays of OpenType features and with OpenType langSys tags.

I've also added -moz-font-feature-settings and -moz-font-language-override as descriptors in @font-face rules.

Comment 22

[Jonathan Kew [:jfkthame]]

Attachment: part 1, v3 - maintain features/langOverride as strings in css rules; also support @font-face descriptors

Comment 23

[Jonathan Kew [:jfkthame]]

Attachment: part 2, v3 - gfx/thebes backend support for the font-feature-settings properties

This supports -moz-font-feature-settings and -language-override both as style properties and @font-face descriptors, with the style taking precedence, as in the current CSS draft.

Note that adding these descriptors to @font-face required me to modify the gfxFontCache so that the key is not (psname + style); instead, it's now keyed on the (fontEntry + style), as there may be multiple fontEntries with different properties referring to the same postscript name. A side benefit is that this should make the cache marginally faster, and tryserver results suggest this may even show up as a (slight) visible improvement.

Comment 24

[Jonathan Kew [:jfkthame]]

Attachment: part 2a - add an Equals method to nsTArray

Although the CSS side no longer uses this, it is still needed to support the gfxFontStyle code that handles arrays of feature settings.

Comment 25

[John Daggett (:jtd)]

+inline PRBool
+operator<(const gfxFontFeature& a, const gfxFontFeature& b)
+{
+    return (a.mTag < b.mTag) || ((a.mTag == b.mTag) && (a.mValue < b.mValue));
+}

Unused, trim.

From comment 23:

> Note that adding these descriptors to @font-face required me to
> modify the gfxFontCache so that the key is not (psname + style);
> instead, it's now keyed on the (fontEntry + style), as there may be
> multiple fontEntries with different properties referring to the same
> postscript name. A side benefit is that this should make the cache
> marginally faster, and tryserver results suggest this may even show
> up as a (slight) visible improvement.

>          static PLDHashNumber HashKey(const KeyTypePointer aKey) {
> -            return HashString(aKey->mString) ^ aKey->mStyle->Hash();
> +            return NS_PTR_TO_INT32(aKey->mFontEntry) ^ aKey->mStyle->Hash();
>          }

Hmmm, I don't think this will work.  While font entries for platform
fonts have a lifetime of the app, downloadable fonts don't, a font
entry could be reused.  The only way to distinguish between fonts is
via the name unfortunately.

I don't quite follow why this was needed.  If the key is (name +
style) and the feature list is part of style, you still have two
unique keys no?

Comment 26

[Jonathan Kew [:jfkthame]]

(In reply to comment #25)
> +inline PRBool
> +operator<(const gfxFontFeature& a, const gfxFontFeature& b)
> +{
> +    return (a.mTag < b.mTag) || ((a.mTag == b.mTag) && (a.mValue < b.mValue));
> +}
> 
> Unused, trim.

Actually, this is needed by gfxFontStyle::ParseFontFeatureSettings because it does InsertElementSorted when adding features to the array. This was done so that (for example) styles with "dlig=1,smcp=1" and "smcp=1,dlig=1" will be seen as identical. However, maybe it's not worth bothering with this.

> 
> From comment 23:
> 
> > Note that adding these descriptors to @font-face required me to
> > modify the gfxFontCache so that the key is not (psname + style);
> > instead, it's now keyed on the (fontEntry + style), as there may be
> > multiple fontEntries with different properties referring to the same
> > postscript name. A side benefit is that this should make the cache
> > marginally faster, and tryserver results suggest this may even show
> > up as a (slight) visible improvement.
> 
> >          static PLDHashNumber HashKey(const KeyTypePointer aKey) {
> > -            return HashString(aKey->mString) ^ aKey->mStyle->Hash();
> > +            return NS_PTR_TO_INT32(aKey->mFontEntry) ^ aKey->mStyle->Hash();
> >          }
> 
> Hmmm, I don't think this will work.  While font entries for platform
> fonts have a lifetime of the app, downloadable fonts don't, a font
> entry could be reused.  The only way to distinguish between fonts is
> via the name unfortunately.
> 
> I don't quite follow why this was needed.  If the key is (name +
> style) and the feature list is part of style, you still have two
> unique keys no?

As per discussion on irc, with feature/language descriptors in @font-face, we can have multiple font entries with the same name but different features in the entry (which are separate from the features in the style). Using the font entry pointer is safe because it's guaranteed to remain unique as long as any font based on that entry remains in existence; the possibility of re-use only occurs after all instances have expired.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 451900 [details] [diff] [review]
part 2a - add an Equals method to nsTArray

You probably want to save Length() into a local variable so you don't have to probe into mHdr each iteration... and probably the same for Elements() of both arrays.  r=dbaron with that

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 451898 [details] [diff] [review]
part 1, v3 - maintain features/langOverride as strings in css rules; also support @font-face descriptors

r=dbaron; sorry for the delay getting to this

I'd note that while this is labeled as patch 1, it depends on the gfxFontStyle members added in patch 2 (and the constructor changes).  (I'm not sure if there's a good way to split the patches so the halfway state compiles.)

Comment 29

[Jonathan Kew [:jfkthame]]

Comment on attachment 451900 [details] [diff] [review]
part 2a - add an Equals method to nsTArray

While refreshing patches, I realized we don't need "part 2a" (adding the nsTArray::Equals method) at all, as there's already an operator== defined that we can use. Hence marking this patch as obsolete.

Comment 30

[Jonathan Kew [:jfkthame]]

pushed to trunk:
http://hg.mozilla.org/mozilla-central/rev/0f7bc3357bc3 (part 1 - css)
http://hg.mozilla.org/mozilla-central/rev/1027e04ad4ee (part 2 - gfx)

Comment 31

[Jonathan Kew [:jfkthame]]

bustage fix for Android build (FT2Fonts):
http://hg.mozilla.org/mozilla-central/rev/14d43898f62a

Comment 32

[will69]

Jonathan, could you update your "CSS OpenType Font Feature demo", so we can easily try this out?

Comment 33

[Jonathan Kew [:jfkthame]]

Good point - the property names have changed since that was done, so it won't work as-is. I'll refresh it....

Comment 34

[Rob Crowther]

I have some versions of Jonathan's demos with the new syntax, if it hasn't changed again in the last three weeks:

http://www.boogdesign.com/examples/fonts/MEgalopolis.html
http://www.boogdesign.com/examples/fonts/cooking.html
http://www.boogdesign.com/examples/fonts/forex.html
http://www.boogdesign.com/examples/fonts/udhr-turkish.html

Comment 35

[Jonathan Kew [:jfkthame]]

Rob: yes, your copies should still work, the syntax hasn't changed (again). Note that the link to the "experimental build" on those pages is now obsolete, however.

I've updated my samples at http://people.mozilla.com/~jkew/feature-samples/ to work with current trunk builds.

Comment 36

[Jonathan Kew [:jfkthame]]

Attachment: followup - add reftests for font feature & language control

This adds a small collection of font-feature reftests, using the Linux Libertine font (as it has a wide range of features and language support - we'll also be able to use it to test a number of more specific CSS properties once we implement those).

Comment 37

[John Daggett (:jtd)]

(In reply to comment #36)
> This adds a small collection of font-feature reftests, using the Linux
> Libertine font (as it has a wide range of features and language support - we'll
> also be able to use it to test a number of more specific CSS properties once we
> implement those).

Can you check in the fonts for these so that I can run the tests using them?

Comment 38

[John Daggett (:jtd)]

As mentioned in IRC, the ref case (font-features-ref.html) needs to set the lang attribute to 'en' or some other language, otherwise these tests will fail when run under Turkish locales.

Comment 39

[Jonathan Kew [:jfkthame]]

Attachment: add reftests for font feature & language control - with explicit lang attribute

Added explicit lang=en to all the tests except where a different lang is specifically being tested, to give consistent behavior regardless of system locale.

Comment 40

[Jonathan Kew [:jfkthame]]

Pushed reftests:
http://hg.mozilla.org/mozilla-central/rev/165b87769964

Comment 41

[Eric Shepherd [:sheppy]]

Is the syntax in comment #0 still accurate, or is there someplace else I can go to look at syntax details? Or do I just need to pore over the examples and piece it together to write documentation?

Comment 42

[Jonathan Kew [:jfkthame]]

(In reply to comment #41)
> Is the syntax in comment #0 still accurate, 

Almost. The property is now -moz-font-feature-settings (following changes to the Editor's Draft of CSS3 Fonts). Note that there may well be further changes in the future as that draft evolves.

In addition to -moz-font-feature-settings, we have also implemented -moz-font-language-override. These correspond to the draft font-feature-settings and font-language-override properties described in http://dev.w3.org/csswg/css3-fonts/.

Comment 43

[Eric Shepherd [:sheppy]]

This got documented yesterday or day before.