994559 - Style used and unused fonts in rule view differently

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, enhancement, P2)

Description

[Patrick Brosset <:pbro>]

Consider this example CSS rule:

  body, td, th, input, select, option, optgroup, .text_input {
    font-family: "Open Sans","Helvetica Neue",Arial,Helvetica,sans-serif;
  }

Only one of these fonts is going to be applied on the page, depending on what is available on the system.

It would be nice if the devtools' Rules view would grey out all the fonts that are not actually applied, so as to let the user know which one got applied in the end.

Comment 1

[Heather Arthur [:harth]]

See also bug 702581

Comment 2

[Patrick Brosset <:pbro>]

Thanks Heather for linking to that bug. Indeed, the font-panel covers that need somehow.
But I still think it could be useful to get the information about which of the font has been applied directly in the rule-view panel.
This also seems like an easy enough bug using inDOMUtils:GetUsedFontFaces(range).

Comment 3

[Heather Arthur [:harth]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #2)
> Thanks Heather for linking to that bug. Indeed, the font-panel covers that
> need somehow.
> But I still think it could be useful to get the information about which of
> the font has been applied directly in the rule-view panel.
> This also seems like an easy enough bug using
> inDOMUtils:GetUsedFontFaces(range).

We abandoned it because it's complicated to represent it correctly with this UI:

```
<div id="parent">
  <div id="child">Some Text</div>
</div>

#parent {
  font-family: Helvetica, sans-serif;
}

#child {
  font-family: Georgia, serif;
}
```

If you were inspecting #parent, I guess you'd grey out all the fonts. Another case:


```
<div id="parent">
  Text
  <div id="child">
    A bunch more text that takes up most of the space.
  </div>
</div>
```

Inspecting #parent, you'd see Helvetica highlighted, even though most of the text you see is Georgia, so you might be mislead.

Firebug did it anyways and addressed a bunch of edge cases probably: https://code.google.com/p/fbug/issues/detail?id=2495

Comment 4

[Heather Arthur [:harth]]

Here's another edge case for future reference:

https://developer.mozilla.org/en-US/docs/Web/CSS/unicode-range#Examples

Comment 5

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Assigning P1 priority for some devedition-40 bugs. 

Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'

Comment 6

[Patrick Brosset <:pbro>]

Filter on CLIMBING SHOES

Comment 7

[Florian Rivoal]

> Consider this example CSS rule:
>
>  body, td, th, input, select, option, optgroup, .text_input {
>    font-family: "Open Sans","Helvetica Neue",Arial,Helvetica,sans-serif;
>  }
>
>Only one of these fonts is going to be applied on the page, depending on what is available on the system.

This is not actually true, not even at the element level, and even if you ignore descendants. If the unicode coverage of these fonts differ, several of them may be in use on the same element. Check this for instance:

http://jsbin.com/cagara/edit?html,css,output

The Font View panel deals with that decently, while this kind of view would not.

Comment 8

[Sebastian Zartner [:sebo]]

(In reply to Florian Rivoal from comment #7)
> > Consider this example CSS rule:
> >
> >  body, td, th, input, select, option, optgroup, .text_input {
> >    font-family: "Open Sans","Helvetica Neue",Arial,Helvetica,sans-serif;
> >  }
> >
> >Only one of these fonts is going to be applied on the page, depending on what is available on the system.
> 
> This is not actually true, not even at the element level, and even if you
> ignore descendants. If the unicode coverage of these fonts differ, several
> of them may be in use on the same element. Check this for instance:
> 
> http://jsbin.com/cagara/edit?html,css,output
> 
> The Font View panel deals with that decently, while this kind of view would
> not.

Thank you for pointing this out, Florian! This case could be easily handled by keeping all used fonts saturated.

Once the Font Inspector got reworked (it's disabled by default starting from Firefox 47, see bug 1247723), there may be an option added to the fonts inside the rule view allowing you to switch to it to get more information.

Sebastian

Comment 10

[Patrick Brosset <:pbro>]

I was revisiting this bug just now, and I think we should do this, even with the edge cases described so far.

I think the plan would be to use what the font-panel uses in order to highlight the font/s that was/were used on the element, in the rule-view and computed-view.

const range = doc.createRange();
range.selectNodeContents(selectedNode);
const fonts = DOMUtils.getUsedFontFaces(range);

This gives you the list of actually used fonts on the element (there may be more than one if a @font-face was defined with a unicode-range for instance, or if the browser fell back to another font as described in comment 7).

Then, using this information, we highlight what can be highlighted in the rule-view and computed-view. Even with the cases described in comment 3, I think this would still be useful.
In addition, we could also add a link from these views to the font-panel.

The getUsedFontFaces API is really awesome btw, we could even use it to detect which font was used character by character and show exactly what font is in effect where in any given node.

Comment 11

[Michael Hoffmann]

Hi, I wonder how I can map the specific fonts on my system returned by `getUsedFontFaces` to the font names specified in the CSS rules?

Comment 12

[Sebastian Zartner [:sebo]]

The CSSFontName properties of the fonts returned by getUsedFontFaces() match the names of the used fonts within the computed values of the 'font' and 'font-family' properties. Those are the names that need to be highlighted. So, you need to split the computed values at the commas to get the font family names, then loop over them and replace each one matching the CSSFontName of one of the fonts returned by getUsedFontFaces() with a highlighted element.

For reference, here is how Firebug did it:

https://github.com/firebug/firebug/blob/313e5bece064c0fb0ab7db123a38ba3b90dfc922/extension/content/firebug/css/stylePanel.js#L843-L933

I'll keep the needinfo flag for Patrick, as he can surely provide more detailed information. Though feel free to removed the flag if the info above is already sufficient for you to solve it.

Two additional notes:
As Florian pointed out in comment 7, there are cases in which several fonts need to be highlighted.
The used fonts may actually change once web fonts are loaded. See https://github.com/firebug/firebug/issues/5551.

Sebastian

Comment 13

[Michael Hoffmann]

Thanks Sebastian!

This should be enough to get started.

Comment 14

[Patrick Brosset <:pbro>]

Great!
So, the nature of the rule-view makes this a best effort kind of thing. Indeed, the rule-view displays authored CSS (the code that the author wrote), but the browser might have to fall back to an entirely different font (or fonts). So there will be cases where we just can't highlight anything in the rule-view.
I think that's fine. The fonts panel is way more adapted for displaying the actual used font, and that's its job. So I would suggest this:

- Instead of greying out unused font families, I would instead underline (actual style to be discussed) used font families. Otherwise there may be cases where everything is greyed out, which could be confusing.
- Improve the font-family preview tooltip (the one that appears when you hover over a font-family value) so instead of only showing a rendering of the font, it also gives the name of the used font-family/ies.
- Provide a link to the fonts panel. Sort of like a mini icon button that appears next to a font-family property and, once clicked, simply switches to the fonts panel.

The first item is the only one really required in this bug.
The 2 other items should be done as separate bugs I guess, I'm just listing them here to expose my vision for this. Feel free to discuss.

As Sebastian said, you'll need to split font-families by commas and loop through the values.
There's a place in the code where we do this sort of things for other types of values (that's where we parse colors in order to show color swatches, and other things):
/devtools/client/shared/output-parser.js

Comment 15

[Sebastian Zartner [:sebo]]

(In reply to Patrick Brosset <:pbro> from comment #14)
> - Instead of greying out unused font families, I would instead underline
> (actual style to be discussed) used font families. Otherwise there may be
> cases where everything is greyed out, which could be confusing.

Good point!
Another solution could be to show an info icon in that case explaining this in a tooltip similiar to how it's outlined in bug 1306054.

> - Improve the font-family preview tooltip (the one that appears when you
> hover over a font-family value) so instead of only showing a rendering of
> the font, it also gives the name of the used font-family/ies.

I'm all for it. The tooltip also needs to always show the hovered font and not only the first used font as it does now, which can be confusing. This is obviously covered in bug 1305986.

> - Provide a link to the fonts panel. Sort of like a mini icon button that
> appears next to a font-family property and, once clicked, simply switches to
> the fonts panel.

+1

> The first item is the only one really required in this bug.
> The 2 other items should be done as separate bugs I guess, I'm just listing
> them here to expose my vision for this. Feel free to discuss.

Yes, let's focus on the first point here. This will build the foundation for bug 1305986 and linking to the Fonts panel. Thinking about that, maybe splitting the font families should be done in a separate bug blocking this one and the others?

Sebastian

Comment 16

[Michael Hoffmann]

Attachment: bug994559.patch

This took a lot longer than expected.

I'm attaching a starting patch for feedback, just to see if I've placed the logic in the right places.  It actually does not work when I change the font-family in the ruleview, as `getUsedFontFaces` is called after the style has been applied.  But before I continue with that and the rest, I just wanted post this here for some initial feedback.

Thanks!

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8886852 [details] [diff] [review]
bug994559.patch

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

This looks like a good start.

I'm wondering about the rule-view performance though.
If I understand correctly, you're only calling the method to get the list of fonts once per element selected in the inspector, which is good, it's what we want. For one given element, there's going to be a list of fonts that have been used by the browser, but there's only one list, not one list per CSS rule.
Did I read the code right?

Looks like you're also doing it when a selector is changed, which is needed too.
What about when a font-family value is modified, are you calling the method again?

In terms of performance, we'll be doing one more call every time a new element is selected. We should make sure this is not delaying showing the content of the rule-view when we start devtools or when an element is selected.
It could actually be something we do lazily, after the rule-view appeared. This could totally be a second, non-blocking, request (perhaps using requestIdleCallback to kick it off). When the result is known, we then decorate the rule-view with the underline.

Comment 18

[Sebastian Zartner [:sebo]]

(In reply to Patrick Brosset <:pbro> from comment #17)
> Comment on attachment 8886852 [details] [diff] [review]
> bug994559.patch
> 
> Review of attachment 8886852 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks like a good start.
> 
> I'm wondering about the rule-view performance though.
> If I understand correctly, you're only calling the method to get the list of
> fonts once per element selected in the inspector, which is good, it's what
> we want. For one given element, there's going to be a list of fonts that
> have been used by the browser, but there's only one list, not one list per
> CSS rule.
> Did I read the code right?
> 
> Looks like you're also doing it when a selector is changed, which is needed
> too.
> What about when a font-family value is modified, are you calling the method
> again?

Note that there are also other actions that possibly cause the used font-families to change like text content updates, webfonts being loaded, etc.

> In terms of performance, we'll be doing one more call every time a new
> element is selected. We should make sure this is not delaying showing the
> content of the rule-view when we start devtools or when an element is
> selected.

> It could actually be something we do lazily, after the rule-view appeared.
> This could totally be a second, non-blocking, request (perhaps using
> requestIdleCallback to kick it off). When the result is known, we then
> decorate the rule-view with the underline.

Yes, the decoration should definitely be added asynchronously to avoid blocking the rule view display. That also makes it easy to update only the decoration later on instead of having to refresh the whole rule view.

Sebastian

Comment 19

[Michael Hoffmann]

(In reply to Patrick Brosset <:pbro> (away) from comment #17)
> If I understand correctly, you're only calling the method to get the list of
> fonts once per element selected in the inspector, which is good, it's what
> we want. For one given element, there's going to be a list of fonts that
> have been used by the browser, but there's only one list, not one list per
> CSS rule.
> Did I read the code right?

Correct, there's only one list per element.  Unless I am mistaken, I think calling `getUsedFontFaces` for each rule will just give the same list back for each call, since we are asking about the same element each time. 

> Looks like you're also doing it when a selector is changed, which is needed
> too.
> What about when a font-family value is modified, are you calling the method
> again?

Yes I am, but it is actually called after the text properties are updated so the changes aren't visible.  But doing it lazily as you mention below should fix that.

> In terms of performance, we'll be doing one more call every time a new
> element is selected. We should make sure this is not delaying showing the
> content of the rule-view when we start devtools or when an element is
> selected.
> It could actually be something we do lazily, after the rule-view appeared.
> This could totally be a second, non-blocking, request (perhaps using
> requestIdleCallback to kick it off). When the result is known, we then
> decorate the rule-view with the underline.

Sounds nice!  I wanted to try out this, but found `requestIdleCallback` nor `window` is defined in element-style.js.  How can I get access to this function?

Comment 20

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

(In reply to Michael Brennan from comment #19)
> (In reply to Patrick Brosset <:pbro> (away) from comment #17)
> > If I understand correctly, you're only calling the method to get the list of
> > fonts once per element selected in the inspector, which is good, it's what
> > we want. For one given element, there's going to be a list of fonts that
> > have been used by the browser, but there's only one list, not one list per
> > CSS rule.
> > Did I read the code right?
> 
> Correct, there's only one list per element.  Unless I am mistaken, I think
> calling `getUsedFontFaces` for each rule will just give the same list back
> for each call, since we are asking about the same element each time. 
> 
> > Looks like you're also doing it when a selector is changed, which is needed
> > too.
> > What about when a font-family value is modified, are you calling the method
> > again?
> 
> Yes I am, but it is actually called after the text properties are updated so
> the changes aren't visible.  But doing it lazily as you mention below should
> fix that.
> 
> > In terms of performance, we'll be doing one more call every time a new
> > element is selected. We should make sure this is not delaying showing the
> > content of the rule-view when we start devtools or when an element is
> > selected.
> > It could actually be something we do lazily, after the rule-view appeared.
> > This could totally be a second, non-blocking, request (perhaps using
> > requestIdleCallback to kick it off). When the result is known, we then
> > decorate the rule-view with the underline.
> 
> Sounds nice!  I wanted to try out this, but found `requestIdleCallback` nor
> `window` is defined in element-style.js.  How can I get access to this
> function?

'window' can be found in this.ruleView.styleWindow

Comment 21

[Michael Hoffmann]

Thanks Gabriel!

Comment 22

[Michael Hoffmann]

Attachment: bug994559.patch

First, sorry for being so slow with this!  I've been busy with other things, but I'm going to make an effort now to finish this bug :-)

I've attached a new patch for feedback where I've simplified things a bit.  I no longer store the used fonts as state but instead request it lazily when the font-family properties are populated.  It seems to work fine for both selecting elements and also updating the properties.  I've copied the "generic"-font logic from Firebug.

Now a couple of questions:

- To get this to work with `font` also (not just `font-family`), should I implement an appropriate parser in output-parser.js for it myself, or is there some shortcut I can take in order to get the font family part from the font shorthand?
- How can I listen for events which Sebastian mentioned, like web fonts loading?  Should it be part of this bug?

Thanks!

Comment 23

[Patrick Brosset <:pbro>]

Comment on attachment 8908350 [details] [diff] [review]
bug994559.patch

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

Sorry about the delay giving you feedback on this. Here's some. Let me know if this is useful!

::: devtools/client/inspector/rules/models/element-style.js
@@ +144,5 @@
> +   * Returns a promise that will be resolved to a list of CSS family
> +   * names.  The list might have duplicates.
> +   */
> +  getUsedFonts: function () {
> +    let deferred = defer();

Whenever we can, we try to use native promises now, and try to avoid introducing more uses of defer (see bug 1283869).
Also, using native async/await functions would also help make this code simpler.

So you could rewrite this to something like this:

getUsedFonts: function () {
  return new Promise((resolve, reject) => {
    this.ruleView.styleWindow.requestIdleCallback(async () => {
      try {
        let fonts = await this.pageStyle.getUsedFontFaces(this.element, { includePreviews: false });
        resolve(fonts.map(font => font.CSSFamilyName));
      } catch (e) {
        reject(e);
      }
    });
  });
}

::: devtools/client/inspector/rules/views/text-property-editor.js
@@ +40,5 @@
>    `.${ANGLE_SWATCH_CLASS}`,
>    "a"
>  ];
>  
> +const GENERIC_FONT_FAMILIES = [

I think this const requires a comment on top. Something like:

// In order to highlight the used fonts in font-family properties, we retrieve the
// list of used fonts from the server. That always returns the actually used font
// family name(s). If the property's authored value is sans-serif for instance, the
// used font might be arial instead.
// So we need the list of all generic font family names to underline those when we find
// them.

But, see my related comment below.

@@ +45,5 @@
> +  "serif",
> +  "sans-serif",
> +  "cursive",
> +  "fantasy",
> +  "monospace"

Reading MDN, it looks like system-ui is another generic font we should add here.
https://developer.mozilla.org/en-US/docs/Web/CSS/font-family

However the spec doesn't have it:
https://drafts.csswg.org/css-fonts-3/#generic-font-families

So, I'm not sure, but I think we should add it.

@@ +390,5 @@
> +
> +        for (let span of fontFamilySpan) {
> +          let authoredFont = span.textContent.trim().toLowerCase();
> +          // Remove any surrounding quotes
> +          if (authoredFont[0] == '"' && authoredFont[authoredFont.length - 1] == '"') {

There may be single quotes too, those work in CSS.
It looks to me like the spans should only contain the trimmed and unquoted family names. The parser should be the one doing the heavy lifting here, so that here, in this file, you don't have to do this again.

@@ +398,5 @@
> +          let isGeneric = GENERIC_FONT_FAMILIES.includes(authoredFont);
> +          if (usedFontsLC.includes(authoredFont) || (isGeneric && !genericFontUsed)) {
> +            span.classList.add("used-font");
> +            if (isGeneric) {
> +              genericFontUsed = true;

Oh I see, this is to avoid underlining serif in 
font-family: sans-serif, serif;
for example.

I'm not convinced we should be underline generic names though.
If I have font-family: arial, serif; for example, then arial is underlined (expected), but serif is too, and that's a problem, because clearly, the font in the page isn't serif.

So, I think, even if sometimes we have cases like font-family:sans-serif; we'd better not underline the value at all.

Or maybe better: if we haven't underlined anything in the list of authored fonts, then we highlight the first generic font we find.
How does that sound?
This way, it will be useful to users to know that their font names were invalid (maybe font "foo" does not exist here), and that the engine did a fall back to a generic font.

So, for font-family: foo, sans-serif; at least you'd see sans-serif underlined, which is useful.
But for font-family: foo, arial, sans-serif; you'd only see arial underlined, which is correct. Right now, we also underline sans-serif here.

::: devtools/client/shared/output-parser.js
@@ +441,5 @@
>                       token.tokenType === "number" || token.tokenType === "dimension" ||
>                       token.tokenType === "percentage" || token.tokenType === "dimension");
>      }
>  
> +    if (options.expectFont) {

I don't think this is the right place for this code. Here you're parsing again some pre-parsed values, splitting on , characters, and removing spaces, etc. 
I think this might be prone to bugs unfortunately.
I advise moving your logic into the actual parsing of the value above, inside the switch (token.tokenType) statement.
We use an actual tokenizer here to walk through the CSS value, token by token. This means it's way more robust to handle things like surrounding spaces (which, we may have multiple of), or things like single or double quotes, etc.

For example in the case "ident", you get a chance to run code for each font-family name. This case is only executed when a string identifier is found. So you don't even need to worry about quotes or spaces or commas, etc.
So I would suggest adding your if (options.expectFont) inside this case, and then calling out to another function like this._appendFontFamily(token.text, options); to create a span with the right class "ruleview-font-family"

Comment 24

[Michael Hoffmann]

Thanks Patrick, your feedback is very useful indeed!

(In reply to Patrick Brosset <:pbro> from comment #23)
> @@ +390,5 @@
> > +
> > +        for (let span of fontFamilySpan) {
> > +          let authoredFont = span.textContent.trim().toLowerCase();
> > +          // Remove any surrounding quotes
> > +          if (authoredFont[0] == '"' && authoredFont[authoredFont.length - 1] == '"') {
> 
> There may be single quotes too, those work in CSS.
> It looks to me like the spans should only contain the trimmed and unquoted
> family names. The parser should be the one doing the heavy lifting here, so
> that here, in this file, you don't have to do this again.

Sounds like a good idea.  It will change the underlining of the font so that the line won't include quotation marks.  I have no idea which way is more correct, but will probably look just fine or maybe better.

> I'm not convinced we should be underline generic names though.
> If I have font-family: arial, serif; for example, then arial is underlined
> (expected), but serif is too, and that's a problem, because clearly, the
> font in the page isn't serif.
> 
> So, I think, even if sometimes we have cases like font-family:sans-serif;
> we'd better not underline the value at all.
> 
> Or maybe better: if we haven't underlined anything in the list of authored
> fonts, then we highlight the first generic font we find.
> How does that sound?
> This way, it will be useful to users to know that their font names were
> invalid (maybe font "foo" does not exist here), and that the engine did a
> fall back to a generic font.
> 
> So, for font-family: foo, sans-serif; at least you'd see sans-serif
> underlined, which is useful.
> But for font-family: foo, arial, sans-serif; you'd only see arial
> underlined, which is correct. Right now, we also underline sans-serif here.

Yes, I agree.  I'll change the implementation to behave like that instead.

> ::: devtools/client/shared/output-parser.js
> @@ +441,5 @@
> >                       token.tokenType === "number" || token.tokenType === "dimension" ||
> >                       token.tokenType === "percentage" || token.tokenType === "dimension");
> >      }
> >  
> > +    if (options.expectFont) {
> 
> I don't think this is the right place for this code. Here you're parsing
> again some pre-parsed values, splitting on , characters, and removing
> spaces, etc. 
> I think this might be prone to bugs unfortunately.
> I advise moving your logic into the actual parsing of the value above,
> inside the switch (token.tokenType) statement.
> We use an actual tokenizer here to walk through the CSS value, token by
> token. This means it's way more robust to handle things like surrounding
> spaces (which, we may have multiple of), or things like single or double
> quotes, etc.
> 
> For example in the case "ident", you get a chance to run code for each
> font-family name. This case is only executed when a string identifier is
> found. So you don't even need to worry about quotes or spaces or commas, etc.
> So I would suggest adding your if (options.expectFont) inside this case, and
> then calling out to another function like this._appendFontFamily(token.text,
> options); to create a span with the right class "ruleview-font-family"

I might be mistaken, but it seems like I do need to worry about the spaces at least.  I printed to the console what I got from parsing some example families, and if I have a value without quotes, like font-family: Nimbus Sans L
I get the following stream of tokens:

(ident, "Nimbus"), (whitespace,), (ident, "Sans"), (whitespace,), (ident, "L")

(The first element of each tuple is the token type and the second is the token text.)

If we have something with quotes, it is easier.
font-family: "Open Sans" gives me (string, "Open Sans")

I originally aimed to use the parsing logic as you advise, but after I found that I had to keep track of the white spaces and commas I figured it was easier to simply split on the commas after the parsing.

I'd be happy to try to move it up to the real parsing, but I think I need to handle some state in order to remember where in the parsing of a font-family we are.  Does this still sound like the correct approach?  Please let me know if I'm mistaken regarding the ident tokens.

Comment 25

[Tom Tromey :tromey]

(In reply to Michael Brennan from comment #24)

> I'd be happy to try to move it up to the real parsing, but I think I need to
> handle some state in order to remember where in the parsing of a font-family
> we are.  Does this still sound like the correct approach?  Please let me
> know if I'm mistaken regarding the ident tokens.

I didn't read the patch (can do if needed) but you're correct about the tokenization.

What I would do in a case like this is look up font-family in MDN:
https://developer.mozilla.org/en-US/docs/Web/CSS/font-family

Then see what Firefox implements (I didn't do this so I don't know);
then find the applicable standard.  There are some links in MDN.

Picking one I found the grammar, which lead to:

https://drafts.csswg.org/css-fonts-3/#family-name-value

... which explains the parsing rule that has to be followed.

Though, since output-parser is generally dealing with the authored sources, there's the
added difficulty of dealing with var(...) in the middle.

Comment 26

[Michael Hoffmann]

Thanks for the clarification!

Comment 27

[Michael Hoffmann]

Attachment: bug994559.patch

Hi, I've addressed your comments. Let me know what you think :)

Comment 28

[Patrick Brosset <:pbro>]

So sorry Michael for the delay. I'll look at your patch right now. Last week has been a busy one for me and I failed to let you know that I wouldn't be able to review your patch then. Sorry about that.

Comment 29

[Patrick Brosset <:pbro>]

Comment on attachment 8914115 [details] [diff] [review]
bug994559.patch

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

Wow this looks great. Love the new changes in output-parser.js.
Now we only need a test to make sure this does not regress in the future. I think the right place for this test would be in devtools/client/shared/test/browser_outputparser.js
You can create a new testParseFontFamily function in there. I think you could model it after testParseShape, by defining a list of different properties to check.

::: devtools/client/inspector/rules/views/text-property-editor.js
@@ +401,5 @@
> +
> +        for (let span of fontFamilySpans) {
> +          const authoredFont = span.textContent.toLowerCase();
> +
> +          if (firstGenericSpan === null && GENERIC_FONT_FAMILIES.includes(authoredFont)) {

!firstGenericSpan instead of firstGenericSpan === null would work too and is shorter/simpler.

@@ +414,5 @@
> +            break;
> +          }
> +        }
> +
> +        if (!foundMatchingFamily && firstGenericSpan !== null) {

And here you can replace firstGenericSpan !== null with just firstGenericSpan

@@ +417,5 @@
> +
> +        if (!foundMatchingFamily && firstGenericSpan !== null) {
> +          firstGenericSpan.classList.add("used-font");
> +        }
> +      });

We should have a promise rejection handler too. If getUsedFontFamilies fail, then we should have a .catch(e => ...) here at the end to catch that.
In that particular case, maybe it should just log an error with console.error("Could not get the list of font families", e);

Comment 30

[Michael Hoffmann]

(In reply to Patrick Brosset <:pbro> from comment #28)
> So sorry Michael for the delay. I'll look at your patch right now. Last week
> has been a busy one for me and I failed to let you know that I wouldn't be
> able to review your patch then. Sorry about that.

Oh, no worries at all! :) But thank you for the positive review!

I've made a new patch with added tests and your comments addressed.  I will set it to feedback? not review? as I don't know how much is left to do for this bug.

In addition to `font-family`, I suppose `font` should be handled too, right?
What about triggering the used fonts check on events like loaded web fonts?
Should we handle var(...) in the middle like :tromey mentioned in comment 25?

Comment 31

[Michael Hoffmann]

Attachment: bug994559.patch

Comment 32

[Patrick Brosset <:pbro>]

Comment on attachment 8918463 [details] [diff] [review]
bug994559.patch

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

This test looks quite good. Thanks for adding it.
We will also need something that tests the underline mechanism. Something that checks that the right span has the underlining class, even for edge cases with generic font-families, etc.
This test should be in a new file in the devtools/client/inspector/rules/test directory.
This would be an integration test sort of like browser_rules_colorpicker-swatch-displayed.js, something that just inserts a lot of different font-family properties and checks that the rules-view shows the right thing.

Comment 33

[Patrick Brosset <:pbro>]

(In reply to Michael Hoffmann from comment #30)
> In addition to `font-family`, I suppose `font` should be handled too, right?
Yeah, that'd be great, but I would suggest doing this as a second step (probably in a separate bug) so that we can take the time to think about how to handle non-font-family values within this short-hand property (e.g. bold, italic, large, ...).
> What about triggering the used fonts check on events like loaded web fonts?
Good point. I had forgotten about that. Can you please create an example HTML page that shows the problem? I'd like to take a look at this.
Sounds like this could be complex and will require constantly listening to a font loaded event. So, better break this out in its own bug too.
> Should we handle var(...) in the middle like :tromey mentioned in comment 25?
Yes, although I'm not sure exactly what we'll support. 
For instance, this works in CSS:

body{
  --partial-font: sans ms;
  font-family: comic var(--partial-font);
}

What would we highlight here?
I'm tempted to maybe just give up if a var is detected inside a font-family property.

Comment 34

[Michael Hoffmann]

Attachment: bug994559.test.patch

Comment 35

[Michael Hoffmann]

(In reply to Patrick Brosset <:pbro> from comment #32)
> Comment on attachment 8918463 [details] [diff] [review]
> bug994559.patch
> 
> Review of attachment 8918463 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This test looks quite good. Thanks for adding it.
> We will also need something that tests the underline mechanism. Something
> that checks that the right span has the underlining class, even for edge
> cases with generic font-families, etc.
> This test should be in a new file in the
> devtools/client/inspector/rules/test directory.
> This would be an integration test sort of like
> browser_rules_colorpicker-swatch-displayed.js, something that just inserts a
> lot of different font-family properties and checks that the rules-view shows
> the right thing.

Thanks for the help! I added a new test in a separate patch in order to make it simpler to review what changed.  But since I don't know what fonts are available on the system running the tests, I've only added tests involving generic family names.  How can I test with real family names?

Comment 36

[Michael Hoffmann]

(In reply to Patrick Brosset <:pbro> from comment #33)
> (In reply to Michael Hoffmann from comment #30)
> > In addition to `font-family`, I suppose `font` should be handled too, right?
> Yeah, that'd be great, but I would suggest doing this as a second step
> (probably in a separate bug) so that we can take the time to think about how
> to handle non-font-family values within this short-hand property (e.g. bold,
> italic, large, ...).
> > What about triggering the used fonts check on events like loaded web fonts?
> Good point. I had forgotten about that. Can you please create an example
> HTML page that shows the problem? I'd like to take a look at this.
> Sounds like this could be complex and will require constantly listening to a
> font loaded event. So, better break this out in its own bug too.
> > Should we handle var(...) in the middle like :tromey mentioned in comment 25?
> Yes, although I'm not sure exactly what we'll support. 
> For instance, this works in CSS:
> 
> body{
>   --partial-font: sans ms;
>   font-family: comic var(--partial-font);
> }
> 
> What would we highlight here?
> I'm tempted to maybe just give up if a var is detected inside a font-family
> property.

Thanks for the answers!  I will look into them, and will also try creating the HTML page you requested.  But to avoid delaying this bug for an even longer time, I'll try to focus finishing up the work done this far to have something to release soon :-)  So it sounds good postponing the extra features into follow up bugs.

Comment 37

[Patrick Brosset <:pbro>]

Comment on attachment 8927679 [details] [diff] [review]
bug994559.test.patch

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

::: devtools/client/inspector/rules/test/browser_rules_highlight-used-fonts.js
@@ +59,5 @@
> +
> +    ok(fonts.length, "Fonts found in the property");
> +    is(fonts.length, nb, "Correct number of fonts found in the property");
> +
> +    const usedFontIndices = Array.from(fonts).reduce((acc, span, idx) => {

I find using reduce a bit confusing for this. What you want here is filter down the list to only the spans that have a particular class. So you couls use the filter function instead. What about this:

const highlighted = [...fonts].filter(span => span.classList.contains("used-font"));

This is shorter and easier to read I think. It also makes your next assertion simpler:

is(highlighted.length, 1, "No more than one font highlighted");

The final assertion however wouldn't work because you want to check the index of the span (which is a good thing to do).
But you could change it to this instead:

is([...fonts].findIndex(f => f === highlighted[0]), used, "Correct font highlighted");

::: devtools/client/inspector/rules/views/text-property-editor.js
@@ +420,5 @@
>            firstGenericSpan.classList.add("used-font");
>          }
> +
> +        this.ruleView.emit("font-highlighted", this.valueSpan);
> +      }.bind(this)).catch(e => console.error("Could not get the list of font families", e));

No need for .bind(this) if you switch the function to an arrow function:

this.rule.elementStyle.getUsedFontFamilies().then(families => {
  ...
  this.ruleView.emit("font-highlighted", this.valueSpan);
}).catch(e => console.error("Could not get the list of font families", e));

Comment 38

[Michael Hoffmann]

(In reply to Patrick Brosset <:pbro> from comment #37)
> I find using reduce a bit confusing for this. What you want here is filter
> down the list to only the spans that have a particular class. So you couls
> use the filter function instead. 

I agree, using filter and then findIndex makes things clearer.

> No need for .bind(this) if you switch the function to an arrow function:
> 
> this.rule.elementStyle.getUsedFontFamilies().then(families => {
>   ...
>   this.ruleView.emit("font-highlighted", this.valueSpan);
> }).catch(e => console.error("Could not get the list of font families", e));

Neat!

I made the changes, and then I squashed the commits into one.  I will upload it now for review.

Comment 39

[Michael Hoffmann]

Attachment: bug994559.patch

Comment 40

[Patrick Brosset <:pbro>]

Comment on attachment 8930641 [details] [diff] [review]
bug994559.patch

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

I've tested this quite a bit and couldn't find a problem with it.
The code changes look fine to me too. One last minor comment below.

::: devtools/client/shared/test/browser_outputparser.js
@@ +82,5 @@
>         ")"]),
>  
>      // In "arial black", "black" is a font, not a color.
> +    // (The font-family parser creates a span)
> +    makeColorTest("font-family", "arial black", ["<span>arial black</span>"]),

The name of makeColorTest assumes this is only to test color-related properties. It's obviously not the case anymore. Maybe now is the time to fix this and rename the function to something more generic. Like makePropertyParsingTest or something.

Comment 41

[Michael Hoffmann]

To me it looks like `makeColorTest` is still only used for testing color-related properties.  I added the span-tags since the parser now adds them when it finds a font, but I think the intention of that particular test is to just check that it doesn't misinterpret "black" to be something that needs a color swatch.  The tests that check the font-parsing is in another function with tests that work a bit different from `makeColorTest`.

I'd be happy to rename and update `makeColorTest`, but I'm having trouble understanding in what way it should be changed.

Comment 42

[Patrick Brosset <:pbro>]

You are right. I misread this. Looking good.

Comment 43

[Patrick Brosset <:pbro>]

Attachment: Bug 994559 - Style used fonts in rule view.

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

Comment 44

[Patrick Brosset <:pbro>]

Comment on attachment 8930641 [details] [diff] [review]
bug994559.patch

Moved the patch to mozreview instead.

Comment 45

[Patrick Brosset <:pbro>]

Comment on attachment 8934526 [details]
Bug 994559 - Style used fonts in rule view.

https://reviewboard.mozilla.org/r/205432/#review211022

Comment 46

[Patrick Brosset <:pbro>]

And here is a try push with this commit, to see if all tests still pass: https://treeherder.mozilla.org/#/jobs?repo=try&revision=85c6fb0ef3b02e9922b29dcf465bbb39f6a44b56

Comment 47

[Michael Hoffmann]

I see that tests are failing because of the changes.  Will investigate and fix.

Comment 48

[Michael Hoffmann]

The tests fail because the patch removes spaces around the commas which separate the fonts.

So its easy to make the tests pass by modifying the expected output. For instance:

--- a/devtools/client/inspector/rules/test/browser_rules_select-and-copy-styles.js
+++ b/devtools/client/inspector/rules/test/browser_rules_select-and-copy-styles.js
@@ -61,7 +61,7 @@ function* checkCopySelection(view) {

   let expectedPattern = "    margin: 10em;[\\r\\n]+" +
                         "    font-size: 14pt;[\\r\\n]+" +
-                        "    font-family: helvetica, sans-serif;[\\r\\n]+" +
+                        "    font-family: helvetica,sans-serif;[\\r\\n]+" +
                         "    color: #AAA;[\\r\\n]+" +
                         "}[\\r\\n]+" +
                         "html {[\\r\\n]+" +


Is it this an acceptable side effect of this patch?

Comment 49

[Patrick Brosset <:pbro>]

(In reply to Michael Hoffmann from comment #48)
> The tests fail because the patch removes spaces around the commas which
> separate the fonts.
> ...
> Is it this an acceptable side effect of this patch?
What about in the UI? Is each font name separate from the previous one by some space? If yes, then feel free to add this change to your commit.

Comment 50

[Michael Hoffmann]

Attachment: bug994559.1.patch

Comment 51

[Michael Hoffmann]

Attachment: bug994559.2.patch

No, the missing spaces were also missing in the UI.

I've made some changes to the patch so that single spaces are
preserved, making the UI work like today.

I'm attaching two patches, the first one is just the same patch you've
already reviewed, just rebased to a more recent version on central.
The second one is with my changes, along with a few more tests to
check the parsed output.

Comment 52

[Michael Hoffmann]

And a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8225e41d3e1d8efce3ca6de3476356e165898775

Comment 53

[Patrick Brosset <:pbro>]

Comment on attachment 8940901 [details] [diff] [review]
bug994559.2.patch

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

Looks good. Are you going to merge this commit with the previous one? Will make it easier to read history.

Comment 54

[Michael Hoffmann]

Thanks!  Yes, I will merge them together later today I hope, before I set the checkin-needed flag.

Comment 55

[Michael Hoffmann]

Attachment: bug994559.patch

Last to patches merged into one, ready for checkin.

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c809b916352b
Style used fonts in rule view. r=pbro

Comment 57

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/c809b916352b

Comment 58

[Sebastian Zartner [:sebo]]

With this change the font preview popup is only shown when hovering the commas between the fonts. Should this be fixed within this bug or should I create a new one? In any case, this regression should get fixed before this patch reaches Release.

Sebastian

Comment 59

[Patrick Brosset <:pbro>]

Good catch Sebastian! Please file a bug and make it block this one. And make it track 59 so we get it fixed and uplifted in time.

Comment 61

[Eric A. Meyer]

Was a separate bug ever filed to get this same capability for the `font` shorthand?  I searched, but nothing came up for the terms I used.

Comment 62

[Michael Hoffmann]

(In reply to Eric A. Meyer from comment #61)
> Was a separate bug ever filed to get this same capability for the `font`
> shorthand?  I searched, but nothing came up for the terms I used.

No, sorry, I haven't done that yet.  I was planning to do that within the next few days though.

Comment 63

[Eric A. Meyer]

(In reply to Michael Hoffmann from comment #62)
> No, sorry, I haven't done that yet.  I was planning to do that within the
> next few days though.

No worries!  I’ll leave it to you, since you have a far better handle on this than I do.  I love seeing it on `font-family` and am confident that within a week I’ll be unable to remember how or why I ever lived without this feature.

Comment 64

[Michael Hoffmann]

I tried this feature with web fonts and it worked fine for me.  So I won't file a bug for it.