1105572 - Font inspector should display fonts using their font weight and style

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Heather Arthur [:harth]]

A follow up from bug 886041.

If there are multiple weights of the same font on the page, there will will be multiple listings of it in the font inspector, but the actual display of these will be the same (all with font weight 400, no style).

If a listing has a font-weight, its display should use that font weight.

Comment 1

[Heather Arthur [:harth]]

Attachment: Apply font weight and style to font previews

This patch will extract the font's weight and style from the 'font-weight' and 'font-style' properties of its @font-face rule, or will guess the weight from examining the font's name. Non-remote system fonts often will have "Bold", etc. in their name.

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8529390 [details] [diff] [review]
Apply font weight and style to font previews

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

Nice, this is definitely giving better previews on pages that use built in system fonts with bold styling as an example.  Just a couple of notes below

::: browser/devtools/fontinspector/test/browser_fontinspector.js
@@ +38,5 @@
>  });
>  
>  function* testBodyFonts(inspector) {
>    let s = viewDoc.querySelectorAll("#all-fonts > section");
> +  is(s.length, 3, "Found 3 fonts");

I believe we will need an additional test case to cover the non @font-face condition - so a bold system font like Arial Bold.

I'm not sure if we have a good way to test the canvas data, but we could hit getFontPreviewData directly with the expected values and compare the data URL returned from that with the one showing up here in the font inspector.

And/or we could check the data URL for Arial Bold and make sure it is different from Arial.

@@ +59,5 @@
> +     "Ostrich Sans Black", "font 1: Right font name");
> +  ok(s[1].classList.contains("is-remote"),
> +     "font 1: is remote");
> +  is(s[1].querySelector(".font-url").value,
> +     "http://mochi.test:8888/browser/browser/devtools/fontinspector/test/ostrich-black.woff",

This file needs a rebase and this font name needs to be updated to the .ttf file after bug 1105808

::: toolkit/devtools/server/actors/styles.js
@@ +40,5 @@
>  const FONT_PREVIEW_TEXT = "Abc";
>  const FONT_PREVIEW_FONT_SIZE = 40;
>  const FONT_PREVIEW_FILLSTYLE = "black";
>  
> +// fudgy list of common font weight names

Maybe also link to this: http://www.w3.org/TR/CSS2/fonts.html#propdef-font-weight or some other resource used to generate the list

@@ +277,5 @@
> +      // Get the weight and style of this font for the preview and sort order
> +      let weight = 400, style = "";
> +      if (font.rule) {
> +        weight = font.rule.style.getPropertyValue("font-weight") || 400;
> +        if (weight == "bold") {

Could you just do something like this instead of special casing bold/regular here (I would think that this could be something like light/extrabold/etc):

if (FONT_WEIGHTS.hasOwnProperty(weight)) {
  weight = FONT_WEIGHTS[weight];
}

Even if the other variations aren't possible or likely, switching to something like that would get rid of some magic numbers here.

@@ +292,5 @@
> +            weight = FONT_WEIGHTS[weightName];
> +            break;
> +          }
> +        }
> +        if (name.contains("italic")) {

Should this also check for " italic" and " oblique" in the same way that there is a space in front of the weight names?

Comment 3

[Heather Arthur [:harth]]

Attachment: Just use font weight and style of fonts from @font-face rules

For right now, let's just show the correct font style for fonts that come from @font-face rules, where we know the font weight and style for sure.

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8537506 [details] [diff] [review]
Just use font weight and style of fonts from @font-face rules

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

::: toolkit/devtools/server/actors/styles.js
@@ +282,5 @@
> +      let weight = REGULAR_FONT_WEIGHT, style = "";
> +      if (font.rule) {
> +        weight = font.rule.style.getPropertyValue("font-weight")
> +                 || REGULAR_FONT_WEIGHT;
> +        if (weight == "bold") {

Do we need to replace these with numeric weights?  Seems like you can pass "bold" into the font property for canvas and it will work.  So I assume anything returned by this getPropertyValue call will work as well.

Comment 5

[Heather Arthur [:harth]]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> Do we need to replace these with numeric weights?  Seems like you can pass
> "bold" into the font property for canvas and it will work.  So I assume
> anything returned by this getPropertyValue call will work as well.

We do, for sorting purposes.

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8537506 [details] [diff] [review]
Just use font weight and style of fonts from @font-face rules

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

Could you add cases for normal and bold weights in the test page to make sure the sorting works as expected?

::: toolkit/devtools/server/actors/styles.js
@@ +281,5 @@
> +      // Get the weight and style of this font for the preview and sort order
> +      let weight = REGULAR_FONT_WEIGHT, style = "";
> +      if (font.rule) {
> +        weight = font.rule.style.getPropertyValue("font-weight")
> +                 || REGULAR_FONT_WEIGHT;

Nit: I think it'd be a bit clearer to remove the `|| REGULAR_FONT_WEIGHT` on this line and instead check `!weight || weight == "normal"` in the conditional where you are checking for a regular weight.

@@ +284,5 @@
> +        weight = font.rule.style.getPropertyValue("font-weight")
> +                 || REGULAR_FONT_WEIGHT;
> +        if (weight == "bold") {
> +          weight = BOLD_FONT_WEIGHT;
> +        } else if (weight == "regular") {

I think the word we want here is "normal", not "regular".

Comment 7

[Heather Arthur [:harth]]

Attachment: Change to "normal" and add a test

Good catch! Changed to "normal", and added a test for it.

Comment 8

[Heather Arthur [:harth]]

https://tbpl.mozilla.org/?tree=Try&rev=9fb4a560a886

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/9a5c3a50b8ed

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9a5c3a50b8ed