1466489 - Implement CSSStyleDeclaration.setProperty for CSSFontFaceRule.style

Status: NEW

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Jonathan Watt [:jwatt]]

We currently implement CSSStyleDeclaration.setProperty for CSSStyleRule.style, but not for CSSFontFaceRule.style.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Prerequisite for getting some of the WPT variable fonts tests passing.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8983013 [details] [diff] [review]
patch

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

Should we also implement SetCSSText and such? Do other browsers do that?

::: layout/style/ServoFontFaceRule.cpp
@@ +110,5 @@
> +               "LookupFontDesc returned value out of range");
> +
> +  // Priority is not valid on @font-face descriptors so we're going to ignore
> +  // it, but nevertheless we follow this step as specified in the spec.
> +  if (!aPriority.IsEmpty() && !aPriority.EqualsLiteral("important")) {

What do other browsers do? Mind adding a WPT for this?

@@ +121,2 @@
>    // FIXME(heycam): If we are changing unicode-range, then a FontFace object
>    // representing this rule must have its mUnicodeRange value invalidated.

Does this need fixing? Is it too hard to fix?

@@ +121,4 @@
>    // FIXME(heycam): If we are changing unicode-range, then a FontFace object
>    // representing this rule must have its mUnicodeRange value invalidated.
>  
> +  if (descID != eCSSFontDesc_UNKNOWN) {

May look cleaner bailing out on an unknown descriptor like:

if (descID == eCSSFontDesc_UNKNOWN) {
  return NS_OK;
}

// ...

@@ +122,5 @@
>    // representing this rule must have its mUnicodeRange value invalidated.
>  
> +  if (descID != eCSSFontDesc_UNKNOWN) {
> +    NS_ConvertUTF16toUTF8 value(aValue);
> +    RefPtr<URLExtraData> url; // nothing to provide here

I don't think this is fine, you need to get the right document URI / base URI / quirks mode.

You can probably re-use nsDOMCSSDeclaration::GetParsingEnvironmentForRule(ContainingRule()) for this.

Also, please add tests for this, since at least length parsing is affected by quirks-mode, so testing this shouldn't be too hard (you can test with an unitless font-weight descriptor).

That means that SetDescriptor should take the quirks mode into account too, which looks like a long-standing issue that also affects the font loading API.

Please add tests for all this.

@@ +127,5 @@
> +    if (!Servo_FontFaceRule_SetDescriptor(mRawRule, descID, &value, url)) {
> +      return NS_ERROR_DOM_SYNTAX_ERR;
> +    }
> +  }
> +

This doesn't update the page in any way as a result of the change, isn't that wrong?

If the descriptor changed, this at least needs to call ContainingRule()->GetStyleSheet()->RuleChanged(ContainingRule()), with the appropriate null-check on GetStyleSheet(), the same way as style declarations work.

It's not clear to me how the font loading API handles this... If we deem that we don't need to restyle in the case the @font-face rule changed (dubious...), we can ignore it in the RuleChanged notifications instead.