Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FontWeight should subsume wght in FontVariation #148026

Open
gnprice opened this issue May 8, 2024 · 7 comments
Open

FontWeight should subsume wght in FontVariation #148026

gnprice opened this issue May 8, 2024 · 7 comments
Labels
a: typography Text rendering, possibly libtxt engine flutter/engine repository. See also e: labels. framework flutter/packages/flutter repository. See also f: labels. P3 Issues that are less important to the Flutter project team-engine Owned by Engine team triaged-engine Triaged by Engine team

Comments

@gnprice
Copy link
Member

gnprice commented May 8, 2024

Problem

Flutter has two ways to specify the font weight in a TextStyle: you can pass either a FontWeight value or a FontVariation with variation wght. For example these look like TextStyle(fontWeight: FontWeight.w500) or TextStyle(fontVariations: [FontVariation('wght', 500)]).

The trouble is that, at present, neither of these subsumes the job of the other, and nor do they interact well with each other.

In general it's impossible for an app to know in advance whether the relevant font will offer variable weight: if it's displaying any kind of user-controlled text, the text may be in a variety of scripts, which may not be covered by the app's primary font and may fall back to a variety of other fonts.

As a result, there doesn't seem to be any existing way for a Flutter app to reliably control the font weight of its text, if any of the fonts it uses offer variable weight.

Proposed solution

(See revised proposal below from @Hixie. The changes to FontWeight are the same, but a wght FontVariation retains a separate role, for backwards compatibility and for use in a particular edge case.)

When fontWeight is specified, interpret it as the exact font weight to use, regardless of whether the font offers variable weight and regardless of whether a wght variation is also present. Consult any wght variation only as a fallback, for compatibility, when fontWeight is absent.

In order to support the same fine-grained range of possible weights currently expressed with a wght FontVariation, add a constructor const FontWeight(this.value) that accepts an int. (The FontWeight type is already a class with a field int value.)

This is the same API that CSS offers: there's just one attribute font-weight, it accepts any number from 1 to 1000, and it works regardless of whether the font ultimately used offers variable weight. This design seems to have worked out well for CSS.

Implementation

I think most of the work to implement this would be in the engine. The framework would pass the engine a single font-weight value from 1 to 1000, rather than both a fontWeight encoded as an int from 0 to 8 and a wght variation from 1 to 1000; the engine then needs to use that single value when it comes time to actually select fonts.

Fundamentally this is the same sort of job the engine is already successfully handling: if the app said FontWeight.w500, and the only otherwise-matching fonts are at weights 400 and 700 (the usual "normal" and "bold" values), then the engine has to somehow pick one of those. What's new is just that the weight might be 517 instead of 500.

For the exact behavior to use with that single weight, one natural model to follow is what CSS specifies for the web. That behavior is described on MDN here and here. It's found in the CSS spec here and at the step beginning "font-weight is matched next" within the font-matching algorithm (beginning a little before this example).

Alternatively, if it turns out there's some relatively easy change that can be made to the existing code to handle this, but it has some other behavior that's different from CSS's, then that's probably fine too.

@gnprice gnprice added framework flutter/packages/flutter repository. See also f: labels. engine flutter/engine repository. See also e: labels. a: typography Text rendering, possibly libtxt team-engine Owned by Engine team labels May 8, 2024
@gnprice
Copy link
Member Author

gnprice commented May 8, 2024

A small other implementation note, copying over from a comment I wrote on another issue, just so it's visible here in case it's helpful:

On the framework side, one question is what to do about the FontWeight.index field. It feels like morally an implementation detail, so potentially nothing outside Flutter's own code is actually using it, making it removable without a breaking change… but that's probably too much to hope for. More realistically, it could become int get index => max(0, value ~/ 100 - 1); (or a variation with different rounding) in order to preserve its behavior exactly on the existing values of FontWeight. That should be enough to ensure that the change doesn't break any existing apps.

@jonahwilliams
Copy link
Member

FYI @Hixie , in case you have thoughts on how these APIs should interact.

@jonahwilliams jonahwilliams added P3 Issues that are less important to the Flutter project triaged-engine Triaged by Engine team labels May 13, 2024
@Hixie
Copy link
Contributor

Hixie commented May 13, 2024

Interesting question.

I agree that we should do something about this.

In practice today what we do is strictly correct, in that fontWeight selects a different font file, and wght configures the font. It's quite plausible for there to be a "bold" font with wght support and a normal font in the same family with wght support and I suppose in theory they could be different and one might want to select one rather than the other. But that's such an edge case that I don't think we should worry about it. (That said, I think the proposal below would work fine even in that case).

The easiest solution is probably just to make FontWeight have a constructor that sets value (with index set to -1 in that case), change lerp to lerp on value instead of index, send the value rather than the index to the engine, and have the engine use the following algorithm instead of what it does now:

  • select a font as now except for weights, pick the font nearest to the value rather than the nearest to the (index+1)*100 or whatever we did before.
  • once a font is selected, if wght is specified, use that, otherwise, imply a wght from the value of fontWeight.

(By "easiest" I mean "simplest", which I think is definitely valuable here. CSS makes this way too complicated.)

@gnprice
Copy link
Member Author

gnprice commented May 13, 2024

That proposal sounds great to me, except for this step:

  • once a font is selected, if wght is specified, use that, otherwise, imply a wght from the value of fontWeight.

This would mean there are still two independent notions of "font weight" in play: the one specified in fontWeight, and the one specified in a FontVariation with wght. To my mind, that's less simple than if we can get to an API where there's only a single axis for "font weight".

I think we can accomplish that by just having this step use the value of fontWeight as the wght to use with the font, and fall back to any FontVariation only if there was no fontWeight.

Then in fact the framework doesn't need to send a wght FontVariation to the engine at all: it sends the FontWeight.value if it has one, and otherwise the wght from a FontVariation.

@Hixie
Copy link
Contributor

Hixie commented May 13, 2024

In practice nobody would use wght so it's far simpler to just not special case it at all, IMHO. The framework would not treat wght any differently than ABCD. It just wouldn't know anything about it.

The reason I think the engine should honor wght over FontWeight, aside from the backwards compatible needs (doing the opposite would break any app that uses wght today) is that it enables the weird edge case I mentioned earlier, where if you really do have two variable fonts that you want to select by weight and then animate internally, you can. It ends up being a more coherent overall design: if there's a font variant, it always takes precedence over everything, and the engine doesn't have to special case any of them. But the engine can also slide in some extra ones if they're not specified, such as wght.

@gnprice
Copy link
Member Author

gnprice commented May 14, 2024

OK, that makes sense.

A crucial part of making that plan work will be to make it clear in the docs that wght isn't something you normally want to use (i.e., not unless you specifically want that weird edge case). So in particular the example at FontVariation should be changed to something else, and FontVariation.weight should explicitly say that FontWeight already expresses the same thing.

@Hixie
Copy link
Contributor

Hixie commented May 14, 2024

agreed

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 7, 2024
We could probably have left out the `const TextStyle().merge()`; I
think the only difference is in the resulting style's debugLabel.
But I figured we might as well be consistent in how we use
weightVariableTextStyle, and in any case it should disappear with
  flutter/flutter#148026 .
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Jun 7, 2024
This converts the last remaining place where we were using
TextStyle.fontWeight directly, when we should have been using
weightVariableTextStyle.

(In a future with
  flutter/flutter#148026
, though, all uses of weightVariableTextStyle should be changed back
to plain TextStyle.fontWeight.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: typography Text rendering, possibly libtxt engine flutter/engine repository. See also e: labels. framework flutter/packages/flutter repository. See also f: labels. P3 Issues that are less important to the Flutter project team-engine Owned by Engine team triaged-engine Triaged by Engine team
Projects
None yet
Development

No branches or pull requests

3 participants