Bring TextTheme into alignment with the current Material spec #22330

HansMuller · 2018-09-27T00:48:54Z

TextTheme no longer matches https://material.io/design/typography/#type-scale

To sync things up, I've added subtext and overline to TextTheme and documented the correspondence between the names in the new spec and the existing TextTheme style names.

By default, the font sizes, font weights and letter spacing have not changed. To create a theme that uses the new sizes, weights, and spacing, configure a Theme with ThemeData.typography set to:

Typography(
  platorm: platform,
  englishLike: Typography.englishLike2018,
  dense: Typography.dense2018,
  tall: Typography.tall2018,
)

This PR includes incompatible changes:

The MaterialLocalizations localTextGeometry getter was removed and the protected MaterialLocalizations scriptCategory getter is now public and returns a ScriptCategory. Hand written sublasses of MaterialLocalizations should remove the localTextGeomtry getter and make the scriptCategory getter public and have it return the matching ScriptCategory.
The MaterialTextGeometry class was removed
MaterialTextGeometry.englishLike is now Typography.englishLike2014
MaterialTextGeometry.dense is now Typography.dense2014
MaterialTextGeometry.tall is now Typography.tall2014

ajsecord · 2018-09-27T14:52:36Z

Please document the exceptions in the TextTheme class docs to avoid developer confusion in the future. You can probably just copy your two bullet points from the PR description and clean up.

I understand that we can't do everything and don't want to break devs, but we need to document. Thanks!

Hixie · 2018-09-27T16:42:23Z

I don't think we should do this. I think we should push back on Material and ask them to keep using the old set, even if they change the style a bit when the developer wants Material 2.0 mode. It's going to be hugely confusing to have this many different text styles.

ajsecord · 2018-09-27T16:56:04Z

@Hixie can you clarify "this" in your first sentence? Hans' work in general, this PR, the new typography categories, or the changed values?

HansMuller · 2018-09-27T17:41:44Z

Just adding the 13 new TextStyles increases TextTheme's bulk from 11 to 24. If we don't adopt the new letter spacing values by default, then it would be possible to only increase the total to 17 by mapping old names to new ones when they're otherwise identical and leaving button and caption alone, since they're already the same modulo letter spacing:

OLD        NEW
display1   headline4
headline   headline5
title      headline6
subhead    subtitle1
body1      paragraph2
button     button
caption    caption

That leaves the old body2, display4, display3, and display2 without a mapping to the new scheme. If we assume that the 3 old display variants are rarely used we could map them as well:

OLD(font size)        NEW(font size)
display4(112.0)       headline1(96.0)
display3(56.0)        headline2(60.0)
display2(45.0)        headline3(48.0)

The old body2 is the same as the old body except that its fontWeight is FontWeight.w500 ("medium") instead of FontWeight.w400 ("normal" or "regular" per the Material spec). Although it maps most closely to paragraph2 in the new spec, we're already mapping the old body1 to that. So we'll map it to paragraph1. That will lead to a clumsy fontSize change, 14 - 16, when using the completely new typography scheme (version 1, see below).

With all of those mappings in place we're at the minimum, in terms of the new typography spec, of 13 TextStyles.

To enable one to create theme where the typography exactly matches the current Material spec, in terms of letterSpacing, font sizes (display4, display3, and display2) and font weights (paragraph2): add a typography ThemeData parameter, and add a Typography version parameter. The default version would be 0, the new version would be 1.

Hixie · 2018-09-27T22:10:17Z

@ajsecord I mean having a whole new set of names.

@HansMuller that seems reasonable.

We already allow ThemeData to take TextThemes, right? Can we provide named variants for the origanal set of styles and today's set of styles, rather than asking for a version number? Then you would just pass the TextTheme you want (or more likely, pass your own custom TextTheme).

Hixie · 2018-09-27T22:11:34Z

(I'm not sure what names we'd use, the two sets seem very similar and it's not clear what the purposes of each of the different styles are.)

Hixie · 2018-09-28T00:03:56Z

packages/flutter/lib/src/material/typography.dart

i think we should go even further and just not have "headline1" in our API. We should IMHO just have a table in the class API docs that has the mapping from the names in the spec to the names in our API.

(mostly i'm worried that this won't scale. Two years from now, if we have a third set of names, do we add those too? What if they overlap?)

I don't think we'll ever be able to get rid of the old names although we can eliminate their use in the Flutter implementation. It seems reasonable, albeit painful, to sync up with the new names once. But never again.

Hixie · 2018-09-28T00:05:23Z

packages/flutter/lib/src/material/theme.dart

Yes. Matches my predilection for ^u^n in emacs.

Hixie · 2018-09-28T00:05:29Z

packages/flutter/lib/src/material/theme.dart

Hixie · 2018-09-28T00:10:17Z

packages/flutter/lib/src/material/material_localizations.dart

this is a breaking change (since they're expected to implement this, not call it)... but more importantly, i don't think this API change makes much sense. app devs aren't going to care what version it is, they're just going to provide their text theme for their localizations.

but I don't have a good alternative, assuming that we do still want to have different font sizes for each part of the text theme in different locales...

The 3 script categories are defined here:
https://material.io/design/typography/language-support.html#language-categories-reference

The adjustments to dense and tall scripts are based on:

Type sizes smaller than title styles should make adjustments to the Latin type scale.

In our tall and dense themes the smaller than "title" font sizes have been increased by one.

For tall scripts:

Use regular weight, as medium weight is unavailable in Noto. Avoid using the bold weight, as bold is too heavy.

We replaced the medium weights in the tall theme: FontWeight.w500 with FontWeight.w400.

The dense text theme uses TextBaseline.ideographic instead of TextBaseline.alphabetic.

ajsecord · 2018-09-28T15:15:59Z

@Hixie it sounds like you're not on board with any of these changes? I might be reading your comments wrong...but comments in Hans' code isn't the right place to clarify, so I'll follow up offline.

Tactically, these names changed ~5 months ago at Google I/O 2018 to better match actual usage in the wild. My ultimate preference would be to have the APIs match the spec so developers can move faster with less confusion.

My suggestion, in decreasing order of desirability:

Mark the old names deprecated and remove them in your next release.
Add the new names, keep the old names, document that the new names are preferred.
Keep the old names and document a lookup table for developers to translate through.

I believe Hans already implemented the second option in this PR, it's a reasonable compromise if changing the names is impossible.

Hixie · 2018-09-28T23:04:57Z

I think #1 is a non-starter, #2 would likely be really confusing (especially since the names overlap), and #3 is probably the easiest since I expect most Flutter developers will only use the Flutter API docs and not look at the Material documentation (so not need the mapping at all). There's also #4 which would be for the Material documentation to change the names to match the previous set of names so that there's no need for confusion. :-)

Re the "version" thing in the API, @HansMuller and I chatted earlier and one option might be to pass through a BuildContext to the localization code so that it can look something up (not sure what exactly, but at least then it would have all the context it needs). The problem is that there's two inputs and both control things (locale and version, there's six possible sets of text styles just considering the defaults).

Hixie · 2018-10-05T23:50:38Z

packages/flutter/lib/src/material/material_localizations.dart

nit: urls can contain dots, and mean something different with them at the end than without.
a few workarounds:

three language categories defined in: https//example.com

three language categories defined in <https//example.com>.

[three language categories](https//example.com).

Good point. I've fixed up the naked URLs.

Hixie · 2018-10-05T23:53:27Z

packages/flutter/lib/src/material/text_theme.dart

i think this means "the spec now calls body1 and body2 paragraph1 and paragraph2"?

Since those aren't the only names that changed, might be easier just to say "This API was first created with an earlier version of the material design specifications that used different names. For this reason, the names above are not used by this API. The names used in this API correspond to the names used above in the manner described in the table below." or some such.

Here's what I came up with:

/// The [TextTheme] API is based on the original material (2014) /// design spec, which used different text style names. For backwards /// compatability's sake, this API continues to use the original /// names. The table below should help with understanding the API in /// terms of the 2018 material spec.

Hixie · 2018-10-05T23:54:20Z

packages/flutter/lib/src/material/text_theme.dart

have not changed from what? you never mention the earlier spec, just that the new spec "changed".

If you use the text I suggested above, you'll have to tweak this to read better too.

OK, I went with:

/// Each of the [TextTheme] text styles corresponds to one of the /// styles from 2018 spec. By default, the font sizes, font weights /// and letter spacings have not changed from their original, /// 2014, values.

Hixie · 2018-10-05T23:56:06Z

packages/flutter/lib/src/material/text_theme.dart

nit: indent is off (the display4 and display3 should line up)

Hixie · 2018-10-05T23:57:15Z

packages/flutter/lib/src/material/theme_data.dart

missing docs

Oops, done.

Hixie · 2018-10-05T23:57:25Z

packages/flutter/lib/src/material/theme_data.dart

commented out code

Sorry about that. Removed.

Hixie · 2018-10-05T23:58:09Z

packages/flutter/lib/src/material/typography.dart

Mountain View ?

Hixie · 2018-10-05T23:58:49Z

packages/flutter/lib/src/material/typography.dart

s/correspond the current spec/correspond to the second iteration of the specification/

Hixie · 2018-10-06T00:00:03Z

packages/flutter/lib/src/material/typography.dart

maybe have a constructor that does this automatically, like Typography.defaultsFor2018(platform)? (If you add this, also add ``Typography.defaultsFor2014`, for consistency.)

Hixie · 2018-10-06T00:00:13Z

packages/flutter/lib/src/material/typography.dart

Hixie · 2018-10-06T00:01:10Z

packages/flutter/lib/src/material/typography.dart

please be explicit about the platforms rather than using default

Hixie · 2018-10-06T00:01:39Z

packages/flutter/lib/src/material/typography.dart

provides, or should provide?

Good point: should provide.

Hixie · 2018-10-06T00:02:26Z

packages/flutter/lib/src/material/typography.dart

no need for the assert false here

Hixie · 2018-10-06T00:02:37Z

packages/flutter/lib/src/material/typography.dart

nit: switch isn't a method call, so there should be a space after the keyword

cc @a14n if you would like an easy lint to add... if(, while(, for(, switch( -- none of these are method calls, so all should have a space between the keyword and the expression to distinguish them visually from method calls.

Hixie · 2018-10-06T00:05:28Z

packages/flutter/lib/src/material/typography.dart

Interesting, we mostly use dynamic but a few use Object. We really should change them all to Object. Not in this PR, obviously...

Hixie · 2018-10-06T00:08:17Z

packages/flutter/lib/src/material/typography.dart

for consistency the 2014 ones should say 2014 in their debugLabels.

Hixie · 2018-10-06T00:09:23Z

modulo some minor comments

…flutter#22330)" This reverts commit 8bfb4b3.

For G3 Roll: * Revert "MaterialButton must honor its minWidth and height parameters (#22919)" This reverts commit a023323. * Revert "Update uses of ButtonTheme.bar: pass along the current Theme's colorScheme (#22827)" This reverts commit 655bf6a. * Revert "ButtonTheme.of().colorScheme defers to Theme (#22880)" This reverts commit a590940. * Revert "Bring TextTheme into alignment with the current Material spec (#22330)" This reverts commit 8bfb4b3. * Revert "Added ColorScheme, updated ThemeData, ButtonTheme, material buttons (#22013)" This reverts commit eea3465. * Manual adjustments to fix reversion issues.

googlebot added the cla: yes label Sep 27, 2018

HansMuller force-pushed the text_theme_update branch from 204cd59 to 69162b9 Compare September 27, 2018 23:28

Hixie reviewed Sep 28, 2018

View reviewed changes

HansMuller force-pushed the text_theme_update branch from 69162b9 to e74a924 Compare September 28, 2018 19:30

HansMuller force-pushed the text_theme_update branch 3 times, most recently from cb78f4d to 0d4aea1 Compare October 3, 2018 20:26

Hixie reviewed Oct 5, 2018

View reviewed changes

Hixie reviewed Oct 6, 2018

View reviewed changes

HansMuller added 12 commits October 8, 2018 11:56

Update TextTheme

58d00e7

update

7ec9203

updated

5b6665a

updated

6501819

updated

0e58881

updated

cc433ac

updated

97be16f

updated

8c9c1c0

updated

acd3280

updated

0aee13c

sync up flutter_localizations.dart

4124a19

updated per review feedback

9eb82e4

HansMuller force-pushed the text_theme_update branch from 9069887 to 9eb82e4 Compare October 8, 2018 20:20

HansMuller merged commit 8bfb4b3 into flutter:master Oct 10, 2018

HansMuller deleted the text_theme_update branch October 10, 2018 00:00

matthew-carroll pushed a commit to matthew-carroll/flutter that referenced this pull request Oct 11, 2018

Revert "Bring TextTheme into alignment with the current Material spec (…

2b0ea8d

…flutter#22330)" This reverts commit 8bfb4b3.

matthew-carroll mentioned this pull request Oct 11, 2018

Reversion for roll #22984

Merged

HansMuller mentioned this pull request Oct 15, 2018

TextTheme.apply() corrections #23108

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2021

Bring TextTheme into alignment with the current Material spec #22330

Bring TextTheme into alignment with the current Material spec #22330

Uh oh!

Conversation

HansMuller commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajsecord commented Sep 27, 2018

Uh oh!

Hixie commented Sep 27, 2018

Uh oh!

ajsecord commented Sep 27, 2018

Uh oh!

HansMuller commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hixie commented Sep 27, 2018

Uh oh!

Hixie commented Sep 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HansMuller Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajsecord commented Sep 28, 2018

Uh oh!

Hixie commented Sep 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HansMuller commented Sep 27, 2018 •

edited

Loading

HansMuller commented Sep 27, 2018 •

edited

Loading

HansMuller Sep 28, 2018 •

edited

Loading