Closed Bug 1463055 Opened 4 years ago Closed 4 years ago

Font editor: Refactor to remove toggle from ruleview and write changes to node inline style

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox62 fixed)

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: rcaliman, Assigned: rcaliman)

References

Details

Attachments

(4 files)

Resulting from consensus reached in a meeting between :pbro, :mbalfanz and :rcaliman and :jensimmons we resolve to:

- remove the font editor toggle from the Rule view;
- read all font properties from the computed style of the selected node;
- write all font property changes to the selected node's inline style;
- pick first used font declared in `font-family` property.
Status: NEW → ASSIGNED
Duplicate of this bug: 1462589
Duplicate of this bug: 1462587
Comment on attachment 8979174 [details]
Bug 1463055 - Font Editor: remove toggle from rule view and associated logic.

https://reviewboard.mozilla.org/r/245424/#review251350
Attachment #8979174 - Flags: review?(pbrosset) → review+
Comment on attachment 8979175 [details]
Bug 1463055 - Font editor: merge editor panel with overview (temporary until redesign).

https://reviewboard.mozilla.org/r/245426/#review251354
Attachment #8979175 - Flags: review?(pbrosset) → review+
Comment on attachment 8979176 [details]
Bug 1463055 - Font editor: read properties from computed style; write to inline style.

https://reviewboard.mozilla.org/r/245428/#review251356

::: devtools/client/inspector/fonts/test/browser.ini:27
(Diff revision 1)
>  [browser_fontinspector_edit-previews.js]
>  [browser_fontinspector_expand-css-code.js]
>  [browser_fontinspector_other-fonts.js]
>  [browser_fontinspector_reveal-in-page.js]
>  [browser_fontinspector_text-node.js]
> +skip-if = true # Drop support for TextNodes from Font Editor MVP

Not sure what to think of this. If we're sure we won't ever do anything with text nodes, then let's just delete the test.
If we just don't know yet, keeping it around but not running is risky because there's a high chance we wont work on this for the MVP scope and the test will just stay there for a long time.
But if we remove it, there's a chance the knowledge we ever had support for text nodes disappear.

My suggestion: delete the test entirely, file a bug for us to think about text node support in the fonts panel, and reference this old test in that bug.
Attachment #8979176 - Flags: review?(pbrosset)
Comment on attachment 8979177 [details]
Bug 1463055 - Font editor: edit font that is both used and declared in font-family.

https://reviewboard.mozilla.org/r/245430/#review251358

::: devtools/client/inspector/fonts/fonts.js:569
(Diff revision 1)
>        this.ruleView.rules.find(rule => rule.style.type === ELEMENT_STYLE);
>      const fontEditor = this.store.getState().fontEditor;
>      const properties = this.getFontProperties();
> +    // Names of fonts declared in font-family property without quotes and space trimmed.
> +    const declaredFontNames =
> +      properties["font-family"].split(",").map(font => font.replace(/\"+/g, "").trim());

Are you certain there can only be double-quotes, or should this also handle single-quotes?
Attachment #8979177 - Flags: review?(pbrosset) → review+
See Also: → 1463084
Comment on attachment 8979176 [details]
Bug 1463055 - Font editor: read properties from computed style; write to inline style.

https://reviewboard.mozilla.org/r/245428/#review251356

> Not sure what to think of this. If we're sure we won't ever do anything with text nodes, then let's just delete the test.
> If we just don't know yet, keeping it around but not running is risky because there's a high chance we wont work on this for the MVP scope and the test will just stay there for a long time.
> But if we remove it, there's a chance the knowledge we ever had support for text nodes disappear.
> 
> My suggestion: delete the test entirely, file a bug for us to think about text node support in the fonts panel, and reference this old test in that bug.

Opened Bug 1463084 and removed test.
Comment on attachment 8979177 [details]
Bug 1463055 - Font editor: edit font that is both used and declared in font-family.

https://reviewboard.mozilla.org/r/245430/#review251386

::: devtools/client/inspector/fonts/fonts.js:569
(Diff revision 1)
>        this.ruleView.rules.find(rule => rule.style.type === ELEMENT_STYLE);
>      const fontEditor = this.store.getState().fontEditor;
>      const properties = this.getFontProperties();
> +    // Names of fonts declared in font-family property without quotes and space trimmed.
> +    const declaredFontNames =
> +      properties["font-family"].split(",").map(font => font.replace(/\"+/g, "").trim());

getComputedStyle() seems to always return double qoutes regardless of the input style.
Comment on attachment 8979176 [details]
Bug 1463055 - Font editor: read properties from computed style; write to inline style.

https://reviewboard.mozilla.org/r/245428/#review251394
Attachment #8979176 - Flags: review?(pbrosset) → review+
Blocks: 1463103
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/125efe55befb
Font Editor: remove toggle from rule view and associated logic. r=pbro
https://hg.mozilla.org/integration/autoland/rev/52b3ed8fa57d
Font editor: merge editor panel with overview (temporary until redesign). r=pbro
https://hg.mozilla.org/integration/autoland/rev/59dd330ceb81
Font editor: read properties from computed style; write to inline style. r=pbro
https://hg.mozilla.org/integration/autoland/rev/767352c4858e
Font editor: edit font that is both used and declared in font-family. r=pbro
There were some leftover CSS variables which were unused and caused that test to fail. It's clean now.
Flags: needinfo?(rcaliman)
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e60ad6a0186a
Font Editor: remove toggle from rule view and associated logic. r=pbro
https://hg.mozilla.org/integration/autoland/rev/ed8005f5f297
Font editor: merge editor panel with overview (temporary until redesign). r=pbro
https://hg.mozilla.org/integration/autoland/rev/5bc4309db28c
Font editor: read properties from computed style; write to inline style. r=pbro
https://hg.mozilla.org/integration/autoland/rev/c13beb863500
Font editor: edit font that is both used and declared in font-family. r=pbro
Product: Firefox → DevTools
Duplicate of this bug: 1453940
Component: Inspector: Fonts → Inspector
You need to log in before you can comment on or make changes to this bug.