'toFont' helper can affect lineHeight if called with a result object from a previous 'toFont' invocation

[stockiNail]

Feature Proposal

Currently, if you invoke toFont helpers function passing as an argument the result object (anyway a font object), the lineHeight property is not correct because the original lineHeight overridden.

const original = {
  size: 12
};
const first = Chart.helpers.toFont(original);
const second = Chart.helpers.toFont(first);

Result:

Original: {
   "size": 12
}

First: {
   "family": "'Helvetica Neue', 'Helvetica', 'Arial', sans-serif",
   "lineHeight": 14.399999999999999,
   "size": 12,
   "style": "normal",
   "weight": null,
   "string": "normal 12px 'Helvetica Neue', 'Helvetica', 'Arial', sans-serif"
}

Second: {
   "family": "'Helvetica Neue', 'Helvetica', 'Arial', sans-serif",
   "lineHeight": 172.79999999999998,
   "size": 12,
   "style": "normal",
   "weight": null,
   "string": "normal 12px 'Helvetica Neue', 'Helvetica', 'Arial', sans-serif"
}

I think the lineHeight property should maintain always the original value and adding another property for the result lineHeight.

Possible Implementation

Current Implementation (helpers.options.ts, row 130):

  const font = {
    family: valueOrDefault(options.family, fallback.family),
    lineHeight: toLineHeight(valueOrDefault(options.lineHeight, fallback.lineHeight), size),
    size,
    style,
    weight: valueOrDefault(options.weight, fallback.weight),
    string: ''
  };

  font.string = toFontString(font);
  return font;

Possible implementation;

  const font = {
    family: valueOrDefault(options.family, fallback.family),
    lineHeight: valueOrDefault(options.lineHeight, fallback.lineHeight),
    size,
    style,
    weight: valueOrDefault(options.weight, fallback.weight),
    // new calculated properties
    string: '',
    lh: toLineHeight(valueOrDefault(options.lineHeight, fallback.lineHeight), size),
  };

  font.string = toFontString(font);
  return font;

Of course, if implemented, it will be a breaking change.
What do you think?

[LeeLenaleee]

But if you increase your font size it should also increase the lineheight otherwise it wont fit 🤔

[stockiNail]

@LeeLenaleee I agree but my concern is related to the "meaning" of lineHeight property in the FontSpec, because with this implementation is not respected comparing argument and return value.

Let me start from some assumptions.

toFont function signature is getting FontSpec as arguments and returns a CanvasFontSpec which is an extension of FontSpec.

The FontSpec contains the lineHeight property which has got 1.2 value as default.
Therefore the lineHeight is not the real line height. In fact, MDN is defining lineHeight as <number> (unitless).

The used value is this unitless multiplied by the element's own font size. The computed value is the same as the specified . In most cases, this is the preferred way to set line-height and avoid unexpected results due to inheritance.

Coming back from the simple test (see comments):

const original = {
  size: 12
}; // Original doesn't have any lineHeight, therefore default 1.2 is used (factor)
const first = Chart.helpers.toFont(original);
// First (which is a font) has got lineHeight with value 14.4 therefore the meaning of lineHeight is changed
// not longer a factor but real line height

That's why I'm proposing that toFont doesn't change lineHeight property (which is a factor and should remain) but create another properties with real calculated line height.
In this way, using always the above simple test, the result will be

Original: {
   "size": 12
}

First: {
   "family": "'Helvetica Neue', 'Helvetica', 'Arial', sans-serif",
   "lineHeight": 1.2,
   "size": 12,
   "style": "normal",
   "weight": null,
   "lh": 14.399999999999999,
   "string": "normal 12px 'Helvetica Neue', 'Helvetica', 'Arial', sans-serif"
}

Second: {
   "family": "'Helvetica Neue', 'Helvetica', 'Arial', sans-serif",
   "lineHeight": 1.2,
   "size": 12,
   "style": "normal",
   "weight": null,
   "lh": 14.399999999999999,
   "string": "normal 12px 'Helvetica Neue', 'Helvetica', 'Arial', sans-serif"
}

and therefore, invoking several times toFont function with the result of previous call, the result will remain the same, as expected.

@LeeLenaleee what do you think? Am I wrong? And apologize for long reply.
If you ask me how I saw that, I was implementing PR chartjs/chartjs-plugin-annotation#890 in annotation plugin, I had very strange result re-using the FontSpec object against to toFont function. And then I went in deep. ;)

[stockiNail]

But if you increase your font size it should also increase the lineheight otherwise it wont fit

@LeeLenaleee I thought more about that even if it's not 100% correct. Assuming that the current lineHeight property meaning is the factor to use to calculate the height of the line, I don't have to change the lineHeight because invoking toFont again, the height of the line will be calculated using new size and old lineHeight (with my proposal).

And currently changing the size or whatever other property of FontSpec, you should invoke toFont again anyway to have the font string representation (to apply to context 2d). Maybe CanvasFontSpec could have the current 'string' and new one to get the line of height "get" properties which invoke the calculation of new values (at runtime) instead of creating new CanvasFontSpec invoking toFont.

Something like (in proposed new toFont impl):

  const font = {
    family: valueOrDefault(options.family, fallback.family),
    lineHeight: valueOrDefault(options.lineHeight, fallback.lineHeight),
    size,
    style,
    weight: valueOrDefault(options.weight, fallback.weight),
    // new calculated and readonly properties
    get string() {
      return toFontString(this);
    },
    get heightOfLine() {
      return toLineHeight(this.lineHeight, this.size);
    }
  };
  return font;

In this way, you could

1. have string and lineOfHeight as read-only properties (correctly I guess), because calculated and they shouldn't be changed.
2. change the config of the font and to have new values at runtime without any further toFont invocation.

Let me also add another topic (i think it's an issue that I would like to fix by PR).

toFont function is exported but marked as private

/**
 * Parses font options and returns the font object.
 * @param options - A object that contains font options to be parsed.
 * @param fallback - A object that contains fallback font options.
 * @return The font object.
 * @private
 */

export function toFont(options: Partial<FontSpec>, fallback?: Partial<FontSpec>) {