1041951 - convert font-variant to a shorthand and parse font-variant subproperty values

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[John Daggett (:jtd)]

In the CSS3 Fonts spec, font-variant is defined to be a shorthand for other font-variant subproperties.

http://www.w3.org/TR/css3-fonts/#font-variant-prop

The CSS parser needs to be modified to accept the long list of subproperty values and correctly set all subproperties.

Since there's no ability to switch longhand/shorthand state based on a pref, this will need to land at the same time as the patches for bug 835191 and bug 975744.

Comment 1

[John Daggett (:jtd)]

Implementation details for parsing font-variant as a shorthand

- within font shorthand, only accept small-caps ==> font-variant-caps

- font-variant value parsing
  = handle normal/inherit/none as single values
  = use keyword to determine which subproperty value to set
  = rework guts of ParseBitmaskValues into helper method for single value
  = reset all subproperty values

- trim out old font-variant value from nsFont/gfxFontStyle

- computed value of font-variant
  = all subproperties normal ==> normal
  = font-variant-ligatures == none, other subproperties normal ==> none
  = font-variant-ligatures == none, other subproperties not normal ==> ???
  = computed value of non-normal subproperties in defined order

- computed value of font
  = hmmm, what happens when font-variant subproperties are not normal/small-caps?

- testing
  = test using mixture of valid/invalid values for subproperties (???)

Note that this patch will essentially ignore the font features pref setting, since there's no way to vary shorthand/longhand state based on a pref. So when this eventually lands it will need to land together with the patch to remove the pref (bug 975744).

Comment 2

[John Daggett (:jtd)]

Haven't quite worked out what to do about the serialized/computed values of font-variant should be (and font for that matter).

  font-variant-ligatures: none;
  font-variant-position: super;

  font-variant ==> ???

Or:

  font-variant: petite-caps;

  font ==> ??? (since only normal/small-caps are allowed in font shorthand)

Guessing empty string will be the answer here...

Comment 3

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

In general, the code for serializing shorthands should produce a nonempty value V if and only if setting the shorthand to that value V would leave the styles unchanged (i.e., expand to the same set of longhands).  (Though the concept of "unchanged" might be a little iffier for computed style.)

So comment 2 sounds correct (assuming that the font-variant shorthand has limitations that prevent it from expressing the first example).

Comment 4

[John Daggett (:jtd)]

Attachment: patch, parse font-variant as a shorthand (wip)

Add ability to parse all font-variant subproperty values within font-variant property. Still needs more work to pass various mochitests but basic functionality is there.

Comment 5

[John Daggett (:jtd)]

Attachment: patch, add additional reftests for font-variant shorthand

With patch applied, all these reftests pass.

Comment 6

[John Daggett (:jtd)]

Attachment: testcase, using var() for font-variant values doesn't serialize correctly

Both within existing trunk code and within code based on my patch, using a var() function for a 'font-variant' value doesn't always serialize correctly.

Using a custom variable for a value of 'font-variant':

--silly-bunt: small-caps

With 'font-variant: var(--silly-bunt)' you get:

getPropertyValue('font') ==> "var(--silly-bunt) 100% serif"
getPropertyValue('font-variant') ==> "var(--silly-bunt)"

With 'font: var(--silly-bunt) 100% serif' you get:

getPropertyValue('font') ==> "var(--silly-bunt) 100% serif"
getPropertyValue('font-variant') ==> ""

It seems to me these should both serialize identically.

Cam, could you point me at the code I should look at to track this down?

Comment 7

[Cameron McCormack (:heycam)]

Pointed out in IRC that the first getPropertyValue("font") should probably return "", since we cannot guarantee that "var(-silly-bunt) 100% serif" will expand out to the same longhands (the requirement mentioned in comment 3).

So I think that means that serializing any shorthand that has a longhand component with a variable reference in it must return "".

The code to handle serializing shorthands Declaration::GetValue in layout/style/Declaration.cpp.  (And bug 137688 is for shorthand serialization not being implemented on computed style declaration objects yet.)

Comment 8

[John Daggett (:jtd)]

I'm seeing a mochitest failure running test_value_storage.html but I think this is a general bug related to the serialization of subproperties that are shorthands. I filed bug 1053114 to fix this.

Comment 9

[John Daggett (:jtd)]

Attachment: patch, parse font-variant as a shorthand for font feature subproperties

Comment 10

[John Daggett (:jtd)]

Note: parsing patch is on top of the patch for fixing bug 1053114.

Comment 11

[John Daggett (:jtd)]

Attachment: patch, fix SMIL test util code that is creating a pseudo computed value for shorthands

The computed value of 'font' should use 'font-variant-caps' rather than 'font-variant' and 'font-variant' is now a shorthand so need to special case it also.

Comment 12

[John Daggett (:jtd)]

Attachment: patch, fix up devtools test that assumes font-variant is a longhand

The 'font-variant' property is now a shorthand, so the set of longhand values affected is different from before. Switch the test to set the longhand property for the 'small-caps' value.

Comment 13

[John Daggett (:jtd)]

Attachment: patch v2b, parse font-variant as a shorthand for font feature subproperties

Slight tweak to avoid serializing 'normal' values for font-variant when serializing 'font'.

Comment 14

[Brian Birtles (:birtles)]

Comment on attachment 8473471 [details] [diff] [review]
patch, fix SMIL test util code that is creating a pseudo computed value for shorthands

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

r=me with comments addressed

::: dom/smil/test/smilTestUtils.js
@@ +57,2 @@
>    // Smart wrapper for getComputedStyle, which will generate a "fake" computed
>    // style for recognized shorthand properties (font, overflow, marker)

I think this should now include font-variant

@@ +78,5 @@
>            }
>          }
>        }
> +    } else if (propName == "font-variant") {
> +      computedStyle = SMILUtil.getComputedStyleSimple(elem, "font-variant-caps");

We should probably add a comment here saying that this isn't complete (i.e. doesn't include all the sub-properties) but is enough for testing purposes.

Comment 15

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

Comment on attachment 8473478 [details] [diff] [review]
patch v2b, parse font-variant as a shorthand for font feature subproperties

+        // only a normal or small-caps values of font-variant-caps can
+        // be represented in the font shorthand
+        bool variantEnum = fontVariantCaps->GetUnit() == eCSSUnit_Enumerated;
+        bool variantSmallCaps = (variantEnum &&
+          fontVariantCaps->GetIntValue() == NS_FONT_VARIANT_CAPS_SMALLCAPS);
+        if (variantEnum && !variantSmallCaps) {
+          return;
+        }

I'm not crazy about assuming that normal and enum are the only possible
units.  It might be better to test:

      if (fontVariantCaps->GetUnit() != eCSSUnit_Normal &&
          (fontVariantCaps->GetUnit() != eCSSUnit_Enumerated ||
           fontVariantCaps->GetIntValue() != NS_FONT_VARIANT_CAPS_SMALLCAPS)) {
        return;
      }

(I think it might also be clearer without the extra bools.)


>-        if (variant->GetUnit() != eCSSUnit_Enumerated ||
>-            variant->GetIntValue() != NS_FONT_VARIANT_NORMAL) {
>-          variant->AppendToString(eCSSProperty_font_variant, aValue,
>+        if (!variantEnum || variantSmallCaps) {
>+          fontVariantCaps->AppendToString(eCSSProperty_font_variant_caps, aValue,
>                                   aSerialization);
>           aValue.Append(char16_t(' '));
>         }

This appends a 'normal' where it didn't before.  In fact, it seems like
this always appends the fontVariantCaps value.

I think the test should just be:
      if (fontVariantCaps->GetUnit() != eCSSUnit_Normal)
(Note that font-variant-caps differs from some other font properties in
storing 'normal' as eCSSUnit_Normal, whereas some others store it as
eCSSUnit_Enumerated.)

(This is the only thing you changed in the new version of the patch, but
I think you need only this condition.)

>+      // in the system font case, skip over font-variant shorthand, since all
>+      // subproperties are already dealt with via the font shorthand
>+      if (shorthand == eCSSProperty_font_variant &&
>+          value.EqualsLiteral("-moz-use-system-font")) {
>+        continue;
>+      }

Instead of this (and the reordering of GetValue and the comment), I
think you should make the change:

>         // That we output the system font is enough for this property if:
>         //   (1) it's the hidden system font subproperty (which either
>         //       means we output it or we don't have it), or
>         //   (2) its value is the hidden system font value and it matches
>         //       the hidden system font subproperty in importance, and
>         //       we output the system font subproperty.
>         const nsCSSValue *val = systemFontData->ValueFor(property);
>         if (property == eCSSProperty__x_system_font ||
>             (haveSystemFont && val && val->GetUnit() == eCSSUnit_System_Font)) {
>           doneProperty = true;
>+          break;
>         }

which fixes an existing bug and avoids the need for your change.


>+  } else {
>+    if (values[1].GetUnit() == eCSSUnit_Enumerated &&
>+        !values[1].GetIntValue() == NS_FONT_VARIANT_CAPS_SMALLCAPS) {
>+      return false; // only normal or small-caps is allowed in font shorthand
>+    }

Could you add a comment that this code is depending on
'font-variant-caps' not having any keyword values that are valid for
the other properties at the beginning of the font shorthand.  (I think
it's ok, but it deserves a comment pointing it out, since if there was
any overlap, you'd be introducing a bug by failing to parse the shorthand
when that value should instead have gone to a different property.)

In MergeBitmaskValue, I'd prefer aMergedValue being the last parameter
on the principle that out or in/out params come at the end.


in ParseFontVariant:

>+    if (eCSSUnit_None == value.GetUnit()) {
>+      AppendValue(eCSSProperty_font_variant_alternates, normal);
>+      AppendValue(eCSSProperty_font_variant_caps, normal);
>+      AppendValue(eCSSProperty_font_variant_east_asian, normal);
>+      AppendValue(eCSSProperty_font_variant_ligatures, value);
>+      AppendValue(eCSSProperty_font_variant_numeric, normal);
>+      AppendValue(eCSSProperty_font_variant_position, normal);
>+    } else {
>+      AppendValue(eCSSProperty_font_variant_alternates, value);
>+      AppendValue(eCSSProperty_font_variant_caps, value);
>+      AppendValue(eCSSProperty_font_variant_east_asian, value);
>+      AppendValue(eCSSProperty_font_variant_ligatures, value);
>+      AppendValue(eCSSProperty_font_variant_numeric, value);
>+      AppendValue(eCSSProperty_font_variant_position, value);
>+    }

This would be simpler as:
      AppendValue(eCSSProperty_font_variant_ligatures, value);
      if (eCSSUnit_None == value.GetUnit()) {
        // 'none' applies the value 'normal' to all properties other
        // than 'font-variant-ligatures'
        value.SetNormalValue();
      }
      AppendValue(eCSSProperty_font_variant_alternates, value);
      AppendValue(eCSSProperty_font_variant_caps, value);
      AppendValue(eCSSProperty_font_variant_east_asian, value);
      AppendValue(eCSSProperty_font_variant_numeric, value);
      AppendValue(eCSSProperty_font_variant_position, value);
and you can then also remove the |normal| variable.

>+    int feature;

int32_t, not int


As described in:
http://lists.w3.org/Archives/Public/www-style/2014Aug/0221.html
your implementation doesn't match the spec.  I think the spec is wrong,
but you should fix one or the other.


>+      if (!nsCSSProps::FindKeyword(keyword,
>+                                   nsCSSProps::kFontVariantAlternatesFuncsKTable,
>+                                   feature) ||
>+          (feature & altFeatures)) {
>+        UngetToken();
>+        return false;
>+      }
>+
>+      altFeatures |= feature;
>+      uint16_t maxElems = 1;
>+      if (keyword == eCSSKeyword_styleset ||
>+          keyword == eCSSKeyword_character_variant) {
>+        maxElems = MAX_ALLOWED_FEATURES;
>+      }
>+      nsCSSValue funcValue;
>+      if (!ParseFunction(keyword, nullptr, VARIANT_IDENTIFIER, 1,
>+                         maxElems, funcValue) ||
>+          funcValue.GetUnit() != eCSSUnit_Function) {
>+        UngetToken();
>+        return false;
>+      }

It seems like you should reuse ParseSingleAlternate instead of copying
most of its contents.

>+    } else {
>+        // bogus keyword, bail...
>+        UngetToken();
>+        return false;
>+    }

2-space indent, please.

In nsComputedDOMStyle, you should keep an implementation for font-variant
and return something non-empty for the cases where you can -- or at a minimum
for the CSS 2.1 cases.

property_database.js:
  I think you should probably use CSS_TYPE_SHORTHAND_AND_LONGHAND
  instead of CSS_TYPE_TRUE_SHORTHAND, since this property was previously a
  longhand.

  However, if that causes a bunch of failures after the nsComputedDOMStyle
  fixes above, I"m likely to reconsider.

  Please list significantly more variations of valid and invalid values.
  In particular, please list a bunch of combinations that are invalid due to
  repeating values from font-variant-{position,caps}, and some that are
  valid with repeated separated values from the other subproperties.


(One other nit:  I think, in nsCSSPropList.h, font-variant-alternates
should have 0 in the variant field instead of VARIANT_HK, since it
uses CSS_PROPERTY_VALUE_PARSER_FUNCTION which I *think* means the variant
field should be unused.  At the very least, it doesn't match
font-variant-east-asian and font-variant-ligatures.  This briefly confused
me while reviewing.)



review- mainly because I'd like to look at the revised nsComputedDOMStyle code.

Comment 16

[John Daggett (:jtd)]

Attachment: patch v2c, parse font-variant as a shorthand for font feature subproperties

Updated based on review comments.

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #15)

> (Note that font-variant-caps differs from some other font properties in
> storing 'normal' as eCSSUnit_Normal, whereas some others store it as
> eCSSUnit_Enumerated.)

It ends up simplifying the code to handle normal/none as explicit values across all font-variant-* properties, since the normal/none values can only appear at the first position within the shorthand and nowhere else. I realize that's not how font-style or font-stretch handles normal but, meh, I think it's more important to be consistent across the font-variant/font-variant-* subproperties and not require extra checks for value == normal for the second or later value when parsing 'font-variant'.

> >+      // in the system font case, skip over font-variant shorthand, since all
> >+      // subproperties are already dealt with via the font shorthand
> >+      if (shorthand == eCSSProperty_font_variant &&
> >+          value.EqualsLiteral("-moz-use-system-font")) {
> >+        continue;
> >+      }
> 
> Instead of this (and the reordering of GetValue and the comment), I
> think you should make the change:
> 
> >         // That we output the system font is enough for this property if:
> >         //   (1) it's the hidden system font subproperty (which either
> >         //       means we output it or we don't have it), or
> >         //   (2) its value is the hidden system font value and it matches
> >         //       the hidden system font subproperty in importance, and
> >         //       we output the system font subproperty.
> >         const nsCSSValue *val = systemFontData->ValueFor(property);
> >         if (property == eCSSProperty__x_system_font ||
> >             (haveSystemFont && val && val->GetUnit() == eCSSUnit_System_Font)) {
> >           doneProperty = true;
> >+          break;
> >         }
> 
> which fixes an existing bug and avoids the need for your change.

I tested this and it doesn't. The problematic example is:

  font: menu; font-family: inherit;

When any font-variant-* longhand property is handled, the value of 'font' shorthand will be empty but 'font-variant' will not. That's the case handled by the conditional. Without this we end up with "font: menu; font-family: inherit: font-variant: -moz-use-system-font" as the value in the example above.

> As described in:
> http://lists.w3.org/Archives/Public/www-style/2014Aug/0221.html
> your implementation doesn't match the spec.  I think the spec is wrong,
> but you should fix one or the other.

Right, this is a spec omission, response here:

http://lists.w3.org/Archives/Public/www-style/2014Aug/0222.html

> >+      if (!nsCSSProps::FindKeyword(keyword,
> >+                                   nsCSSProps::kFontVariantAlternatesFuncsKTable,
> >+                                   feature) ||
> >+          (feature & altFeatures)) {
> >+        UngetToken();
> >+        return false;
> >+      }
> >+
> >+      altFeatures |= feature;
> >+      uint16_t maxElems = 1;
> >+      if (keyword == eCSSKeyword_styleset ||
> >+          keyword == eCSSKeyword_character_variant) {
> >+        maxElems = MAX_ALLOWED_FEATURES;
> >+      }
> >+      nsCSSValue funcValue;
> >+      if (!ParseFunction(keyword, nullptr, VARIANT_IDENTIFIER, 1,
> >+                         maxElems, funcValue) ||
> >+          funcValue.GetUnit() != eCSSUnit_Function) {
> >+        UngetToken();
> >+        return false;
> >+      }
> 
> It seems like you should reuse ParseSingleAlternate instead of copying
> most of its contents.

I thought about this a bit when I wrote the original patch and decided that trying to share code here would lead to odd coding within ParseFontVariant and not really save a lot in terms of code sharing. I've pulled out the max elements calculation into a helper method but I think it's more natural not to try and merge the code used in the functional value handling block with the code in ParseSingleAlternate.

When parsing the value of 'font-variant-alternates', the functionality of ParseSingleAlternate is:

1. get a token
2. is the token ident or function?
3. lookup keyword
4. find keyword in values table, depending upon ident/function
5. if ident, set value
6. else parse function

For the functional value block within ParseFontVariant, steps 1-3 have already been handled when it gets to that section and step 5 is unnecessary because non-functional values need to be looked up using a different table.

Comment 17

[John Daggett (:jtd)]

Attachment: patch, revise font-variant computed value

Set up a separate patch for the font-variant computed value changes. Not sure if this is what you want or not.

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #15)

> In nsComputedDOMStyle, you should keep an implementation for font-variant
> and return something non-empty for the cases where you can -- or at a minimum
> for the CSS 2.1 cases.

The attached patch does that, it returns a value when font-variant-caps is normal or small-caps but otherwise returns nothing. Should it return nothing when any of the other font-variant-* subproperties have non-normal values?

> property_database.js:
>   I think you should probably use CSS_TYPE_SHORTHAND_AND_LONGHAND
>   instead of CSS_TYPE_TRUE_SHORTHAND, since this property was previously a
>   longhand.
> 
>   However, if that causes a bunch of failures after the nsComputedDOMStyle
>   fixes above, I"m likely to reconsider.

Yeah, setting CSS_TYPE_SHORTHAND_AND_LONGHAND causes the computed value mochitests to break, since setting values like font-variant: petite-caps returns a blank value.

We're definitely in uncharted waters I think here. This probably needs to be in some OM spec somewhere.

Comment 18

[John Daggett (:jtd)]

Specifically, test_value_cloning, test_value_computation, test_value_storage all have test failures with font-variant set to CSS_TYPE_SHORTHAND_AND_LONGHAND.

Comment 19

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

Comment on attachment 8474371 [details] [diff] [review]
patch v2c, parse font-variant as a shorthand for font feature subproperties

I suppose v2c is a revision of v2b, although I don't follow the number
vs. letter distinction here.  It sure looks like it.

r=dbaron (at least conditionally on the next patch being good as well)

Comment 20

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

Comment on attachment 8474372 [details] [diff] [review]
patch, revise font-variant computed value

You also need to return null if *any* of the other subproperties have a non-normal value.  So if you check all the other subproperties and return null at the start if any are non-normal, r=dbaron.

It might also reduce codesize a drop more to do:

nsCSSKeyword keyword;
switch (StyleFont()->mFont.variantCaps) {
  case 0:
    keyword = eCSSKeyword_normal;
    break;
  case NS_FONT_VARIANT_CAPS_SMALLCAPS:
    keyword = eCSSKeyword_small_caps;
    break;
  default:
    return nullptr;
}

and then construct val from keyword.

(Also, you wouldn't actually call GetStyleFont() like that since you'd have it, or its mFont, in a variable already, from checking the other subproperties.)

Comment 21

[John Daggett (:jtd)]

Pushed to inbound, including changes based on review comments
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3c9eea58fa2
https://hg.mozilla.org/integration/mozilla-inbound/rev/4544c35982b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/90c95c106a9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/900a8b788309
https://hg.mozilla.org/integration/mozilla-inbound/rev/8251a9b0c9ef

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c3c9eea58fa2
https://hg.mozilla.org/mozilla-central/rev/4544c35982b4
https://hg.mozilla.org/mozilla-central/rev/90c95c106a9c
https://hg.mozilla.org/mozilla-central/rev/900a8b788309
https://hg.mozilla.org/mozilla-central/rev/8251a9b0c9ef

Comment 23

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8474371 [details] [diff] [review]
patch v2c, parse font-variant as a shorthand for font feature subproperties

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

::: layout/style/nsCSSParser.cpp
@@ +11809,5 @@
>      // Provide default font-variant
> +    values[1].SetNormalValue();
> +  } else {
> +    if (values[1].GetUnit() == eCSSUnit_Enumerated &&
> +        !values[1].GetIntValue() == NS_FONT_VARIANT_CAPS_SMALLCAPS) {

Was applying the negation to "values[1].GetIntValue()" intended? In any case, clang complains with:

 6:16.43 In file included from /home/keeler/mozilla-central/obj-x86_64-unknown-linux-gnu/layout/style/Unified_cpp_layout_style1.cpp:41:
 6:16.44 /home/keeler/mozilla-central/layout/style/nsCSSParser.cpp:11805:9: error: logical not is only applied to the left hand side of this comparison [-Werror,-Wlogical-not-parentheses]
 6:16.44         !values[1].GetIntValue() == NS_FONT_VARIANT_CAPS_SMALLCAPS) {
 6:16.44         ^                        ~~
 6:16.45 /home/keeler/mozilla-central/layout/style/nsCSSParser.cpp:11805:9: note: add parentheses after the '!' to evaluate the comparison first
 6:16.45         !values[1].GetIntValue() == NS_FONT_VARIANT_CAPS_SMALLCAPS) {
 6:16.45         ^
 6:16.45          (                                                        )
 6:16.46 /home/keeler/mozilla-central/layout/style/nsCSSParser.cpp:11805:9: note: add parentheses around left hand side expression to silence this warning
 6:16.46         !values[1].GetIntValue() == NS_FONT_VARIANT_CAPS_SMALLCAPS) {
 6:16.46         ^
 6:16.46         (                       )
 6:16.46 1 error generated.

(this is with "ac_add_options --enable-warnings-as-errors")

Comment 24

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Please see comment 23.

Comment 25

[Daniel Holbert [:dholbert]]

Attachment: followup to fix comparison (and fix clang build warning/error)

This addresses the build error in comment 23.  I'm almost certain this is what was intended.

Comment 26

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

Comment on attachment 8476077 [details] [diff] [review]
followup to fix comparison (and fix clang build warning/error)

r=dbaron

Comment 27

[Daniel Holbert [:dholbert]]

Landed followup to address comment 23:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/57310a010b9e

Comment 28

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Thanks!

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/57310a010b9e

Comment 31

[John Daggett (:jtd)]

If anyone needs help with devdoc description/examples of 'font-variant' and the subproperties, please contact me.

Comment 32

[Jean-Yves Perrier [:teoli]]

Sure! I have started documenting these. I will have questions and the articles will need reviews. Hope not to be distracted by other tasks.