text: Apply "Source Sans 3" and (on Android) "Noto Color Emoji" widely across the app #439

chrisbobbe · 2023-12-06T02:46:38Z

(As usual please pardon the battery-life indicator in some of these screenshots. Ugh.)

In the second set of screenshots, note that the 🤯 emoji is made to look the same in the message list and the reaction chip. (This is taken on the office Android device.)

Before	After

Fixes: #294
Fixes: #438

chrisbobbe · 2023-12-06T02:47:50Z

I would probably write a test of zulipTypography to make sure it matches an expected value, which would be hard-coded in the test. I'll go ahead with that if the direction taken in this PR seems like one we want to pursue. 🙂

gnprice · 2023-12-06T21:50:49Z

Thanks!

I see there's a lot going on here, so I'm afraid I won't be able to get to it today before my vacation. But I'll be glad to take a look in a couple of weeks when I'm back 🙂

gnprice · 2024-01-05T02:23:39Z

Oops, I didn't previously get back to this PR in my catching-up after vacation. Looking now; just rebased it onto current main.

gnprice

Thanks! Glad to see the text styling get more centralized like this. Generally these changes all look good — various small comments below.

gnprice · 2024-01-05T02:30:18Z

lib/widgets/text.dart

+/// A [FontVariation] "wght" value that's 300 above a given, clamped to [kWghtMax].
+double bolderWght(double baseWght) {
+  assert(baseWght >= kWghtMin && baseWght <= kWghtMax);
+  return (baseWght + 300).clamp(kWghtMin, kWghtMax).toDouble();


Suggested change

return (baseWght + 300).clamp(kWghtMin, kWghtMax).toDouble();

return clampDouble(baseWght + 300, kWghtMin, kWghtMax);

(clampDouble is from flutter/foundation; see its doc.)

gnprice · 2024-01-05T02:31:48Z

lib/widgets/text.dart

+
+/// A [FontVariation] "wght" value that's 300 above a given, clamped to [kWghtMax].
+double bolderWght(double baseWght) {
+  assert(baseWght >= kWghtMin && baseWght <= kWghtMax);


nit: I find this sort of condition more readable when the comparisons are all <= or < so that everything's in order:

Suggested change

assert(baseWght >= kWghtMin && baseWght <= kWghtMax);

assert(kWghtMin <= baseWght && baseWght <= kWghtMax);

gnprice · 2024-01-05T02:33:04Z

test/widgets/text_test.dart

+    testWeights('default `wght`, specific `wghtIfPlatformRequestsBold`; platform requests bold',
+      styleBuilder: (context) => weightVariableTextStyle(context, wght: 775),


This test's description and code don't seem to match.

gnprice · 2024-01-05T02:44:57Z

lib/widgets/message_list.dart

-      fontFamily: 'Source Sans 3',
      fontSize: 16,
      letterSpacing: 0.02 * 16,
      height: (18 / 16),
-    ).merge(weightVariableTextStyle(context, wght: 600, wghtIfPlatformRequestsBold: 900));
+    ).merge(weightVariableTextStyle(context, wght: 600));


Happy to see these repetitive elements getting centralized!

I'd actually had the thought in the last couple of weeks that I should take a look at trying to deduplicate some of the recurring code around our text styles. This PR looks like great progress already in that direction.

gnprice · 2024-01-05T02:46:07Z

lib/widgets/text.dart

+/// Processes the "geometry" [TextTheme] fields
+/// ([Typography.dense], [Typography.englishLike], and [Typography.tall])
+/// with [weightVariableTextTheme], passing along the [BuildContext].
+Typography weightVariableTypography(BuildContext context, Typography input) =>


nit: Let's put all the Typography and TextTheme stuff (the flutter/material stuff) consecutively in this file.

At the top seems good, consistent with the main entry point being the thing at the top.

gnprice · 2024-01-05T03:38:35Z

lib/widgets/text.dart

+///
+/// This assumes that if a non-null field omits [TextStyle.fontWeight], then
+/// the normal font weight is implied, and it will be expressed in the output.
+TextTheme weightVariableTextTheme(BuildContext context, TextTheme input) {


Similarly:

Suggested change

TextTheme weightVariableTextTheme(BuildContext context, TextTheme input) {

TextTheme _weightVariableTextTheme(BuildContext context, TextTheme input) {

gnprice · 2024-01-05T03:58:48Z

lib/widgets/text.dart

+/// This looks at each of `input`'s fields, such as [TextTheme.bodyMedium], and:
+///
+/// If the field is non-null, calls [TextStyle.merge], passing the result of
+/// [weightVariableTextStyle] called with:
+/// - this function's [BuildContext] param, and
+/// - for `wght`:
+///   - if present, the field's [TextStyle.fontWeight]
+///     (via [wghtFromFontWeight]), else
+///   - null.
+///
+/// If the field is null, gives null.
+///
+/// This assumes that if a non-null field omits [TextStyle.fontWeight], then
+/// the normal font weight is implied, and it will be expressed in the output.


This is awfully close to restating the code, and it feels like more specific detail than one wants when trying to understand how to use the function.

Here's a version that tries to be a bit more abstract, and hopefully also easier to understand the point:

Suggested change

/// This looks at each of `input`'s fields, such as [TextTheme.bodyMedium], and:

///

/// If the field is non-null, calls [TextStyle.merge], passing the result of

/// [weightVariableTextStyle] called with:

/// - this function's [BuildContext] param, and

/// - for `wght`:

/// - if present, the field's [TextStyle.fontWeight]

/// (via [wghtFromFontWeight]), else

/// - null.

///

/// If the field is null, gives null.

///

/// This assumes that if a non-null field omits [TextStyle.fontWeight], then

/// the normal font weight is implied, and it will be expressed in the output.

/// This looks at each of the [TextStyle]s found on the input [TextTheme]

/// (such as [TextTheme.bodyMedium]),

/// and uses [weightVariableTextStyle] to adjust the [TextStyle].

/// Fields that are null in the input [TextTheme] remain null in the output.

///

/// For each input [TextStyle], the `wght` value passed

/// to [weightVariableTextStyle] is based on the input's [TextStyle.fontWeight].

/// A null [TextStyle.fontWeight] is interpreted as the normal font weight.

Great; thanks for the suggestion!

gnprice · 2024-01-05T04:07:06Z

lib/widgets/content.dart

  static TextStyle getTextStyle(BuildContext context) => const TextStyle(
-    fontFamily: 'Source Sans 3',
-    fontSize: 14,
-    height: (17 / 14),
-  ).merge(weightVariableTextStyle(context));
+    fontSize: kBaseFontSize,
+    height: (17 / kBaseFontSize),
+  );


For a further nice cleanup (probably in a further cleanup commit at the end):

static const textStyle = TextStyle( fontSize: kBaseFontSize, height: (17 / kBaseFontSize));

Since we're no longer looking at context, this can become a constant.

Ah indeed; I meant to do this as a followup here but forgot.

gnprice · 2024-01-05T04:14:42Z

lib/widgets/message_list.dart

+              textStyle:
+                // Restate [FilledButton]'s default, which inherits from
+                // [zulipTypography]…
+                Theme.of(context).textTheme.labelLarge!
+                // …then clobber some attributes to follow Figma:
+                .merge(const TextStyle(
+                  fontSize: 18,
+                  height: (23 / 18))
+                .merge(weightVariableTextStyle(context, wght: 400))),


Hmm interesting. I guess this will be a case to consider when thinking about how we ultimately want to arrange the data flow for the text styles.

gnprice · 2024-01-05T04:27:10Z

lib/widgets/text.dart

+final kDefaultFontFamilyFallback = [
+  // iOS doesn't support any of the formats this font is available in.
+  // If we use it on iOS, we'll get blank spaces where we could have had Apple-
+  // style emojis.
+  if (defaultTargetPlatform == TargetPlatform.android) 'Noto Color Emoji',


This is a bit of a perilous pattern because defaultTargetPlatform can change over time. Not likely to happen in a running app — but it happens all the time in tests, which indeed is the point of defaultTargetPlatform.

So as a static final, kDefaultFontFamilyFallback will get computed once when it's first accessed, and subsequent accesses will get the same value even if defaultTargetPlatform has changed. If we have a test that cares about the value, then the test could pass or fail depending on which other tests ran first. That in turn can be a big pain for debugging why some test is behaving the way it is (e.g. for figuring out why some other change broke a test).

I think a good fix would be to make it a getter, just like defaultTargetPlatform itself:

List<String> get defaultFontFamilyFallback => [

There's no performance need to memoize it, because at the tip of the PR we're only calling this twice per build of ZulipApp.

chrisbobbe · 2024-01-05T22:59:23Z

Thanks for the review! Revision pushed.

chrisbobbe · 2024-01-05T23:02:26Z

Oh, and what do you think of #439 (comment) :

I would probably write a test of zulipTypography to make sure it matches an expected value, which would be hard-coded in the test. I'll go ahead with that if the direction taken in this PR seems like one we want to pursue. 🙂

? I haven't done that yet but could. A Typography with all its details given literally will be very verbose.

chrisbobbe · 2024-01-09T18:51:21Z

Just pushed a revision with a test that spot-checks zulipTypography; PTAL. 🙂

gnprice

Thanks for the revision! Sorry for the belated review. This all looks good, with a few nits.

I think there'll also be a few spots to update when you rebase atop the package:checks upgrade you're doing right now.

gnprice · 2024-02-01T20:41:00Z

lib/widgets/text.dart

+      if (wghtIfPlatformRequestsBold != null) 'wghtIfPlatformRequestsBold: $wghtIfPlatformRequestsBold',
+    ];
+    result = result.copyWith(
+      debugLabel: 'weightVariableTextStyle(${attributes.join(', ')})');


nit / good news:

This won't actually take effect until an upstream issue is fixed: https://github.com/flutter/flutter/issues/141140

That issue has been fixed! (Your upstream PR for it got merged the next day.)

gnprice · 2024-02-01T20:41:59Z

lib/widgets/text.dart

+const kDefaultFontFamily = 'Source Sans 3';
+
+/// The [TextStyle.fontFamilyFallback] for use with [kDefaultFontFamily].
+List<String> get defaultFontFamilyFallback => [


nit: commit message has a reference to a previous revision's name for this:

If there's some corner of the app where the two fonts aren't getting applied, we'll eventually find it, and apply kDefaultFontFamily and kDefaultFontFamilyFallback according to their doc comments, to fix.

nit: this now refers to a defaultFontFamily, which doesn't exist 🙂 :

If there's some corner of the app where the two fonts aren't getting applied, we'll eventually find it, and apply defaultFontFamily and defaultFontFamilyFallback according to their doc comments, to fix.

gnprice · 2024-02-01T23:47:04Z

test/widgets/text_test.dart

+      ..fontFamilyFallback.isNotNull().deepEquals(defaultFontFamilyFallback);
+
+    matchesWeight(FontWeight weight) => it<TextStyle>()
+      ..fontWeight.isNotNull().equals(weight)


nit:

Suggested change

..fontWeight.isNotNull().equals(weight)

..fontWeight.equals(weight)

gnprice · 2024-02-01T23:48:45Z

test/widgets/text_test.dart

+    final matchesFonts = it<TextStyle>()
+      ..fontFamily.equals(kDefaultFontFamily)
+      ..fontFamilyFallback.isNotNull().deepEquals(defaultFontFamilyFallback);


nit:

Suggested change

final matchesFonts = it<TextStyle>()

..fontFamily.equals(kDefaultFontFamily)

..fontFamilyFallback.isNotNull().deepEquals(defaultFontFamilyFallback);

final matchesFontFamilies = it<TextStyle>()

..fontFamily.equals(kDefaultFontFamily)

..fontFamilyFallback.isNotNull().deepEquals(defaultFontFamilyFallback);

since it's not concerned with the font weight, etc., only the font families

gnprice · 2024-02-01T23:51:45Z

test/widgets/text_test.dart

+    check(const TextTheme().toDiagnosticsNode().getProperties().map((n) => n.name).toList())
+      .deepEquals([
+        'displayLarge',  'displayMedium',  'displaySmall',


nit: can make this a bit more robust to possible upstream changes that we shouldn't need to care about:

Suggested change

check(const TextTheme().toDiagnosticsNode().getProperties().map((n) => n.name).toList())

.deepEquals([

'displayLarge', 'displayMedium', 'displaySmall',

check(const TextTheme().toDiagnosticsNode().getProperties().map((n) => n.name).toList())

.unorderedEquals([

'displayLarge', 'displayMedium', 'displaySmall',

In zulip#436, we brought back Noto Color Emoji, but just for reaction chips. This brings it to message content in the message list, and all the other places where we've been specifying Source Sans 3. This leaves some edge cases where we'll still fall back to a system emoji font, like if there's an emoji in a user's name and you're looking at the profile screen. (Or in the "sender" area of a message in the message list, for that matter, checking again just now.) Those will be fixed later in this series, along with a cleanup that eliminates the repetition of [kDefaultFontFamily] and [kDefaultFontFamilyFallback] that we see in this commit. Fixes-partly: zulip#438

…tyle Now this should only call for work when the bold text setting is changed, and not when other parts of [MediaQueryData] change.

While we're at it, make them doubles.

…Bold In particular, remove this assert: assert((wght != null) == (wghtIfPlatformRequestsBold != null)); and default the effective `wghtIfPlatformRequestsBold`, when absent, to a consistent formula that gives the same result for all the callers so far. (Including callers that haven't been passing either `wght` or `wghtIfPlatformRequestsBold`: those have been getting [FontWeight.normal.value] / [FontWeight.bold.value], which is 400 / 700; and our new helper gives 700 for 400.)

…tyle This will take effect thanks to my PR for an upstream issue: flutter/flutter#141140 flutter/flutter#141141

chrisbobbe · 2024-02-02T00:44:37Z

Thanks for the review! Revision pushed, atop my latest revision for that checks upgrade, #502.

gnprice

Thanks for the revision! One fresh nit below — then please go ahead and merge.

gnprice · 2024-02-02T01:18:23Z

lib/widgets/text.dart

+const kDefaultFontFamily = 'Source Sans 3';
+
+/// The [TextStyle.fontFamilyFallback] for use with [kDefaultFontFamily].
+List<String> get defaultFontFamilyFallback => [


nit: this now refers to a defaultFontFamily, which doesn't exist 🙂 :

If there's some corner of the app where the two fonts aren't getting applied, we'll eventually find it, and apply defaultFontFamily and defaultFontFamilyFallback according to their doc comments, to fix.

With plumbing to make kDefaultFamily work, since it's a variable-weight font. As it happens, most -- perhaps all -- of the app's text is built on Typography styles. See the next commit for some thoughts on that. Since that's the case, I'll mark this commit as fixing these issues: - zulip#294 Use "Source Sans 3" font for most UI text - zulip#438 Consistently use "Noto Color Emoji" for emojis on Android If there's some corner of the app where the two fonts aren't getting applied, we'll eventually find it, and apply kDefaultFontFamily and defaultFontFamilyFallback according to their doc comments, to fix. Fixes: zulip#294 Fixes: zulip#438

chrisbobbe · 2024-02-02T01:29:26Z

Thanks! Merged, with that fix.

chrisbobbe requested a review from gnprice December 6, 2023 02:46

gnprice requested a review from sirpengi December 6, 2023 21:51

gnprice force-pushed the pr-fonts-all-over-app branch from 60ec0d5 to f9985e4 Compare January 5, 2024 02:22

gnprice reviewed Jan 5, 2024

View reviewed changes

chrisbobbe force-pushed the pr-fonts-all-over-app branch from f9985e4 to 8801f85 Compare January 5, 2024 22:58

chrisbobbe force-pushed the pr-fonts-all-over-app branch from 8801f85 to 4b5902e Compare January 9, 2024 18:50

chrisbobbe mentioned this pull request Feb 1, 2024

content: Fix **bold code** rendering with regular weight #501

Merged

gnprice reviewed Feb 1, 2024

View reviewed changes

chrisbobbe added 8 commits February 1, 2024 16:38

text [nfc]: Pull out kDefaultFontFamily constant

117670a

content [nfc]: Use kBaseFontSize in a place we were using literal value

f47ce05

text: Use more efficient MediaQuery.boldTextOf in weightVariableTextS…

cdc2688

…tyle Now this should only call for work when the bold text setting is changed, and not when other parts of [MediaQueryData] change.

text [nfc]: Take out helper constants kWght{Min,Max}

806a3c3

While we're at it, make them doubles.

text test [nfc]: Reorder some lines to be like surrounding code

bff2071

text [nfc]: In dev, add debugLabel to result of weightVariableTextS…

8567fdb

…tyle This will take effect thanks to my PR for an upstream issue: flutter/flutter#141140 flutter/flutter#141141

chrisbobbe force-pushed the pr-fonts-all-over-app branch from 4b5902e to 9553f9f Compare February 2, 2024 00:44

gnprice reviewed Feb 2, 2024

View reviewed changes

chrisbobbe added 3 commits February 1, 2024 17:23

text [nfc]: Remove some now-redundant fontFamily{,Fallback} attributes

6784ef9

content [nfc]: Make paragraph text style const

97afdcd

chrisbobbe force-pushed the pr-fonts-all-over-app branch from 9553f9f to 97afdcd Compare February 2, 2024 01:28

chrisbobbe merged commit 97afdcd into zulip:main Feb 2, 2024

chrisbobbe deleted the pr-fonts-all-over-app branch February 2, 2024 01:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

text: Apply "Source Sans 3" and (on Android) "Noto Color Emoji" widely across the app #439

text: Apply "Source Sans 3" and (on Android) "Noto Color Emoji" widely across the app #439

chrisbobbe commented Dec 6, 2023 •

edited

Loading

chrisbobbe commented Dec 6, 2023

gnprice commented Dec 6, 2023

gnprice commented Jan 5, 2024

gnprice left a comment

gnprice Jan 5, 2024

gnprice Jan 5, 2024

gnprice Jan 5, 2024

gnprice Jan 5, 2024

gnprice Jan 5, 2024

gnprice Jan 5, 2024

gnprice Jan 5, 2024

chrisbobbe Jan 5, 2024

gnprice Jan 5, 2024

chrisbobbe Jan 5, 2024

gnprice Jan 5, 2024

gnprice Jan 5, 2024

chrisbobbe commented Jan 5, 2024

chrisbobbe commented Jan 5, 2024

chrisbobbe commented Jan 9, 2024

gnprice left a comment

gnprice Feb 1, 2024

gnprice Feb 1, 2024

gnprice Feb 2, 2024

gnprice Feb 1, 2024

gnprice Feb 1, 2024

gnprice Feb 1, 2024

chrisbobbe commented Feb 2, 2024

gnprice left a comment

gnprice Feb 2, 2024

chrisbobbe commented Feb 2, 2024

	return (baseWght + 300).clamp(kWghtMin, kWghtMax).toDouble();
	return clampDouble(baseWght + 300, kWghtMin, kWghtMax);

	assert(baseWght >= kWghtMin && baseWght <= kWghtMax);
	assert(kWghtMin <= baseWght && baseWght <= kWghtMax);

		testWeights('default `wght`, specific `wghtIfPlatformRequestsBold`; platform requests bold',
		styleBuilder: (context) => weightVariableTextStyle(context, wght: 775),

	TextTheme weightVariableTextTheme(BuildContext context, TextTheme input) {
	TextTheme _weightVariableTextTheme(BuildContext context, TextTheme input) {

	..fontWeight.isNotNull().equals(weight)
	..fontWeight.equals(weight)

text: Apply "Source Sans 3" and (on Android) "Noto Color Emoji" widely across the app #439

text: Apply "Source Sans 3" and (on Android) "Noto Color Emoji" widely across the app #439

Conversation

chrisbobbe commented Dec 6, 2023 • edited Loading

chrisbobbe commented Dec 6, 2023

gnprice commented Dec 6, 2023

gnprice commented Jan 5, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jan 5, 2024

chrisbobbe commented Jan 5, 2024

chrisbobbe commented Jan 9, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Feb 2, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Feb 2, 2024

chrisbobbe commented Dec 6, 2023 •

edited

Loading