Closed
Bug 1463115
Opened 6 years ago
Closed 6 years ago
Add a new skipWhitespace option to InspectorUtils.getUsedFontFaces
Categories
(Core :: Layout: Text and Fonts, enhancement, P3)
Core
Layout: Text and Fonts
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: pbro, Assigned: jfkthame)
References
()
Details
Attachments
(1 file)
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•6 years 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•6 years 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•6 years ago
|
Priority: -- → P3
Reporter | ||
Comment 3•6 years 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.
Comment 4•6 years ago
|
||
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•6 years 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•6 years ago
|
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•6 years 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•6 years 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•6 years ago
|
Attachment #8980334 -
Flags: review?(jwatt)
![]() |
||
Updated•6 years ago
|
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
Assignee | ||
Comment 9•6 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f63e09b914570efce8e047f4f51aaf58ead46f75 for frequent mochitest failures on Linux debug.
Comment 10•6 years 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b7688ef14e3a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Reporter | ||
Comment 12•6 years 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.
Description
•