Closed Bug 1464796 Opened 7 years ago Closed 7 years ago

Font editor: indicate used fonts from font family declaration

Categories

(DevTools :: Inspector, enhancement, P3)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: rcaliman, Assigned: rcaliman)

References

Details

Attachments

(6 files)

Bug 1464796 - (Part 1) Hide list of node used fonts from overview.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1464796 - (Part 2) Select declared and used fonts. Group font family names by used and not used.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1464796 - (Part 3) Show metadata for all used fonts.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1464796 - (Part 4) Make DOM tweaks and add events to aid testing.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1464796 - (Part 5) Update tests.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1464796 - (Part 6) Pick a font from descendants if no declared font was used on node.
59 bytes, text/x-review-board-request
pbro
: review+
Details
The list of used fonts in the font overview will be replaced by the expanded font-family UI in the font editor.
Summary: Font editor: remove used fronts from overview → Font editor: indicate used fonts from font family declaration
Attachment #8981125 - Flags: review?(pbrosset)
Comment on attachment 8981125 [details] Bug 1464796 - (Part 1) Hide list of node used fonts from overview. https://reviewboard.mozilla.org/r/247216/#review255428 Looks fine for now, but this component might need to be re-done or deleted or renamed in the future. Right now it basically shows the list of other fonts found elsewhere. But it also shows a message when the current element has no fonts. Those things look like they don't belong together anymore. The message should probably be in the fonts editor component because that's the main thing we show first now. And this component should probably become a OtherFonts component of some sort. But again, fine for now.
Attachment #8981125 - Flags: review?(pbrosset) → review+
Comment on attachment 8983357 [details] Bug 1464796 - (Part 2) Select declared and used fonts. Group font family names by used and not used. https://reviewboard.mozilla.org/r/249264/#review255440 ::: devtools/client/inspector/fonts/fonts.js:578 (Diff revision 1) > - // Mark available fonts as used if their names appears in the font-family declaration. > - // TODO: sort used fonts in order of font-family declaration. > - for (const font of fonts) { > - font.used = declaredFontNames.includes(font.CSSFamilyName); > + // Subset of fonts used on the node whose family names exist in the CSS font-family > + // property sorted in the order they are defined in font-family. > + const fontsUsed = familiesDeclared.reduce((acc, family) => { > + const match = fonts.find(font => { > + // CSSGeneric is set to the string value of the generic font family from which the > + // font is resolved (ex: "serif", "sans-serif", "monospace") or null otherwise. > + return font.CSSGeneric === family || font.CSSFamilyName === family; > + }); > + > + if (match) { > + acc.push(match); > - } > + } > > + return acc; > + }, []); > + > + const families = {}; > + // Font family names declared and used (sorted). > + families.used = fontsUsed.map(font => > + font.CSSGeneric ? font.CSSGeneric : font.CSSFamilyName > + ); > + // Font family names declared but not used. > + families.notUsed = familiesDeclared.filter(family => !families.used.includes(family)); As discussed on IRC, I think this is fine, but will need some more comments. There's a lot of hidden complexity here that I want to make sure is captured in a comment for new people to understand.
Attachment #8983357 - Flags: review?(pbrosset)
Comment on attachment 8981125 [details] Bug 1464796 - (Part 1) Hide list of node used fonts from overview. https://reviewboard.mozilla.org/r/247216/#review255428 I realised I didn't account for the font editor pref. If this went in as-is, the element fonts will not be rendered at all. I will revert Part 1 of the patch and prevent rendering of element fonts only when the font editor pref is on. Agreed, at a later stage, FontOverview may be rewritten or removed completely. Until that decision, I will keep it relatively untouched.
Blocks: 1464336
Blocks: 1465022
Comment on attachment 8981125 [details] Bug 1464796 - (Part 1) Hide list of node used fonts from overview. https://reviewboard.mozilla.org/r/247216/#review256470
Comment on attachment 8983357 [details] Bug 1464796 - (Part 2) Select declared and used fonts. Group font family names by used and not used. https://reviewboard.mozilla.org/r/249264/#review256488 Looking good, I like the refactoring that moved code and comments to separate functions. I found a bug we need to fix before landing this commit though. Also, there's one part of the spec that isn't implemented yet and I think it belongs to this commit. Or alternatively, it could be done in a separate bug, which is fine: Looking at the list of fonts from the font-family property value on the current node, if none of these fonts are actually used, right now the panel is empty (apart from the "other fonts in page" section). We specified the following in this case: - If the list of fonts returned by the getUsedFontFaces API was empty or if all of the fonts in the list are non-variable, show the non-variable editor UI. - If that list contained exactly 1 variable font, show the editor UI for this font. - If that list contained >= 2 variable fonts, show the non-variable editor UI and a message "Detected multiple variable fonts in descendant elements, please try selecting another element". Probably good to move to another bug. ::: devtools/client/inspector/fonts/fonts.js:189 (Diff revision 2) > + * Subset of `fonts` whose font family names appear in `fontFamilies`. > + */ > + filterFontsUsed(fonts = [], fontFamilies = []) { > + return fontFamilies.reduce((acc, family) => { > + const match = fonts.find(font => { > + return font.CSSGeneric === family || font.CSSFamilyName === family; I've found a bug here where if family is "arial" (lowercase) then it won't be marked as used, because the CSSFamilyName is "Arial" (capitalized). You'll need to lowercase everything here before comparing to avoid this issue. This is important now that we're getting the font-family from the CSS authored style, rather than the computed style. Because the value now comes directly from what the author wrote, and there's no telling what shape or form it could be.
Attachment #8983357 - Flags: review?(pbrosset)
Comment on attachment 8984178 [details] Bug 1464796 - (Part 3) Show metadata for all used fonts. https://reviewboard.mozilla.org/r/249980/#review256514
Attachment #8984178 - Flags: review?(pbrosset) → review+
Comment on attachment 8984179 [details] Bug 1464796 - (Part 4) Make DOM tweaks and add events to aid testing. https://reviewboard.mozilla.org/r/249982/#review256516
Attachment #8984179 - Flags: review?(pbrosset) → review+
Comment on attachment 8984180 [details] Bug 1464796 - (Part 5) Update tests. https://reviewboard.mozilla.org/r/249984/#review256518 ::: devtools/client/inspector/fonts/test/browser.ini (Diff revision 1) > subsuite = clipboard > [browser_fontinspector_edit-previews.js] > [browser_fontinspector_expand-css-code.js] > [browser_fontinspector_other-fonts.js] > [browser_fontinspector_reveal-in-page.js] > -[browser_fontinspector_theme-change.js] As discussed, we still have font previews in the "other fonts in page" section of the panel. So it would be good to keep this test but make it test a font from that section instead. ::: devtools/client/inspector/fonts/test/browser_fontinspector.js:12 (Diff revision 1) > - url: URL_ROOT + "ostrich-regular.ttf", > - cssName: "barnormal" > -}]; > > add_task(async function() { > + Services.prefs.setBoolPref("devtools.inspector.fonteditor.enabled", true); We have access to a better API for this, which does not require to clear the pref at the end of the test: ``` await pushPref("devtools.inspector.fonteditor.enabled", true); ``` Same comment for all the other tests you changed here. ::: devtools/client/inspector/fonts/test/head.js:152 (Diff revision 1) > + * @param {DOMNode} fontEl > + * The font element. > + * @return {String} > + * The URL where the font was loaded from as shown in the UI. > + */ > +function getURL(fontEl) { I don't understand why this new function doesn't get seen by ESLint as a global in tests automatically. This should work out of the box, and you shouldn't need to add the globals comment in that test.
Attachment #8984180 - Flags: review?(pbrosset)
Blocks: 1465424
Product: Firefox → DevTools
Comment on attachment 8983357 [details] Bug 1464796 - (Part 2) Select declared and used fonts. Group font family names by used and not used. https://reviewboard.mozilla.org/r/249264/#review257834 ::: devtools/client/inspector/fonts/fonts.js:423 (Diff revision 3) > + font.CSSGeneric ? font.CSSGeneric : font.CSSFamilyName > + ); > + const familiesUsedLowercase = families.used.map(familiy => familiy.toLowerCase()); > + // Font family names declared but not used. > + families.notUsed = fontFamilies > + .map(familiy => familiy.toLowerCase()) typo: `familiy` should be `family`
Attachment #8983357 - Flags: review?(pbrosset) → review+
Comment on attachment 8984180 [details] Bug 1464796 - (Part 5) Update tests. https://reviewboard.mozilla.org/r/249984/#review257836 Any luck finding out why getURL wasn't being picked up automatically as a test global by ESLint? Not necessary to be fixed before landing this, but I want to make sure we don't forget.
Attachment #8984180 - Flags: review?(pbrosset) → review+
Comment on attachment 8986103 [details] Bug 1464796 - (Part 6) Pick a font from descendants if no declared font was used on node. https://reviewboard.mozilla.org/r/251560/#review257838 ::: devtools/client/inspector/fonts/fonts.js:686 (Diff revision 1) > const axes = parseFontVariationAxes(properties["font-variation-settings"]); > Object.keys(axes).map(axis => { > this.writers.set(axis, this.getWriterForAxis(axis)); > }); > > + // Pick fonts from descendants if no decalred fonts were used on this node. Typo: `decalred`
Attachment #8986103 - Flags: review?(pbrosset) → review+
Pushed by rcaliman@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/66c18fc78671 (Part 1) Hide list of node used fonts from overview. r=pbro https://hg.mozilla.org/integration/autoland/rev/bf5c5830f226 (Part 2) Select declared and used fonts. Group font family names by used and not used. r=pbro https://hg.mozilla.org/integration/autoland/rev/e35ef7dd3a1d (Part 3) Show metadata for all used fonts. r=pbro https://hg.mozilla.org/integration/autoland/rev/779cf6bd23c3 (Part 4) Make DOM tweaks and add events to aid testing. r=pbro https://hg.mozilla.org/integration/autoland/rev/bdb17fcebf8f (Part 5) Update tests. r=pbro https://hg.mozilla.org/integration/autoland/rev/9fccfa690e6b (Part 6) Pick a font from descendants if no declared font was used on node. r=pbro
Depends on: 1471914
Component: Inspector: Fonts → Inspector
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: