Font inspector should display fonts using their font weight and style

RESOLVED FIXED in Firefox 37

Status

DevTools
Inspector
RESOLVED FIXED
4 years ago
11 days ago

People

(Reporter: harth, Assigned: harth)

Tracking

(Depends on: 1 bug)

unspecified
Firefox 37
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8529390 [details] [diff] [review]
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.
Assignee: nobody → fayearthur
Attachment #8529390 - Flags: review?(bgrinstead)
Depends on: 886041
Depends on: 1105808
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?
Attachment #8529390 - Flags: review?(bgrinstead)
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Depends on: 1111031
(Assignee)

Comment 3

4 years ago
Created attachment 8537506 [details] [diff] [review]
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.
Attachment #8529390 - Attachment is obsolete: true
Attachment #8537506 - Flags: review?(bgrinstead)
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.
(Assignee)

Comment 5

4 years ago
(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 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".
Attachment #8537506 - Flags: review?(bgrinstead)
(Assignee)

Comment 7

4 years ago
Created attachment 8538162 [details] [diff] [review]
Change to "normal" and add a test

Good catch! Changed to "normal", and added a test for it.
Attachment #8537506 - Attachment is obsolete: true
Attachment #8538162 - Flags: review?(bgrinstead)
Attachment #8538162 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/integration/fx-team/rev/9a5c3a50b8ed
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9a5c3a50b8ed
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37

Updated

11 days ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.