Add a new skipWhitespace option to InspectorUtils.getUsedFontFaces

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: pbro, Assigned: jfkthame)

Tracking

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

11 months ago
STR:
- open the following page
data:text/html,<style>body{font-family:sans-serif}div{font-family:serif}</style><body> <div>text</div></body>
- open DevTools, the inspector, and the Fonts panel in the sidebar
- with <body> selected in the inspector, see that the Fonts panel displays 2 fonts

We would like there to be a way to avoid retrieving fonts for whitespaces.
Indeed, in this example, there's a space textnode, before the <div>, and because <body> has a defined font-family of sans-serif, then DevTools shows the font for that.

We believe that it will be much more logical for users not to see these fonts in the panel (perhaps in the future we could have an advanced mode that does show them, but for now we don't).

So, we would like the API InspectorUtils.getUsedFontFaces to accept an extra parameter (e.g. skipWhitespace) to do this.

We could still do this at the DevTools level now, in JS, but that will be hard and possibly slow. 
Especially when doing this on top-level nodes such as <body>. We'd have to construct ranges for each and every non-whitespace text nodes in the inspected element.

For context, this is going to be very important for us as we make the final touches to our MVP for the Fonts panel in Firefox 62. Indeed, we're wanting to display the fonts specified in CSS vs. the fonts actually used in this panel, for the currently selected node. And right now, showing the fonts that were used for whitespace text nodes is confusing to users.
(Reporter)

Comment 1

11 months ago
Martin: We talked last week about the fact that we showed fonts for whitespaces. Do you agree with the need to stop doing this going forward?
Blocks: 1441576
Flags: needinfo?(mbalfanz)
(Assignee)

Comment 2

11 months ago
I wonder if we need to allow for the API to make a distinction between collapsed whitespace (which should almost certainly be ignored) and non-collapsed? In your example, the whitespace is all collapsed and so knowing the font assigned to it is hardly ever going to be of interest; but consider a variation such as

  data:text/html,<div style="font-family:Apple Color Emoji, serif">hello world</div>

on macOS. This shows an unusually wide space between the two words, and the reason for this is that the space is taken from the Apple Color Emoji font, even though all the visible letters fall back to the serif font. If we _don't_ show the font assigned to the (non-collapsed) whitespace here, so that DevTools claims the text is entirely Times Roman, that will also be quite confusing.
(Assignee)

Updated

11 months ago
Priority: -- → P3
(Reporter)

Comment 3

11 months ago
I think limiting this to collapsed whitespace would be the right thing to do indeed. Thanks for bringing that up.

Martin and I had discussed about this particular test case:
- go to https://developer.microsoft.com/en-us/microsoft-edge/testdrive/demos/variable-fonts/
- open the inspector and select the .intro__masthead element

=> the fonts panel shows 2 fonts: Bahnschrift and Segoe UI.
However the text content really only uses Bahnschrift (there are 4 spans in this subtree, and 1 non-collapsed whitespace between the words variable and fonts).

So if the option makes the API return Bahnschrift only in this case, I think that is already a really good thing.
I agree we should only address collapsed whitespace.

I found many more cases where this becomes an issue, esp. for sites that use non-minified HTML. We could vastly improve our font list if we would ignore these, but respect non-collapsed whitespace.
Flags: needinfo?(mbalfanz)
(Assignee)

Comment 5

11 months ago
Here's an experimental patch to see how it goes. Tryserver build available at https://treeherder.mozilla.org/#/jobs?repo=try&revision=563670df7ed874db9215306d59101b304221329d. Patrick, does this help with your use cases?
Attachment #8980334 - Flags: feedback?(pbrosset)
(Assignee)

Updated

11 months ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Comment 6

11 months ago
BTW, in this patch the new skipCollapsedWhitespace option defaults to true, so you should see the change in behavior automatically, without needing to tweak the devtools code at all. For the example from comment 3, at least, it seems to have the desired result.
(Reporter)

Comment 7

11 months ago
Comment on attachment 8980334 [details] [diff] [review]
Try to skip irrelevant (collapsed/trimmed) whitespace when collecting used font faces for devtools inspector

I tried it, and it worked beautifully!
Thanks.
Attachment #8980334 - Flags: feedback?(pbrosset) → feedback+
(Assignee)

Updated

11 months ago
Attachment #8980334 - Flags: review?(jwatt)

Updated

11 months ago
Attachment #8980334 - Flags: review?(jwatt) → review+

Comment 8

11 months ago
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/36687c035662
Try to skip irrelevant (collapsed/trimmed) whitespace when collecting used font faces for devtools inspector. r=jwatt
(Assignee)

Comment 9

11 months ago
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f63e09b914570efce8e047f4f51aaf58ead46f75 for frequent mochitest failures on Linux debug.

Comment 10

11 months ago
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7688ef14e3a
Try to skip irrelevant (collapsed/trimmed) whitespace when collecting used font faces for devtools inspector. r=jwatt

Comment 11

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b7688ef14e3a
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(Reporter)

Comment 12

11 months ago
Thank you very much Jonathan and Jonathan for working on this!
You need to log in before you can comment on or make changes to this bug.