Font editor: property values should be read from rules and use computed style as fallback

RESOLVED FIXED in Firefox 62

Status

defect
P2
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: rcaliman, Assigned: rcaliman)

Tracking

unspecified
Firefox 62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments)

The patch for Bug 1463055 replaced reading property values from rules with reading from computed style. This means units like "em" are read as computed "px" for properties like font-size. The editor writes back updated values with "px" instead of "em".

The font editor should attempt to read CSS font property values from the rules which apply to the element and fallback to computed style when explicit declarations were not found.
Comment on attachment 8983356 [details]
Bug 1465397 - Font Editor: read font properties from computed style and overwrite with ones explicitly declared. .

https://reviewboard.mozilla.org/r/249262/#review255444

::: devtools/client/inspector/fonts/fonts.js:180
(Diff revision 1)
> +            !textProp.value.includes("calc") &&
> +            !textProp.value.includes("var") &&

What if there's a font-family or a custom axis (in font-variation-settings) that contain the sustrings calc or var?
e.g. `font-family: "calc", serif;`
In this case, we'd fallback to the computed style where we really shouldn't.
Given the description of the bug, I don't think this will matter much, because those strings are not going to appear in properties that have units. But still, we should make this more correct. Maybe matching on `calc(` and `var(` would be enough of a fix, without necessarily having to use a parser.
Attachment #8983356 - Flags: review?(pbrosset)
Comment on attachment 8983356 [details]
Bug 1465397 - Font Editor: read font properties from computed style and overwrite with ones explicitly declared. .

https://reviewboard.mozilla.org/r/249262/#review255444

> What if there's a font-family or a custom axis (in font-variation-settings) that contain the sustrings calc or var?
> e.g. `font-family: "calc", serif;`
> In this case, we'd fallback to the computed style where we really shouldn't.
> Given the description of the bug, I don't think this will matter much, because those strings are not going to appear in properties that have units. But still, we should make this more correct. Maybe matching on `calc(` and `var(` would be enough of a fix, without necessarily having to use a parser.

Yes, this is just a way to correctly display values with units (Ex: font-size computed value would be px, but declared value would be em). This guard wouldn't affect font-family or axes since defaulting to computed style sould result in the same value. I can add the extra parenthesis in the check.
Comment on attachment 8983356 [details]
Bug 1465397 - Font Editor: read font properties from computed style and overwrite with ones explicitly declared. .

https://reviewboard.mozilla.org/r/249262/#review255716
Attachment #8983356 - Flags: review?(pbrosset) → review+
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e0d8aa3344b8
Font Editor: read font properties from computed style and overwrite with ones explicitly declared. r=pbro.
https://hg.mozilla.org/mozilla-central/rev/e0d8aa3344b8
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
I think this breaks font-weight now if the user didn't specify a numeric value.
Flags: needinfo?(rcaliman)
See Also: → 1467408
Good catch! I did not account for property-specific keywords, like "bold", "bolder" or "lighter" in this patch (only generic "inherit", "initial", "unset", and instances of var() and calc()).

In all those cases, the font editor should fallback to computed values. I opened Bug 1467408 to track this.
Flags: needinfo?(rcaliman)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.