1473314 - Make InspectorUtils.getUsedFontFaces return the faces in a consistent order [was: Font editor displays wrong font name, sometimes]

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Martin Balfanz [:mbalfanz]]

Attachment: Screen Shot 2018-07-04 at 15.27.41.png

The font editor displays both the font family name and the font name.  However, if multiple font names apply (e.g. multiple fonts are used to render an element) the displayed font name may sometimes be wrong.

The attached screenshot shows a selected element that mainly uses Fira Sans.  It also has an `i` child that uses Fira Sans Italic.  In this case, the italic version is displayed even though it shouldn't.  This is very confusing for the end user, as the font name says italic while the respective toggle is flipped off.

Apparently there is no guarantee for the order in the rendered fonts list.


STR:
1. Visit [1] and inspect the .comment element in the platform section
2. Open the fonts panel and look at the Family

Expected result:
- Font family name and font name should both be Fira Sans

Actual result:
- Font family name is Fira Sans, but font name is Fira Sans Italic



[1]: https://bugzilla.mozilla.org/enter_bug.cgi?product=DevTools&component=Font%20Inspector

Comment 1

[Martin Balfanz [:mbalfanz]]

Attachment: Screen Shot 2018-07-04 at 15.32.57.jpg

The same example on a different machine, showing the correct result.

Comment 2

[Patrick Brosset <:pbro>]

(In reply to Martin Balfanz [:mbalfanz] from comment #0)
> Apparently there is no guarantee for the order in the rendered fonts list.
The getUsedFontFaces API we used behind the scenes to get the list of fonts used to render some content might not have any guaranty in terms of order it seems (judging by the fact that 2 machines seem to return 2 different results).

Anyway, this is probably a fix on DevTools around the filterFontsUsed function in /devtools/client/inspector/fonts/fonts.js where we go through the font-families defined on the node, and try to match those up to the returned value from getUsedFontFaces to see which of these font families is/are used.
We use the font's CSSFamilyName property, which in both cases is Fira Sans (not Fira Sans Italic, that's the font name, not its CSSFamilyName). So there's probably a bug where we stop as soon as we got the first one, and if the API happens to be returning the italic one first, then that's the one we show.

Comment 3

[Razvan Caliman [:rcaliman]]

This no longer seems to happen. Can you please check and close if true, Martin?

Comment 4

[Martin Balfanz [:mbalfanz]]

Attachment: Screenshot_2018-09-07 15.14.19_E6aErP.png

The issue still exists for me.

While with the new design both families are listed, it still shows Fira Sans Italic first in the list.  What is also confusing then is that the italic toggle is not active.

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Martin Balfanz [:mbalfanz] from comment #4)
> Created attachment 9007203 [details]
> Screenshot_2018-09-07 15.14.19_E6aErP.png
> 
> The issue still exists for me.
> 
> While with the new design both families are listed,

Small terminology nit: this should be "both *faces* are listed"; they both belong to the single *family* Fira Sans.

I think this is expected: the UI is correctly showing that the selected content (i.e. the .comment element, including its children) is using two faces of the Fira Sans family.

> it still shows Fira Sans
> Italic first in the list.

The getUsedFontFaces API collects the faces from the given range into a set, and then returns the members present in that set; I'm afraid it doesn't offer any particular guarantee as to the order they'll be returned in.

I suppose the font editor could attempt to sort them in some way, but it's not obvious to me what the most useful order would be. In this example, listing the Regular face before the Italic would seem most natural, but in more complex cases it is less clear what order would be best. E.g. if the range included Light, Regular, Bold and Heavy faces, should we list them in order of weight? Or always put Regular first, followed by other weights? Do we order first by weight and then by italicness, or vice versa? Or should we sort the faces according to the order of their occurrence within the inspected range?

>  What is also confusing then is that the italic
> toggle is not active.

I guess that control (along with the weight slider, etc) reflects the style specified on the root element of the selected range. (I wonder what it does if the range begins in one element, and ends in a sibling element with different style?)

So this would be less confusing if we listed the Regular face first, and then the Italic. But suppose the .comment element were styled with italic (and its inline child then used regular); then we'd still have the same two faces in the list, but the italic toggle would be turned on, so I guess we'd want to list the italic face first.

Comment 6

[Jonathan Kew [:jfkthame]]

Martin, I was just thinking about this a bit, and one thing we could do fairly easily is to make getUsedFontFaces report the font faces in the order in which they're first encountered as it works through the given range (rather than returning a randomly-ordered collection).

So in the original example here, where the selected element uses Fira Sans Regular with a child element in the middle that uses Fira Sans Italic, they'll consistently be returned in the order <Regular, Italic>. But if the italic child element were the first thing, before any regular text, they'd be returned as <Italic, Regular>.

Does that seem like a worthwhile improvement? At least it would provide a more consistent result, with a visible relation to the content being inspected. If you're in favor of this, I'll post a patch for it.

Comment 7

[Martin Balfanz [:mbalfanz]]

That sounds fantastic!  I like the approach because it feels to me that this is the logic it should be in, but also because it gives more predictable results the user.

So yes, I would really appreciate a patch for it :-)

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Make nsRange::GetUsedFontFaces accumulate font faces in the order they are encountered in the document

So the idea here is simply to append faces to the result array as soon as we find them during the traversal, rather than waiting until the end and then iterating over the gfxFontEntry-to-InspectorFontFace map, which gives us an unpredictable order.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a05984b5e160
Make nsRange::GetUsedFontFaces accumulate font faces in the order they are encountered in the document. r=xidorn

Comment 10

[Raul Gurzau (:RaulG)]

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