1449891 - Font editor: write registered axis values to corresponding CSS font properties

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement)

Description

[Razvan Caliman [:rcaliman]]

Follow-up to basic implementation in https://bugzilla.mozilla.org/show_bug.cgi?id=1449885

font-variation-settings is a low-level CSS property. It contains declarations for variable font custom axes and these registered axes: wght, wdth, slnt, ital, opsz.

Registered axis values defined in font-variation-settings will overwrite corresponding CSS font properties.

Registered axis to font property mapping:

wght -> font-weight
wdth -> font-stretch
slnt -> font-style: oblique + angle
ital -> font-style: italic
opsz -> font-optical-sizing

When editing values for registered axes in the font editor, write them to corresponding CSS font properties instead of font-variation-settings except when the registered axis is already declared in font-variation-settings on the rule or inherited from another rule. Show a warning that this is not encouraged.

Comment 1

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1449891 - Add logic to Font Editor to map axes values to font properties.

Review commit: https://reviewboard.mozilla.org/r/239278/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/239278/

Comment 2

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1449891 - Setup Font Editor with font properties from rule or inherited.

Review commit: https://reviewboard.mozilla.org/r/239280/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/239280/

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8970503 [details]
Bug 1449891 - Add logic to Font Editor to map axes values to font properties.

https://reviewboard.mozilla.org/r/239278/#review245238

::: devtools/client/inspector/fonts/fonts.js:62
(Diff revision 1)
> +    this.nodeComputedStyle = {};
>      this.pageStyle = this.inspector.pageStyle;
>      this.ruleView = this.inspector.getPanel("ruleview").view;
>      this.selectedRule = null;
>      this.store = this.inspector.store;
> +    this.textProperties = {};

I would prefer this to be a ES6 Map

::: devtools/client/inspector/fonts/fonts.js:62
(Diff revision 1)
> +    this.nodeComputedStyle = {};
>      this.pageStyle = this.inspector.pageStyle;
>      this.ruleView = this.inspector.getPanel("ruleview").view;
>      this.selectedRule = null;
>      this.store = this.inspector.store;
> +    this.textProperties = {};

We should add a comment about what these Maps/objects represent

::: devtools/client/inspector/fonts/fonts.js:63
(Diff revision 1)
>      this.pageStyle = this.inspector.pageStyle;
>      this.ruleView = this.inspector.getPanel("ruleview").view;
>      this.selectedRule = null;
>      this.store = this.inspector.store;
> +    this.textProperties = {};
> +    this.writers = {};

Add a comment about this object/Map

::: devtools/client/inspector/fonts/fonts.js:63
(Diff revision 1)
>      this.pageStyle = this.inspector.pageStyle;
>      this.ruleView = this.inspector.getPanel("ruleview").view;
>      this.selectedRule = null;
>      this.store = this.inspector.store;
> +    this.textProperties = {};
> +    this.writers = {};

This should probably be a Map as well

::: devtools/client/inspector/fonts/fonts.js:145
(Diff revision 1)
>      this.ruleView.off("ruleview-rule-unselected", this.onRuleUnselected);
>      gDevTools.off("theme-switched", this.onThemeChanged);
>  
>      this.document = null;
>      this.inspector = null;
> +    this.nodeComputedStyle = {};

This should be null.

::: devtools/client/inspector/fonts/fonts.js:150
(Diff revision 1)
> +    this.nodeComputedStyle = {};
>      this.pageStyle = null;
>      this.ruleView = null;
>      this.selectedRule = null;
>      this.store = null;
> +    this.textProperties = {};

null

::: devtools/client/inspector/fonts/fonts.js:151
(Diff revision 1)
>      this.pageStyle = null;
>      this.ruleView = null;
>      this.selectedRule = null;
>      this.store = null;
> +    this.textProperties = {};
> +    this.writers = {};

null

::: devtools/client/inspector/fonts/fonts.js:184
(Diff revision 1)
>      return this.excludeNodeFonts(allFonts, nodeFonts);
>    }
>  
>    /**
> -   * Returns true if the font inspector panel is visible, and false otherwise.
> +   * Get a reference to a TextProperty instance from the current selected rule for a
> +   * given property name. If one doesn't, create and return a new one.

If one doesn't exist,

::: devtools/client/inspector/fonts/fonts.js:240
(Diff revision 1)
> +      this.selectedRule.textProps.find(prop => prop.name === "font-variation-settings");
> +    const FVSComputedStyle = this.nodeComputedStyle["font-variation-settings"];
> +
> +    // If "font-variation-settings" CSS property is defined (on the rule or inherited)
> +    // and contains a declaration for the given registered axis, write to it.
> +    if (

This isn't really a common way we do long if statements. Would prefer to see 

if ((FVSTextProperty && FVSTextProperty.value.includes(axis)) ||
    (FVSComputedStyle && FVSComputedStyle.value.includes(axis))) {
    
}

::: devtools/client/inspector/fonts/fonts.js:249
(Diff revision 1)
> +      return writer;
> +    }
> +
> +    // We need to grab CSS from the inspected window, since calling supports() on the
> +    // one from the current global may have different platform feature support.
> +    const CSS = this.document.defaultView.CSS;

I suspect we shouldn't be doing CSS.supports here and this might need to be done at the server actor level. 

I am gonna ask pbro as a second reviewer for this.

::: devtools/client/inspector/fonts/fonts.js:281
(Diff revision 1)
> +
> +        default:
> +          condition = CSS.supports(`${property}: ${value}`);
> +      }
> +
> +      if (condition === true) {

if (condition) {} should be suffice

::: devtools/client/inspector/fonts/fonts.js:310
(Diff revision 1)
> +   *        CSS property name or axis name.
> +   * @return {Function}
> +   *         Method which updates the rule view and page style.
> +   */
> +  getWriterForProperty(name) {
> +    if (!this.writers[name]) {

We can kinda save an indent in this if statement chain if we did, it kinda makes it more readable but we have to call return twice. 

if (this.writers[name]) {
  return this.writers[name];
}

if (REGISTERED_AXES.includes(name)) {
  this.writers[name] = this.getWriterForAxis(name);
} else if (FONT_PROPERTIES.includes(name)) {
  this.writers[name] = (value) => {
    this.updateFontProperty(name, value);
  };
} else {
  this.writers[name] = this.updateFontVariationSettings;
}

return this.writers[name];

::: devtools/client/inspector/fonts/fonts.js:378
(Diff revision 1)
>     * @param {Array} values
>     *        Array of objects with axes and values defined by the variation instance.
>     */
>    onInstanceChange(name, values) {
>      this.store.dispatch(applyInstance(name, values));
> -    this.applyChanges();
> +    values.map(obj => this.getWriterForProperty(obj.axis).call(this, obj.value));

We should bind getWriterForProperty and avoid .call(this)

::: devtools/client/inspector/fonts/fonts.js:497
(Diff revision 1)
>  
>        return acc;
>      }, {});
>  
> +    // Get all computed styles for selected node; used for identifying inherited values.
> +    this.nodeComputedStyle = await this.pageStyle.getComputed(node, {});

Can remove the {}

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8970503 [details]
Bug 1449891 - Add logic to Font Editor to map axes values to font properties.

:pbro can you check the CSS.supports and nodeComputedStyle usage. I noticed getComputed() is realistically only used in computed.js, but wasn't sure if there was something better to be used here.

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8970503 [details]
Bug 1449891 - Add logic to Font Editor to map axes values to font properties.

https://reviewboard.mozilla.org/r/239278/#review245324

::: devtools/client/inspector/fonts/fonts.js:234
(Diff revision 1)
> +    const FVSTextProperty =
> +      this.selectedRule.textProps.find(prop => prop.name === "font-variation-settings");
> +    const FVSComputedStyle = this.nodeComputedStyle["font-variation-settings"];

As discussed on Slack, we don't need both calls here. Either we use the computed style once (and don't need to look at the rule), or we just loop through all CSS rules, since we have access to them locally (no server-side call needed).

I would prefer going through the rules since that doesn't require a call to the server.
We would need to find a way to access them from the rule-view model, which seems easily feasible, since we already have access to the selected rule.

Also, the getComputedStyle to the server returns all properties at once, which can be a large packet.
I'm a bit concerned for remote debugging cases.

But, I'm not completely sure either way.
We would need to measure perf to really tell. So I won't block review on this.
But at the very least, we need to choose one way, and remove the other one from this code.

::: devtools/client/inspector/fonts/fonts.js:249
(Diff revision 1)
> +      return writer;
> +    }
> +
> +    // We need to grab CSS from the inspected window, since calling supports() on the
> +    // one from the current global may have different platform feature support.
> +    const CSS = this.document.defaultView.CSS;

What gl said is correct. You are not getting the inspected window's CSS.support function here.
`this.document` is the document where the inspector is loaded, not the content page.

And you are right, calling supports() on this document may behave differently from doing the same thing in the content page (especially if you take remote debugging into account).

DevTools' front-end and the page run in completely different processes, and sometimes on different machines altogether.

As Gabriel said, we need to get this information from the server instead. That's the only place where we can know for sure if a feature is supported.
It might be hard to do this here, each time we try to write to a property.
Instead, I suggest doing a general feature detection upfront on the actor/server side and send the related information just once to the client. So that here, instead of using `CSS.supports` you can check an object that tells you what the content document supports.

One good place to do this could be in the `traits` object in the `PageStyleActor` actor.
This object is created server-side (by the actor, see the `form` function in `PageStyleActor` in `/devtools/server/actors/styles.js`) when the inspector first starts, and sent to the client.

That gives us a point where we can detect all those features and send the data to the client.

If we can't do this, because we absolutely must detect support for a given property at run-time, when interacting with the VF UI, then we will need to do a server-side round trip each time. Hopefully that doesn't slow the whole thing down.
In this case, we could add a new method on the `StyleRuleActor` class that returns a property/value pair based on data from the axis being updated.
It would do all of the `CSS.supports` heavylifting.

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8970504 [details]
Bug 1449891 - Setup Font Editor with font properties from rule or inherited.

https://reviewboard.mozilla.org/r/239280/#review246028

I am assuming I will need to look at this again because part 1 needs some changes. So, just flaggin some stuff and clearing the review.

::: devtools/client/inspector/fonts/fonts.js:155
(Diff revision 1)
>      this.textProperties = {};
>      this.writers = {};
>    }
>  
> +  /**
> +   * Collect all expected CSS font properties and their values for the selected node

s/all/all the

::: devtools/client/inspector/fonts/fonts.js:171
(Diff revision 1)
> +    }
> +
> +    // Then, replace with any enabled font properties found on the selected rule.
> +    for (let textProp of this.selectedRule.textProps) {
> +      if (FONT_PROPERTIES.includes(textProp.name) &&
> +        textProp.enabled && !textProp.overridden) {

indent this again so it will align within the ( above

::: devtools/client/inspector/fonts/reducers/font-editor.js:94
(Diff revision 1)
>            acc[tag] = value;
>            return acc;
>          }, {});
>      }
>  
> +    // If not defined on font-variation-settings, setup "wght" axis with the value of

If not defined in font-variation-settings

::: devtools/client/inspector/fonts/reducers/font-editor.js:95
(Diff revision 1)
>            return acc;
>          }, {});
>      }
>  
> +    // If not defined on font-variation-settings, setup "wght" axis with the value of
> +    // "font-weight" if that is numeric and not a keyword.

if it is numeric

::: devtools/client/inspector/fonts/reducers/font-editor.js:101
(Diff revision 1)
> +    let weight = properties["font-weight"];
> +    if (axes.wght === undefined && parseFloat(weight).toString() === weight.toString()) {
> +      axes.wght = weight;
> +    }
> +
> +    // If not defined on font-variation-settings, setup "wdth" axis with the percentage

s/on/in

::: devtools/client/inspector/fonts/reducers/font-editor.js:102
(Diff revision 1)
> +    if (axes.wght === undefined && parseFloat(weight).toString() === weight.toString()) {
> +      axes.wght = weight;
> +    }
> +
> +    // If not defined on font-variation-settings, setup "wdth" axis with the percentage
> +    // number from the value of "font-stretch" if that is not a keyword.

s/that/it

Comment 7

[Razvan Caliman [:rcaliman]]

Comment on attachment 8970503 [details]
Bug 1449891 - Add logic to Font Editor to map axes values to font properties.

https://reviewboard.mozilla.org/r/239278/#review245238

> We should bind getWriterForProperty and avoid .call(this)

`this.getWriterForProperty()` returns a function. The `.call(this, obj.value)` was just a way to call out attention to that and save a `{}` block. I'll rewrite this to be more explicit and look like the one used in `onAxisUpdate`:

```javascript
let writer;
values.map(obj => { 
  writer = this.getWriterForProperty(obj.axis);
  writer(obj.value);
});
```

Comment 8

[Razvan Caliman [:rcaliman]]

Comment on attachment 8970503 [details]
Bug 1449891 - Add logic to Font Editor to map axes values to font properties.

https://reviewboard.mozilla.org/r/239278/#review245324

> As discussed on Slack, we don't need both calls here. Either we use the computed style once (and don't need to look at the rule), or we just loop through all CSS rules, since we have access to them locally (no server-side call needed).
> 
> I would prefer going through the rules since that doesn't require a call to the server.
> We would need to find a way to access them from the rule-view model, which seems easily feasible, since we already have access to the selected rule.
> 
> Also, the getComputedStyle to the server returns all properties at once, which can be a large packet.
> I'm a bit concerned for remote debugging cases.
> 
> But, I'm not completely sure either way.
> We would need to measure perf to really tell. So I won't block review on this.
> But at the very least, we need to choose one way, and remove the other one from this code.

> Also, the getComputedStyle to the server returns all properties at once, which can be a large packet.
> I'm a bit concerned for remote debugging cases.

I tried relying on reading from the author and user-agent rules to build up the initial properties object. However, I don't get values for all font properties. For example, an unstyled `<h1>` has the following properties coming from the UA stylesheet:

```
display: block;
font-size: 2em;
font-weight: bold;
margin-block-start: .67em;
margin-block-end: .67em;
```

It's missing values for properties like `font-style`, `line-height`, `font-variation-settings`, `font-stretch`, etc. I have not found any other way of getting those default values for an element without using `getComputed()`.

In an attempt to trim the payload and address the performance issues raised, I extended the options for `getComputed()` to accept a list of properties to filter by, thereby returning and object with values only for the given properties.

> What gl said is correct. You are not getting the inspected window's CSS.support function here.
> `this.document` is the document where the inspector is loaded, not the content page.
> 
> And you are right, calling supports() on this document may behave differently from doing the same thing in the content page (especially if you take remote debugging into account).
> 
> DevTools' front-end and the page run in completely different processes, and sometimes on different machines altogether.
> 
> As Gabriel said, we need to get this information from the server instead. That's the only place where we can know for sure if a feature is supported.
> It might be hard to do this here, each time we try to write to a property.
> Instead, I suggest doing a general feature detection upfront on the actor/server side and send the related information just once to the client. So that here, instead of using `CSS.supports` you can check an object that tells you what the content document supports.
> 
> One good place to do this could be in the `traits` object in the `PageStyleActor` actor.
> This object is created server-side (by the actor, see the `form` function in `PageStyleActor` in `/devtools/server/actors/styles.js`) when the inspector first starts, and sent to the client.
> 
> That gives us a point where we can detect all those features and send the data to the client.
> 
> If we can't do this, because we absolutely must detect support for a given property at run-time, when interacting with the VF UI, then we will need to do a server-side round trip each time. Hopefully that doesn't slow the whole thing down.
> In this case, we could add a new method on the `StyleRuleActor` class that returns a property/value pair based on data from the axis being updated.
> It would do all of the `CSS.supports` heavylifting.

> One good place to do this could be in the traits object in the PageStyleActor actor.

Good catch! I added additional traits to the `PageStyleActor` to check if the font properties accept values conforming to CSS Fonts Level 4 Module.

In this context, a one-off validation is sufficient.

For on-demand validation, I discovered that the `CssProperties` front has methods to query the page for feature support (`CssProperties.isValidOnClient()`)

Comment 9

[Razvan Caliman [:rcaliman]]

Comment on attachment 8970503 [details]
Bug 1449891 - Add logic to Font Editor to map axes values to font properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239278/diff/1-2/

Comment 10

[Razvan Caliman [:rcaliman]]

Comment on attachment 8970504 [details]
Bug 1449891 - Setup Font Editor with font properties from rule or inherited.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239280/diff/1-2/

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8970503 [details]
Bug 1449891 - Add logic to Font Editor to map axes values to font properties.

https://reviewboard.mozilla.org/r/239278/#review246896

I primarily reviewed the getComputed stuff (and its backward compat implications), as well as usage of traits (which seem fine to me).
I'll let Gabriel review the rest.

::: devtools/client/inspector/fonts/fonts.js:62
(Diff revision 2)
> +    // Map CSS property names to corresponding TextProperty instances from the Rule view.
> +    this.textProperties = new Map();
> +    // Map CSS property names and variable font axis names to methods that write their
> +    // corresponding values to the appropriate TextProperty from the Rule view.
> +    // Values of variable font registered axes may be written to CSS font properties under
> +    // certain cascade circumstances and platform support. @see `getWriterForAxis(axis)`
> +    this.writers = new Map();

We don't ever need to loop over these 2 maps, so in theory they could be `WeakMap` instances instead.

::: devtools/client/inspector/fonts/fonts.js:264
(Diff revision 2)
> +   * @return {Function}
> +   *         Method to call which updates the corresponding CSS font property.
> +   */
> +  getWriterForAxis(axis) {
> +    // Default to writing all axes to font variation settings.
> +    let writer = this.updateFontVariationSettings;

You don't seem to be using this variable anywhere else than in the early `return` statement below, so maybe no need to initialize it at all, just return the reference to the function directly.

::: devtools/client/inspector/fonts/fonts.js:514
(Diff revision 2)
>  
>      const node = this.inspector.selection.nodeFront;
>      const fonts = await this.getFontsForNode(node, options);
> +    // Get all computed styles for selected node; used for identifying inherited values.
> +    this.nodeComputedStyle = await this.pageStyle.getComputed(node, {
> +      filterProperties: FONT_PROPERTIES

I tested locally against a PageStyleActor that did not support this method signature, and it seems to completely ignore it, which is great.
That means when we're connected to older servers, it won't fail, it will just end up returning the whole list of properties (which we don't care about but that's ok).
And when connected to newer servers, it will also work, and return only the properties we want.

::: devtools/server/actors/styles.js:113
(Diff revision 2)
> +    // We need to grab CSS from the inspected window, since calling supports() on the
> +    // one from the current global may have different platform feature support.

I think some rephrasing is needed here:

We need to use CSS from the inspected window in order to use CSS.supports() and detect the right platform features from there.

Comment 12

[Razvan Caliman [:rcaliman]]

Comment on attachment 8970503 [details]
Bug 1449891 - Add logic to Font Editor to map axes values to font properties.

https://reviewboard.mozilla.org/r/239278/#review246896

> We don't ever need to loop over these 2 maps, so in theory they could be `WeakMap` instances instead.

Can't use strings with WeakMap. From MDN:
> Keys of WeakMaps are of the type Object only. Primitive data types as keys are not allowed.

Comment 13

[Razvan Caliman [:rcaliman]]

Comment on attachment 8970503 [details]
Bug 1449891 - Add logic to Font Editor to map axes values to font properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239278/diff/2-3/

Comment 14

[Razvan Caliman [:rcaliman]]

Comment on attachment 8970504 [details]
Bug 1449891 - Setup Font Editor with font properties from rule or inherited.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239280/diff/2-3/

Comment 15

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8970503 [details]
Bug 1449891 - Add logic to Font Editor to map axes values to font properties.

https://reviewboard.mozilla.org/r/239278/#review247874

::: devtools/client/inspector/fonts/fonts.js:155
(Diff revisions 1 - 3)
>      this.nodeComputedStyle = {};
>      this.pageStyle = null;
>      this.ruleView = null;
>      this.selectedRule = null;
>      this.store = null;
> -    this.textProperties = {};
> +    this.textProperties = null;

We should probably clear these Maps as well.

::: devtools/client/inspector/fonts/fonts.js:177
(Diff revisions 1 - 3)
> +
> +    // Then, replace with enabled font properties found on any of the rules that apply.
> +    for (let rule of this.ruleView.rules) {
> +      for (let textProp of rule.textProps) {
> +        if (FONT_PROPERTIES.includes(textProp.name) && textProp.enabled &&
> +           !textProp.overridden) {

Can use a space here so ! is under the F in FONT_PROPERTIES and within in the braces

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8970504 [details]
Bug 1449891 - Setup Font Editor with font properties from rule or inherited.

https://reviewboard.mozilla.org/r/239280/#review247882

Comment 17

[Razvan Caliman [:rcaliman]]

Comment on attachment 8970503 [details]
Bug 1449891 - Add logic to Font Editor to map axes values to font properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239278/diff/3-4/

Comment 18

[Razvan Caliman [:rcaliman]]

Comment on attachment 8970504 [details]
Bug 1449891 - Setup Font Editor with font properties from rule or inherited.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239280/diff/3-4/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41fc8534882e
Add logic to Font Editor to map axes values to font properties. r=gl,pbro
https://hg.mozilla.org/integration/autoland/rev/8824e436d3bd
Setup Font Editor with font properties from rule or inherited. r=gl

Comment 20

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/41fc8534882e
https://hg.mozilla.org/mozilla-central/rev/8824e436d3bd