1472116 - Not so useful fonts reported in some cases where whitespaces are involved

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

Here are the steps I followed:
- make sure you have the new font editor pref ON
- open http://cn.nus.edu.sg/ in a new tab
- open the inspector and select the .sprocket-padding .sprocket-mosaic-title a element in the dom (that's the link below the photo in the bottom part where there are 4 media elements side by side)
- open the Fonts panel

==> The Fonts panel shows that the Arial font family is used only. Now, that doesn't really help me, because Arial happens to only be used here for rendering some non-collapsible whitespace that happens to be there inside the text node.
Arial *is* used, and *is* part of the font-family on that node, so the tool is working as expected, but it's not very helpful in this case.

Now, go and edit the text node in the inspector and trim all whitespaces.
Select another node and then select the link node again (to force a refresh of the fonts editor)

==> Now the Fonts panel shows something that makes more sense to me. On Windows, it tells me that Microsoft YaHei and Yu Gothic are both used (which is true cause there's a character in there that one of the fonts can't render so it falls back to another one).

If I remember correctly, that's because none of the fonts defined in the font-family are used at all, so we go and get fonts used from descendants (in this case, the text node) as returned by the getUsedFontFaces platform API.

So, this is a case where the font-overview panel was actually more useful here.
In many cases, the font-editor panel is way more useful, but in cases like this (which I think are common), our new logic falls a bit short.

I can see 2 solutions:
- Add a section like the "other fonts in page" section maybe named "font faces rendered" that basically does what the font-overview did. It would be collapsed by default, and the font-editor would do the same thing it does today. That is: focus on the defined font-family value, and go from there. But that new section would be helpful for these use cases.
- Or, do something smart where the font-editor checks if the font family that was found to be used was in fact only used for whitespaces. But I'm not a fan of this one because of this comment bug 1463115 comment 2 (sometimes whitespaces *is* important).

Comment 1

[Razvan Caliman [:rcaliman]]

Attachment: Screen Shot 2018-06-29 at 13.46.39.jpg

On MacOS it alternates between Helvetica before removing whitespace and two other system fonts after:
- Hiragino Kaku Gothic ProN
- PingFang SC

Comment 2

[Patrick Brosset <:pbro>]

Attachment: Bug 1472116 - Show the font overview in a collapsible section in the font editor;

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

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8988732 [details]
Bug 1472116 - Show the font overview in a collapsible section in the font editor;

This is what I have in mind for the first solution. Quite easy to do, doesn't change the font-editor UI much at all, and is a simple way to preserve the font-overview features for those who need it.
Let me know what you think, and I'll prepare a better patch with a test too if you're ok with it.

Comment 4

[Razvan Caliman [:rcaliman]]

Comment on attachment 8988732 [details]
Bug 1472116 - Show the font overview in a collapsible section in the font editor;

https://reviewboard.mozilla.org/r/253940/#review260916

This is solving atomically the issue you discovered (showing additional system fallback fonts used). This will work for now.

However, I'm concerned by the complexity introduced over time with showing the used fonts.

With this change we will have the following reasons for showing a "used font":

- declared in the font-family CSS property
- the first variable font / any font used on a child element because no TextNode exists on the selected element
- used as a system fallback (not declared in font-family)

This is very opaque to the user. No reason is given for why a "used font" shows up and sometimes the choice is confusing.

Speaking from the perspective of an average web developer who knows enough about fonts just to design a website, I'm often confused by the complexity of fonts selection. I venture to assume that many users will have similar depth with regards to fonts.

My suggestion for a better font editor is to change the "Family" section. Rename it to "Used fonts", then show the list of all rendered fonts per selected element and annotate them with labels to help explain why they were used:

For example:

  [declared in font-family]
- Arial

  [system fallback]
- Hiragino Kaku Gothic ProNSystem Font

  [used on descendant elements]
- Courier

This may yield a longer list sometimes, but it adds informative labels so a user could easily understand what they're looking at.

Again, as a web developer with average understanding of fonts in CSS, I welcome insight from more experienced others. Perhaps Victoria, Jen or Martin can offer an improved solution. But I'd like to ensure this font selection complexity is kept within the grasp of the average web developer.

In conclusion, the solution you proposed solves an immediate problem and that's good. You can proceed. 
But in the longer term I'd like for us to have a more helpful "Used fonts" section which both shows and indicates _why_ fonts were used.

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8988732 [details]
Bug 1472116 - Show the font overview in a collapsible section in the font editor;

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

Comment 6

[Razvan Caliman [:rcaliman]]

Comment on attachment 8988732 [details]
Bug 1472116 - Show the font overview in a collapsible section in the font editor;

https://reviewboard.mozilla.org/r/253940/#review260960

::: devtools/client/inspector/fonts/test/browser_fontinspector_other-fonts.js:18
(Diff revision 2)
>  add_task(async function() {
>    await pushPref("devtools.inspector.fonteditor.enabled", true);
>    const { inspector, view } = await openFontInspectorForURL(TEST_URI);
>    const viewDoc = view.document;
>  
> -  const otherFontsAccordion = viewDoc.querySelector("#font-container .accordion");
> +  const otherFontsAccordion = viewDoc.querySelectorAll("#font-container .accordion")[1];

Since you introduced `getOtherFontsAccordion()` in `head.js`, you could reuse it here to encapsulate the  query logic in the method from `head.js`

::: devtools/client/inspector/fonts/test/browser_fontinspector_other-fonts.js:22
(Diff revision 2)
>  
> -  const otherFontsAccordion = viewDoc.querySelector("#font-container .accordion");
> +  const otherFontsAccordion = viewDoc.querySelectorAll("#font-container .accordion")[1];
>    ok(otherFontsAccordion, "There's an accordion in the panel");
>    is(otherFontsAccordion.textContent, "Other fonts in page", "It has the right title");
>  
>    await expandOtherFontsAccordion(viewDoc);

if you reuse `getOtherFontsAccordion()`, this can become `await expandAccordion(otherFontsAccordion)`, then you can remove the explicit `expandOtherFontsAccordion()` from `head.js`

::: devtools/client/inspector/fonts/test/browser_fontinspector_rendered-fonts.js:17
(Diff revision 2)
> +add_task(async function() {
> +  await pushPref("devtools.inspector.fonteditor.enabled", true);
> +  const { inspector, view } = await openFontInspectorForURL(TEST_URI);
> +  const viewDoc = view.document;
> +
> +  const fontsAccordion = viewDoc.querySelectorAll("#font-container .accordion")[0];

Same here with reusing `getRenderedFontsAccordion()` from `head.js`

::: devtools/client/inspector/fonts/test/browser_fontinspector_rendered-fonts.js:21
(Diff revision 2)
> +
> +  const fontsAccordion = viewDoc.querySelectorAll("#font-container .accordion")[0];
> +  ok(fontsAccordion, "There's an accordion for rendered fonts in the panel");
> +  is(fontsAccordion.textContent, "Rendered fonts", "It has the right title");
> +
> +  await expandRenderedFontsAccordion(viewDoc);

Same as above, this can become `expandAccordion(fontsAccordion)` (perhaps renamed to `renderedFontsAccordion` to avoid confusion), then you can remove the explicit `expandRenderedFontsAccordion()` form `head.js`.

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8988732 [details]
Bug 1472116 - Show the font overview in a collapsible section in the font editor;

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

Comment 8

[Patrick Brosset <:pbro>]

Try push for this last version of the commit: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a5d307de42c041cbea855d0af4c4f57a6dbb101&group_state=expanded

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57be8463bb24
Show the font overview in a collapsible section in the font editor; r=rcaliman

Comment 10

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/57be8463bb24

Comment 11

[Catalin Sasca, Desktop QA [:csasca]]

I successfully reproduced the issue on Firefox Nightly 63.0a1 (2018-06-29) under Windows 10 (x64) using the STR from Comment 0.

The issue is no longer reproducible on Firefox Beta 63.0b13 and latest Firefox 64.0a1 (2018-10-09). Tests were performed on Windows 10 (x64), macOS 10.13 and Ubuntu 16.04 (x64).