Implement CSS support for the font-variation-settings property

RESOLVED FIXED in Firefox 53

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
9 months ago
6 months ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla53
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(10 attachments, 6 obsolete attachments)

1.79 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
1.22 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
4.67 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
1.57 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
8.75 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
2.50 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
8.09 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
8.23 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
19.21 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
3.74 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
(Assignee)

Description

9 months ago
See https://drafts.csswg.org/css-fonts-4/#low-level-font-variation-settings-control-the-font-variation-settings-property.

As a first step towards OpenType Variation Fonts support, we should implement the low-level font-variation-settings property. (This is analogous to the approach we took with OpenType Features, where we initially implemented font-feature-settings, and subsequently added support for the specific properties for well-known features such as ligatures. This is also the implementation sequence Webkit is following.)

I think we'll want to store variation settings in nsFont and gfxFontStyle similarly to the existing support for feature settings. The CSS parsing, etc., isn't quite identical to font-feature-settings, as we have a list of <tag, number> pairs rather than <tag> having an optional value that can be a keyword or integer, but it'll be similar to that existing case.
(Assignee)

Comment 1

9 months ago
Created attachment 8815365 [details] [diff] [review]
pt 1 - Add a gfxFontVariation struct to represent a <variation-axis, value> pair
Attachment #8815365 - Flags: review?(dholbert)
(Assignee)

Updated

9 months ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Comment 2

9 months ago
Created attachment 8815366 [details] [diff] [review]
pt 2 - Add an array of font variations to gfxFontStyle
Attachment #8815366 - Flags: review?(dholbert)
(Assignee)

Comment 3

9 months ago
Created attachment 8815367 [details] [diff] [review]
pt 2.1 - While we're here, remove an obsolete declaration
Attachment #8815367 - Flags: review?(dholbert)
(Assignee)

Comment 4

9 months ago
Created attachment 8815368 [details] [diff] [review]
pt 3 - Add an array of variation settings to nsFont, and hook it up to gfxFontStyle
Attachment #8815368 - Flags: review?(dholbert)
(Assignee)

Comment 5

9 months ago
Created attachment 8815369 [details] [diff] [review]
pt 4 - Implement CSS parsing of the font-variations-setting property, storing the value into nsFont
Attachment #8815369 - Flags: review?(dholbert)
(Assignee)

Comment 6

9 months ago
Created attachment 8815370 [details] [diff] [review]
pt 4.1 - Regenerate the devtools property db (auto-generated using ./mach devtools-css-db)
Attachment #8815370 - Flags: review?(dholbert)
(Assignee)

Comment 7

9 months ago
Created attachment 8815371 [details] [diff] [review]
pt 5 - Add support for animating the font-variations-setting property
Attachment #8815371 - Flags: review?(dholbert)
(Assignee)

Updated

9 months ago
Blocks: 1321031
(Assignee)

Comment 8

9 months ago
Comment on attachment 8815369 [details] [diff] [review]
pt 4 - Implement CSS parsing of the font-variations-setting property, storing the value into nsFont

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

::: modules/libpref/init/all.js
@@ +2542,5 @@
>  // Is support for the font-display @font-face descriptor enabled?
>  pref("layout.css.font-display.enabled", false);
>  
> +// Is support for variation fonts enabled?
> +pref("layout.css.font-variations.enabled", true);

Oops, I meant to make this initially 'false' by default, and then we'll have a separate bug about flipping it to 'true' (nightly-only at first, I expect). I'll fix that in my local queue.
Attachment #8815367 - Flags: review?(dholbert) → review+
Attachment #8815370 - Flags: review?(dholbert) → review+
Comment on attachment 8815365 [details] [diff] [review]
pt 1 - Add a gfxFontVariation struct to represent a <variation-axis, value> pair

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

r=me -- one nit:

::: gfx/thebes/gfxFontVariations.h
@@ +14,5 @@
> +};
> +
> +inline bool
> +operator<(const gfxFontVariation& a, const gfxFontVariation& b)
> +{

This less-than operator doesn't seem to be exercised by any of your patches right now. (I tried applying the full patch stack, and then commenting out this operator, and I could still compile successfully.

So: probably remove it (unless it truly is needed, in a way that my compiler is missing)?

If you have plans to use this operator in a later bug (e.g. for sorting), I guess it's fine to add it here, though it might arguably be cleaner to add it as part of the tentative-patch that uses it in that later bug (so if we end up taking a different route & never needing it after all, we don't end up stuck with it as dead code).

(The other operator here, operator==, *does* seem to be used.  I tried removing that one as well, and I got a compile error, because we exercise it as part of "does this nsTArray<gfxFontVariation> instance equal this other one" comparison.)
Attachment #8815365 - Flags: review?(dholbert) → review+
Comment on attachment 8815366 [details] [diff] [review]
pt 2 - Add an array of font variations to gfxFontStyle

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

r- for now, since this patch is pretty small and I'm suggesting changes to half of its chunks (and it'll be trivial to re-review after those are addressed)

::: gfx/thebes/gfxFont.cpp
@@ +3959,5 @@
>      variantCaps(aStyle.variantCaps),
>      variantSubSuper(aStyle.variantSubSuper)
>  {
>      featureSettings.AppendElements(aStyle.featureSettings);
> +    variationSettings.AppendElements(aStyle.variationSettings);

Why are we handling nsTArrays differently from all the other copied member variables, in this copy-constructor? Can't we just use the nsTArray copy-constructor in the init list?  i.e. coudln't we just insert this into the init list:
     variationSettings(aStyle.variationSettings),

If we can (and it's not extra-cost for some reason), we should, since that's more declarative and consistent.

So: maybe spin off a (preliminary) helper-patch to convert the current AppendElements calls here into the init list, and then fix this patch so that it uses the init list as well?

::: gfx/thebes/gfxFont.h
@@ +216,3 @@
>              (languageOverride == other.languageOverride) &&
>              (alternateValues == other.alternateValues) &&
>              (featureValueLookup == other.featureValueLookup);

Tangent: It looks like "alternateValues" & "featureValueLookup" comparisons here are out-of-order, with respect to how these member-vars are declared in the .cpp file. They belong up just after "featureSettings".

Could you post a trivial helper-patch to reorder those into the correct position in this list of comparisons, so that it's easier to verify that we haven't skipped any member-vars here? (This could be part of the same helper-patch I'm suggesting in my other review-nit on this patch, if you like.)
Attachment #8815366 - Flags: review?(dholbert) → review-
(If it's not clear: the second review-nit in comment 10 is about the contextual code *directly* after your inserted line, BTW -- it's about fixing the list that you're inserting into, to make it clearer that the insertion is in the correct position.)
And for the first review-nit in comment 10: do we actually need the gfxFontStyle copy-constructor at all? (the thing that you're modifying there)  At first glance, it looks like we're just manually doing what the default copy-constructor does.  Maybe we should really take a helper-patch that deletes that copy-constructor entirely?
Flags: needinfo?(jfkthame)
Attachment #8815368 - Flags: review?(dholbert) → review+
Comment on attachment 8815371 [details] [diff] [review]
pt 5 - Add support for animating the font-variations-setting property

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

Partial review of Part 5:

::: layout/style/StyleAnimationValue.cpp
@@ +4505,5 @@
>          }
>  
> +        case eCSSProperty_font_variation_settings: {
> +          const nsStyleFont *font =
> +            static_cast<const nsStyleFont*>(styleStruct);

Snuggle pointer star with the type, not the variable name.
  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Declarations

BUT, probably-better in this partiuclar case: feel free to use "auto" here, so that these 2 lines just become:
  auto font = static_cast<const nsStyleFont*>(styleStruct);

(I think there's consensus that "auto" is a Good Thing where the type is unambiguously defined elsewhere on the line, as it is in static_cast<Foo*> & "new Foo()" operations.)

@@ +4506,5 @@
>  
> +        case eCSSProperty_font_variation_settings: {
> +          const nsStyleFont *font =
> +            static_cast<const nsStyleFont*>(styleStruct);
> +          nsAutoPtr<nsCSSValuePairList> result;

Could you write this using UniquePtr rather than nsAutoPtr, as we do in at least one other spot in this file for nsCSSValuePairList?  We're actively trying to migrate from nsAutoPtr to UniquePtr (and we've done some work in this file to make that conversion already) so we shouldn't be adding new nsAutoPtr usages unless absolutely necessary.

This will mean your result.forget() line further down will need to be replaced with "result.release()". ("release" is the UniquePtr equivalent of nsAutoPtr::forget())

(Downside is you don't have getter_AddRefs, which is convenient albeit a bit magical.  The lack of getter_AddRefs might make this a bit more clumsy, but hopefully not too much...)

@@ +4509,5 @@
> +            static_cast<const nsStyleFont*>(styleStruct);
> +          nsAutoPtr<nsCSSValuePairList> result;
> +          if (!font->mFont.fontVariationSettings.IsEmpty()) {
> +            // Make a new list that clones the current settings
> +            nsCSSValuePairList **resultTail = getter_Transfers(result);

Snuggle pointer star with the type, not the variable name.

@@ +4511,5 @@
> +          if (!font->mFont.fontVariationSettings.IsEmpty()) {
> +            // Make a new list that clones the current settings
> +            nsCSSValuePairList **resultTail = getter_Transfers(result);
> +            for (auto v : font->mFont.fontVariationSettings) {
> +              nsCSSValuePairList *clone = new nsCSSValuePairList;

Snuggle pointer star with the type, not the variable name.

@@ +4515,5 @@
> +              nsCSSValuePairList *clone = new nsCSSValuePairList;
> +              nsAutoString tagString;
> +              tagString.Append(char(v.mTag >> 24));
> +              tagString.Append(char(v.mTag >> 16));
> +              tagString.Append(char(v.mTag >> 8));

Please include a comment here to explain this mysterious bit-shifting.

@@ +4518,5 @@
> +              tagString.Append(char(v.mTag >> 16));
> +              tagString.Append(char(v.mTag >> 8));
> +              tagString.Append(char(v.mTag));
> +              clone->mXValue = nsCSSValue(tagString, eCSSUnit_String);
> +              clone->mYValue = nsCSSValue(v.mValue, eCSSUnit_Number);

Can you use mXValue.SetStringValue & mYValue.SetFloatValue here?  That seems more direct than constructing an extra temporary nsCSSValue and assigning it.

(If we didn't have these setters, then what you've got here would be necessary, I think. But since we've got the setters, we might as well use them.)
(Sorry -- when I said getter_AddRefs I meant to say getter_Transfers)
(Assignee)

Updated

9 months ago
Depends on: 1321512
Keywords: dev-doc-needed
Created attachment 8816227 [details] [diff] [review]
pt 6: Use UniquePtr instead of nsAutoPtr (& nsCSSValue setters) in font-variations-setting StyleAnimationValue code

Here's a patch to addresse all of my review feedback from comment 13 -- the s/nsAutoPtr+getter_Transfers/UniquePtr+Move/ conversion is subtle, so I just went ahead and did it in a followup patch, since I reviewed another patch that did something similar recently & I have that conversion-code handy.

This change is based on the nsAutoPtr/getter_Transfers removal here:
  https://hg.mozilla.org/mozilla-central/rev/9900b2f32860#l1.127
...which used a new helper function called "AppendToCSSValueList" (which I've copypasted here as AppendToCSSValuePairList).

I'm hoping that I'm posting this "part 6" soon enough that I'm not duplicating work that you've already done. :) (or perhaps you've started exploring this conversion, but this patch will save you the work of getting it just right)
Attachment #8816227 - Flags: review?(jfkthame)
(In reply to Daniel Holbert [:dholbert] from comment #15)
> Created attachment 8816227 [details] [diff] [review]
> Here's a patch to addresse [sic] all of my review feedback from comment 13

(Er, it addressed all of the feedback except for one thing -- this chunk of code still needs a comment to explain the " tagString.Append(char(v.mTag >> 24));" bit-shifting.  Please still do that in "pt 5".  Feel free to make any other changes you like in pt 5, too (e.g. if you'd already locally addressed some of the other feedback before I posted pt 6), and I'll gladly rebase pt 6 on top of the updates.
Comment on attachment 8815369 [details] [diff] [review]
pt 4 - Implement CSS parsing of the font-variations-setting property, storing the value into nsFont

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

r- for now on part 4, since there's enough changing here that it probably merits another round of review.

(But, this will probably be r+ with this feedback addressed.)

::: layout/style/nsCSSParser.cpp
@@ +15364,5 @@
> +                              nullptr)) {
> +    return true;
> +  }
> +
> +  nsCSSValuePairList *cur = aValue.SetPairListValue();

Two nits:
 (1) The * should be snuggled to the left.
 (2) Generally, in functions that have an outparam & may fail, we aim to leave the outparam untouched except when we succeed.  We're violating that guideline here (since at this point we don't know yet if we're going to succeed or fail).  It looks like this code is copypasted from ParseFontFeatureSettings, so ideally it'd be great to either fix up that existing function beforehand (and then copy the corrected pattern into this code) -- though you could also fix up both in a followup patch.  Could you do one or the other of those?

Here, "fix up" would mean replacing the SetPairListValue call with:
    auto resultHead = MakeUnique<nsCSSValuePairList>();
    nsCSSValuePairList* cur = resultHead.get();
...and then adding at the very bottom, just before we return true:
    aValue.AdoptPairListValue(Move(resultHead));

@@ +15381,5 @@
> +    cur->mXValue.SetStringValue(mToken.mIdent, eCSSUnit_String);
> +
> +    if (!GetToken(true)) {
> +      UngetToken();
> +      return false;

Drop the "UngetToken()" here -- this is the error-handling clause that we enter when "GetToken()" fails, so there shouldn't be anything for us to Unget.

::: layout/style/nsRuleNode.cpp
@@ +3965,5 @@
> +  const nsCSSValue* variationSettingsValue =
> +    aRuleData->ValueForFontVariationSettings();
> +
> +  switch (variationSettingsValue->GetUnit()) {
> +  case eCSSUnit_Null:

Please indent the case statements (really the entire body of the switch {...}) by 2 spaces, per coding style guide:
 https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

(Similar to elsewhere, it looks like this is copypasting preexisting badness from some nearby "font-feature-settings" code. You might consider fixing up that existing font-feature-settings code in a helper-patch.)

@@ +3976,5 @@
> +
> +  case eCSSUnit_Inherit:
> +  case eCSSUnit_Unset:
> +    aConditions.SetUncacheable();
> +    aFont->mFont.fontVariationSettings = aParentFont->mFont.fontVariationSettings;

This line is a bit too long - please wrap after the "=".

@@ +4155,5 @@
>    }
>  }
>  
> +/* static */ void
> +nsRuleNode::ComputeFontVariations(const nsCSSValuePairList *aVariationsList,

Snuggle * to the left.

@@ +4160,5 @@
> +                                  nsTArray<gfxFontVariation>& aVariationSettings)
> +{
> +  aVariationSettings.Clear();
> +  for (const nsCSSValuePairList* p = aVariationsList; p; p = p->mNext) {
> +    gfxFontVariation var = {0, 0};

Replace the second "0" with "0.0f" please, to be a bit more explicit. (gfxFontVariation's members are uint32_t and float)

(Alternately, it looks like you could just leave "var" uninitialized here, since we set its members in all cases below this. And the compiler should be able to tell us [with fatal warnings] if we accidentally leave one of the members unset.)

@@ +4165,5 @@
> +
> +    MOZ_ASSERT(aVariationsList->mXValue.GetUnit() == eCSSUnit_String,
> +               "unexpected value unit");
> +
> +    // tag is a 4-byte ASCII sequence

Right now there aren't any assertions to validate this statement.  Could you add one?  Unfortunately we don't have an "IsOnlyASCII()" method that we could use, but I think we can still check that pretty easily, using EqualsASCII() and NS_LossyConvertUTF16toASCII().

So: could you add something like this here:
    MOZ_ASSERT(tag.Length() == 4 &&
               tag.EqualsASCII(NS_LossyConvertUTF16toASCII(tag).get()),
               "parser accepted tag string w/ invalid length or character");

(Perhaps it'd be nice to put this in a debug-only helper method here, AssertValidFontFeatureTag(), which you could call from both this function (ComputeFontVariations) and from ComputeFontFeatures.)

@@ +4169,5 @@
> +    // tag is a 4-byte ASCII sequence
> +    nsAutoString tag;
> +    p->mXValue.GetStringValue(tag);
> +    if (tag.Length() != 4) {
> +      continue;

(This is perhaps still worth leaving in, even after you've added the assertion above, as a belt-and-suspenders protection against uninitialized reads in opt builds.)

::: layout/style/nsRuleNode.h
@@ +1023,5 @@
>  
>    static void ComputeFontFeatures(const nsCSSValuePairList *aFeaturesList,
>                                    nsTArray<gfxFontFeature>& aFeatureSettings);
>  
> +  static void ComputeFontVariations(const nsCSSValuePairList *aVariationsList,

Snuggle * to the left.

::: layout/style/nsStyleUtil.cpp
@@ +387,5 @@
> +  for (uint32_t i = 0, numVars = aVariations.Length(); i < numVars; i++) {
> +    const gfxFontVariation& var = aVariations[i];
> +
> +    if (i != 0) {
> +        aResult.AppendLiteral(", ");

Nit: deindent this line by 2 spaces.

::: layout/style/test/property_database.js
@@ +5695,5 @@
> +    other_values: [
> +      "'wdth' 0", "'wdth' -.1", "\"wdth\" 1", "'wdth' 2, 'wght' 3", "\"XXXX\" 0"
> +    ],
> +    invalid_values: [
> +      "wdth", "wdth 1", "\"wdth\"", "\"wdth\" \"wght\""

A few more invalid values we should include here, to fully exercise the edge cases in the parsing logic you're adding in nsCSSValue:
 - Several otherwise-valid values, whose quoted-strings are just the wrong length:
   * 0 characters (edge case)
   * 3 characters (too low)
   * 5 characters (too high)
 - An otherwise-valid value, which is a pair-list of length 2 and is missing the comma between its entries.
 - An otherwise-valid value, which has a trailing comma.
 - An otherwise-valid value, which is a pair-list of length 2 and has an extra comma between its entries (,,)
 - An otherwise-valid value that has a comma between the string & the integer.
 - An otherwise-valid value that has mimatched quote characters around its <string>, e.g. 'wdth\" 1

::: modules/libpref/init/all.js
@@ +2542,5 @@
>  // Is support for the font-display @font-face descriptor enabled?
>  pref("layout.css.font-display.enabled", false);
>  
> +// Is support for variation fonts enabled?
> +pref("layout.css.font-variations.enabled", true);

(Do remember to fix this to "false", if you haven't already done so.)
Attachment #8815369 - Flags: review?(dholbert) → review-
(Assignee)

Comment 18

9 months ago
Created attachment 8816531 [details] [diff] [review]
pt 1.5 - Delete the redundant copy constructor in gfxFontStyle (default copy constructor is fine), and rationalize field ordering a bit

As suggested, this deletes the gfxFontStyle copy constructor; I've also made the ordering of the fields a little more sane, and 'style' is no longer a 2-bit bitfield, as there's no benefit to that given that we have 7 individual boolean bits. Note that the order of fields in the Equals() method is still not simply a copy of the order in the struct: the major fields that seem most likely to differ are put first, in the hope of more frequent short-circuit results.
Attachment #8816531 - Flags: review?(dholbert)
(Assignee)

Comment 19

9 months ago
Created attachment 8816532 [details] [diff] [review]
pt 2 - Add an array of font variations to gfxFontStyle
Attachment #8816532 - Flags: review?(dholbert)
(Assignee)

Updated

9 months ago
Attachment #8815366 - Attachment is obsolete: true
(Assignee)

Comment 20

9 months ago
Created attachment 8816536 [details] [diff] [review]
pt 4 - Implement CSS parsing of the font-variations-setting property, storing the value into nsFont
Attachment #8816536 - Flags: review?(dholbert)
(Assignee)

Updated

9 months ago
Attachment #8815369 - Attachment is obsolete: true
(Assignee)

Comment 21

9 months ago
Created attachment 8816537 [details] [diff] [review]
pt 4.2 - Fix up some nits in existing font-feature-settings code as per review comments on the new font-variation-settings code
Attachment #8816537 - Flags: review?(dholbert)
(Assignee)

Comment 22

9 months ago
Created attachment 8816539 [details] [diff] [review]
pt 5 - Add support for animating the font-variations-setting property

I've added a comment here, but left the rest of the fixups for your pt6. (Thanks for posting that.)
Attachment #8816539 - Flags: review?(dholbert)
(Assignee)

Updated

9 months ago
Attachment #8815371 - Attachment is obsolete: true
Attachment #8815371 - Flags: review?(dholbert)
(Assignee)

Comment 23

9 months ago
Created attachment 8816540 [details] [diff] [review]
pt 4 - Implement CSS parsing of the font-variations-setting property, storing the value into nsFont
Attachment #8816540 - Flags: review?(dholbert)
(Assignee)

Updated

9 months ago
Attachment #8816536 - Attachment is obsolete: true
Attachment #8816536 - Flags: review?(dholbert)
(Assignee)

Comment 24

9 months ago
Comment on attachment 8816227 [details] [diff] [review]
pt 6: Use UniquePtr instead of nsAutoPtr (& nsCSSValue setters) in font-variations-setting StyleAnimationValue code

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

LGTM, thanks for this!
Attachment #8816227 - Flags: review?(jfkthame) → review+
(Assignee)

Updated

9 months ago
Flags: needinfo?(jfkthame)
Comment on attachment 8816531 [details] [diff] [review]
pt 1.5 - Delete the redundant copy constructor in gfxFontStyle (default copy constructor is fine), and rationalize field ordering a bit

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

r=me
Attachment #8816531 - Flags: review?(dholbert) → review+
Comment on attachment 8816532 [details] [diff] [review]
pt 2 - Add an array of font variations to gfxFontStyle

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

r=me, optional nit:

::: gfx/thebes/gfxFont.h
@@ +104,5 @@
>  
>      // -- object used to look these up once the font is matched
>      RefPtr<gfxFontFeatureValueSet> featureValueLookup;
>  
> +    // opentype variation settings

Nit: should we fix the capitalization to s/opentype/OpenType/? (I think that's how we tend to spell this elsewhere; not sure how much it matters)
Attachment #8816532 - Flags: review?(dholbert) → review+
Comment on attachment 8816537 [details] [diff] [review]
pt 4.2 - Fix up some nits in existing font-feature-settings code as per review comments on the new font-variation-settings code

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

r=me -- thanks for fixing this up!

::: layout/style/nsRuleNode.cpp
@@ +4161,5 @@
>                SETFCT_NONE | SETFCT_UNSET_INHERIT);
>  }
>  
> +static inline void
> +AssertValidFontTag(const nsString& aString)

Optional nit: in this patch, we're moving this function (without modification) up 50 lines or so.

Maybe just move it to the correct final spot in Pt 4, where it's created?  And then this patch won't need to touch it. *shrug*  Not a big deal, I guess.
Attachment #8816537 - Flags: review?(dholbert) → review+
Comment on attachment 8816539 [details] [diff] [review]
pt 5 - Add support for animating the font-variations-setting property

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

Thanks - the new comment is quite helpful here! r=me
Attachment #8816539 - Flags: review?(dholbert) → review+
Created attachment 8816570 [details] [diff] [review]
pt 6, v2 (rebased): Use UniquePtr instead of nsAutoPtr (& nsCSSValue setters) in font-variations-setting StyleAnimationValue code

Here's pt 6 again, rebased on top of the changes to pt5 (the new comment in contextual code).

Carrying forward r+ from comment 24.
Attachment #8816570 - Flags: review+
Attachment #8816227 - Attachment is obsolete: true
Created attachment 8816571 [details] [diff] [review]
pt 6, v3 (rebased): Use UniquePtr instead of nsAutoPtr (& nsCSSValue setters) in font-variations-setting StyleAnimationValue code

(Sorry - here's pt6 one more time, with s/r?/r=/ in the commit message so that it's ready to land).
Attachment #8816570 - Attachment is obsolete: true
Attachment #8816571 - Flags: review+
Comment on attachment 8816540 [details] [diff] [review]
pt 4 - Implement CSS parsing of the font-variations-setting property, storing the value into nsFont

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

r=me, just 2 notes:

::: layout/style/nsRuleNode.cpp
@@ +4193,5 @@
>  
> +static inline void
> +AssertValidFontTag(const nsString& aString)
> +{
> +  // To be valid as a font feature tag, a string MUST be:

(As noted for Pt 4.2: consider just moving this function into its correct final location (about 50 lines higher up) **in this patch**, so that Pt 4.2 doesn't have to move it around.)

::: layout/style/test/property_database.js
@@ +5706,5 @@
> +      "'wdth' 1 , , 'wght' 2", // extra comma
> +      "'wdth', 1" // comma within pair
> +    ],
> +    unbalanced_values: [
> +      "'wdth\" 1", "\"wdth' 1" // mismatched quotes

Just to satisfy my curiosity -- I'm not immediately clear on what makes this "unbalanced".  The quote-nesting seems "balanced" to me, even though it's a completely bogus value for this property.

Ignoring the escaped quotes and other characters, this is effectively just:
  ' "" '
...which seems balanced / nicely-nested.

And even if you consider the escaped quotes, it still seems balanced -- it looks like:
  ' \" " " \" '
The nesting there seems sound, even though it's an invalid value.  So: what is it that makes this unbalanced?
Attachment #8816540 - Flags: review?(dholbert) → review+
(Maybe any consecutive unescaped mismatched open-quote characters are just immediately considered imbalanced by our parser, even if they happen to be nested nicely?  I can imagine that being the case.  Anyway, unbalanced_values is new to me so I'm not entirely sure what sorts of mismatches make for unbalanced-ness.)
(Assignee)

Comment 33

9 months ago
> Ignoring the escaped quotes and other characters, this is effectively just:
>   ' "" '
> ...which seems balanced / nicely-nested.

No, I think you're misreading that. It's an array of two strings. Stripping off the double-quotes that are quoting the strings for the JS file, and unescaping the embedded double-quotes, the actual values we're testing here are:
    'wdth" 1
    "wdth' 1
both of which are unbalanced.

FWIW, unbalanced_values was new to me, too; but when I initially included the mismatched quotes in invalid_values, I got failures in test_property_syntax_errors.html.

Consider the first of the unbalanced_values examples: "'wdth\" 1". This means we'll be attempting to parse CSS that looks like:

  font-variation-settings: 'wdth" 1;

which has a string that begins with a single quote, and is not properly closed; therefore, parsing will "absorb" the declaration that follows, and that's what test_property_syntax_errors detects and complains about if it's in invalid_values. Putting this example into unbalanced_values instead means that behavior is the expected result.
(Assignee)

Comment 34

9 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee75de0e12acc2c7557daa437fdb4d5c53393304
Bug 1321022 pt 1 - Add a gfxFontVariation struct to represent a <variation-axis, value> pair. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0d84787e5643ab00cf79018e8f80b759b57a95b
Bug 1321022 pt 1.5 - Delete the redundant copy constructor in gfxFontStyle (default copy constructor is fine), and rationalize field ordering a bit. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9fadc2e73631344646748400a6419b320c3ed17
Bug 1321022 pt 2 - Add an array of font variations to gfxFontStyle. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/d97f80b77cb2ce37e9f4e6a2ef527bc399404a78
Bug 1321022 pt 2.1 - While we're here, remove an obsolete declaration. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/289358891b5f2d7c1f8d2383a83a9bf86258d9ce
Bug 1321022 pt 3 - Add an array of variation settings to nsFont, and hook it up to gfxFontStyle. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/763cff42073a4abf77f8586edb289d91eb751cc7
Bug 1321022 pt 4 - Implement CSS parsing of the font-variations-setting property, storing the value into nsFont. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/180d4d40de6b0089b2ccf41f00c43afd93b5d874
Bug 1321022 pt 4.1 - Regenerate the devtools property db (auto-generated using ./mach devtools-css-db). r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/b649cf92aa2d04afbfe454ccf31097eb41216b76
Bug 1321022 pt 4.2 - Fix up some nits in existing font-feature-settings code as per review comments on the new font-variation-settings code. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/7878d113285818e7a5ec92e4c43cd43a84728db3
Bug 1321022 pt 5 - Add support for animating the font-variations-setting property. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/870d19bb47592adec25736d9e7260bb0ca917fc0
Bug 1321022 pt 6: Use UniquePtr instead of nsAutoPtr (& nsCSSValue setters) in font-variations-setting StyleAnimationValue code. r=jfkthame
(Assignee)

Comment 35

9 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/71fff6495d23e355d5147d9673dfd5df30d3953c
Bug 1321022 followup, refresh devtools css db to fix xpcshell errors.

Comment 36

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ee75de0e12ac
https://hg.mozilla.org/mozilla-central/rev/c0d84787e564
https://hg.mozilla.org/mozilla-central/rev/e9fadc2e7363
https://hg.mozilla.org/mozilla-central/rev/d97f80b77cb2
https://hg.mozilla.org/mozilla-central/rev/289358891b5f
https://hg.mozilla.org/mozilla-central/rev/763cff42073a
https://hg.mozilla.org/mozilla-central/rev/180d4d40de6b
https://hg.mozilla.org/mozilla-central/rev/b649cf92aa2d
https://hg.mozilla.org/mozilla-central/rev/7878d1132858
https://hg.mozilla.org/mozilla-central/rev/870d19bb4759
https://hg.mozilla.org/mozilla-central/rev/71fff6495d23
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Updated

8 months ago
Blocks: 1323668
I've documented this property in the CSS reference:

https://developer.mozilla.org/en-US/docs/Web/CSS/font-variation-settings

The formal syntax/attributes that are currently erroring on the page have been added to the database, and the PR is just awaiting final signoff, so those errors should go soon.

I've also added a note to the Fx53 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/53#CSS

I think the only thing that's missing at this point is some other syntax examples, and a proper example - can you point me at some code that I could use/adapt to write these details up?
Keywords: dev-doc-needed → dev-doc-complete
Flags: needinfo?(jfkthame)
(Assignee)

Comment 38

6 months ago
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #37)
> I've documented this property in the CSS reference:
> 
> https://developer.mozilla.org/en-US/docs/Web/CSS/font-variation-settings
> 
> The formal syntax/attributes that are currently erroring on the page have
> been added to the database, and the PR is just awaiting final signoff, so
> those errors should go soon.

I notice that page says "The <number> is a positive integer.", which is not consistent with either the syntax example (font-variation-settings: "XHGT" 0.7;) above, or with the spec, which says "Values are allowed to be fractional or negative."[1]

> I've also added a note to the Fx53 release notes:
> 
> https://developer.mozilla.org/en-US/Firefox/Releases/53#CSS

I doubt we want this in release notes at this point; it's preffed off by default, and even if preffed on, it's functional only on one platform (macOS/Sierra). So it seems rather premature to put it in relnotes, IMO.

> I think the only thing that's missing at this point is some other syntax
> examples, and a proper example - can you point me at some code that I could
> use/adapt to write these details up?

There's a simple testcase at https://people-mozilla.org/~jkew/tests/skia-wght.html, and a fuller one (takes a moment to render, though!) at https://people-mozilla.org/~jkew/tests/skia.html.

See also http://axis-praxis.org/ for live examples (not all fonts on that site work, for various reasons, but it's a cool demo all the same).

(Remember that you'll need to enable the layout.css.font-variations.enabled pref in about:config before anything will work! And for the downloadable fonts on axis-praxis, you also need gfx.downloadable_fonts.keep_variation_tables, just landed on Nightly.)


[1] https://drafts.csswg.org/css-fonts-4/#low-level-font-variation-settings-control-the-font-variation-settings-property
Flags: needinfo?(jfkthame)
I've removed the entry from Fx 53 for devs, but I have added it on our Experimental features of Firefox page: https://developer.mozilla.org/en-US/Firefox/Experimental_features#CSS

Kyle: do you have a bug number about the downloadable fonts on axis-praxis?
Flags: needinfo?(jfkthame)
Kyle: an extra question! Do you think this feature is worth an entry (in development) under platform-status.mozilla.org ?
(Assignee)

Comment 41

6 months ago
(In reply to Jean-Yves Perrier [:teoli] from comment #39)
> do you have a bug number about the downloadable fonts on axis-praxis?

The gfx.downloadable_fonts.keep_variation_tables pref needed to allow downloadable fonts to have variation tables was added in bug 1341085.

(Somewhat relevant, there's also the gfx.downloadable_fonts.otl_validation pref recently added in bug 1331737; some of the fonts on axis-praxis require this set to 'false' in order to load successfully.)
(Assignee)

Comment 42

6 months ago
(In reply to Jean-Yves Perrier [:teoli] from comment #40)
> Kyle: an extra question! Do you think this feature is worth an entry (in
> development) under platform-status.mozilla.org ?

Yes, sounds like a good idea. There's quite a bit of interest in the font/design community around this.

BTW, who's Kyle? :)
Flags: needinfo?(jfkthame)
You need to log in before you can comment on or make changes to this bug.