Closed Bug 1463115 Opened 7 years ago Closed 7 years ago

Add a new skipWhitespace option to InspectorUtils.getUsedFontFaces

Categories

(Core :: Layout: Text and Fonts, enhancement, P3)

Product:
Core
Component:
Layout: Text and Fonts
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: pbro, Assigned: jfkthame)

References

(
URL
)

Details

Attachments

(1 file)

Try to skip irrelevant (collapsed/trimmed) whitespace when collecting used font faces for devtools inspector
9.07 KB, patch
jwatt
: review+
pbro
: feedback+
Details | Diff | Splinter Review
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.
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)
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.
Priority: -- → P3
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)
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: nobody → jfkthame
Status: NEW → ASSIGNED
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.
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+
Attachment #8980334 - Flags: review?(jwatt)
Attachment #8980334 - Flags: review?(jwatt) → review+
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
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f63e09b914570efce8e047f4f51aaf58ead46f75 for frequent mochitest failures on Linux debug.
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
https://hg.mozilla.org/mozilla-central/rev/b7688ef14e3a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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.

Attachment

General

Creator:
Created:
Updated:
Size: