Closed Bug 1464846 Opened Last year Closed Last year

Issues with inherited values

Categories

(DevTools :: Inspector: Fonts, defect, P2)

defect

Tracking

(firefox62 fixed)

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: mbalfanz, Assigned: rcaliman)

References

Details

Attachments

(3 files)

When values for font-variation-settings are inherited, the editor behaves very unexpected.

The first video shows how touching a slider for variable axis SERI changes the font weight to it's default of 400. The second video is another example.

STR:
1. go to v-fonts.com
2. open the fonts editor on a div element that inherits font-variation-settings from a ul.fonts
3. touch a slider, for example for width

Actual result:
- font-variation-settings is set to initial
- width is set to font-stretch

Expected result:
- changes should be written to font-variation-settings OR the width should be written to font-stretch while all other font values are respected
Attached video sliders-on-click.mp4
Assignee: nobody → rcaliman
Status: NEW → ASSIGNED
(In reply to Martin Balfanz [:mbalfanz] from comment #0)
> Created attachment 8981176 [details]
> overwrite-on-inherit.mp4
> 
> When values for font-variation-settings are inherited, the editor behaves
> very unexpected.
> 
> The first video shows how touching a slider for variable axis SERI changes
> the font weight to it's default of 400.

I'm very intrigued by this because when I change the SERI slider alone I don't expect the weight to change too.
So I avoided loading the inspector at all, to make sure it did not have any impact on the page, I opened the console only, and executed this:
$$(".sample")[2].style.fontVariationSettings = '"SERI" 10';

Doing this not only changes the value of the SERI axis, but also sets the weight to 400.
Can anyone explain to me why this happens? I'm confused, but also if this happens without the font tool being opened at all, isn't this expected?
Digging some more ... If you use the sliders on the demo page itself, then all axis are written at once in the font-variation-settings property. While when you use the fonts devtools, only the axis you modify is written in the font-variation-settings property.
Should the tool write a value for font-variation-settings that contains something for each and every axis? To prevent unexpected changes to other axis?
The fonts devtools editor also writes updates axes all at once into `font-variation-settings`. It does so for those that were not matched with a CSS font property (ex, registered axes).

See: https://searchfox.org/mozilla-central/source/devtools/client/inspector/fonts/fonts.js#653

This writes values for all the axes that were originally found on the node including those updated with the editor in the meanwhile. It _should_ filter out registered axes that _must_ be written to CSS font properties. 

But I just discovered that filtering doesn't happen if the registered axis that _should_ be written to a CSS font property was not touched before the custom axis. The writer method gets assigned to the axis when its corresponding slider updates. In this scenario it is still default, thus the registered axis is written to font-variation-settings". 

See: https://searchfox.org/mozilla-central/source/devtools/client/inspector/fonts/fonts.js#317

I tried to be frugal with assigning writers and did it only on-demand. But it seems that they should be assigned on setup/refresh as opposed to onchange to mitigate cases this scenario.

I don't think this is the root cause of this bug in particular, but it definitely impacts others.
This issue here should be solved by the patch attached and the patch for Bug 1464801 (already landed).

- The patch attached replaces the "initial" value with the desired value when the property is created.

- The patch for Bug 1464801 ensures that axes values declared in 'font-variation-settings' won't be overwritten if they've never been touched in the editor.
Comment on attachment 8983362 [details]
Bug 1464846 - Font Editor: create new property with desired value instead of initial. .

https://reviewboard.mozilla.org/r/249268/#review255422
Attachment #8983362 - Flags: review?(pbrosset) → review+
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/600510ecf7d2
Font Editor: create new property with desired value instead of initial. r=pbro.
https://hg.mozilla.org/mozilla-central/rev/600510ecf7d2
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.