ui: Set letter spacing to 1% in buttons

chrisbobbe · chrisbobbe · commit f7669ead1a1a · 2024-04-25T16:31:20.000-07:00
It's a bit annoying that we have to repeat all of elevatedButtonTheme filledButtonTheme menuButtonTheme outlinedButtonTheme segmentedButtonTheme textButtonTheme but anyway, here we are; I don't think I've left any out. [ThemeData.buttonTheme] *sounds* like it might be one central place to define button text's letter spacing...but on a closer look, it doesn't allow setting letter spacing, and moreover it's deprecated, with these various FooButtonThemes meant to replace it. Empirically, the mark-all-as-read button (a [FilledButton.icon]) ignores the TextStyle in the ambient FilledButtonThemeData unless explicitly told to use it; that's because we use the `textStyle` param. So, explicitly merge that in, and adjust some tests (action_sheet_test, autocomplete_test, message_list_test) that weren't providing the FilledButtonThemeData so that they do provide it. ...hmm, but for that specific button, the Figma asks for a specific font size, which differs from the default `labelLarge`. That means we need to set `letterSpacing` right alongside `fontSize`, because the letter spacing should be proportional to the font size. Having done so, the restatement of the TextStyle in FilledButtonThemeData is now completely redundant. Shrug...I guess... if we add to those theme styles, we'll want them to be merged in, as done here, and that seems really easy to forget to do. Fixes: #548
diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart
@@ -128,7 +128,8 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
                 (_) => widget._declareReady());
             }
             GlobalLocalizations.zulipLocalizations = ZulipLocalizations.of(context);
-            return child!;
+            return Theme(data: innerThemeData(context),
+              child: child!);
           },
 
           // We use onGenerateInitialRoutes for the real work of specifying the
diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart
@@ -22,6 +22,7 @@ import 'profile.dart';
 import 'sticky_header.dart';
 import 'store.dart';
 import 'text.dart';
+import 'theme.dart';
 
 class MessageListPage extends StatefulWidget {
   const MessageListPage({super.key, required this.narrow});
@@ -458,6 +459,11 @@ class MarkAsReadWidget extends StatelessWidget {
 
   @override
   Widget build(BuildContext context) {
+    final themeData = Theme.of(context);
+    assert(themeData.filledButtonTheme.style?.textStyle != null);
+    final filledButtonThemeTextStyle =
+      themeData.filledButtonTheme.style?.textStyle!.resolve({});
+    assert(filledButtonThemeTextStyle != null);
     final zulipLocalizations = ZulipLocalizations.of(context);
     final store = PerAccountStoreWidget.of(context);
     final unreadCount = store.unreads.countInNarrow(narrow);
@@ -480,12 +486,13 @@ class MarkAsReadWidget extends StatelessWidget {
                 backgroundColor: _UnreadMarker.color,
                 minimumSize: const Size.fromHeight(38),
                 textStyle:
-                  // Restate [FilledButton]'s default, which inherits from
-                  // [zulipTypography]…
-                  Theme.of(context).textTheme.labelLarge!
+                  // Restate [FilledButton]'s default…
+                  themeData.textTheme.labelLarge!.merge(filledButtonThemeTextStyle)
                   // …then clobber some attributes to follow Figma:
-                  .merge(const TextStyle(
+                  .merge(TextStyle(
                     fontSize: 18,
+                    letterSpacing: proportionalLetterSpacing(context,
+                      kButtonTextLetterSpacingProportion, baseFontSize: 18),
                     height: (23 / 18))
                   .merge(weightVariableTextStyle(context, wght: 400))),
                 shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(7)),
diff --git a/lib/widgets/theme.dart b/lib/widgets/theme.dart
@@ -37,3 +37,44 @@ ThemeData outerThemeData(BuildContext context) {
 /// This is chosen as the sRGB midpoint of the Zulip logo's gradient.
 // As computed by Anders: https://github.com/zulip/zulip-mobile/pull/4467
 const kZulipBrandColor = Color.fromRGBO(0x64, 0x92, 0xfe, 1);
+
+/// A [ThemeData] that depends on [outerThemeData] and copies it with additions.
+///
+/// [context] must be below a [Theme] that provides [outerThemeData],
+/// so that this can use [Theme.of] to synthesize a [ThemeData.textTheme]
+/// based on [outerThemeData]'s [ThemeData.typography].
+// TODO(upstream) nicer if this could return a [ThemeData] with just the added
+//   fields, instead of having to call `copyWith` on `Theme.of(context)`.
+//   Then callers could call a hypothetical ThemeData.merge method with it.
+//   But such a method doesn't exist yet:
+//     https://github.com/flutter/flutter/issues/43823
+ThemeData innerThemeData(BuildContext context) {
+  final theme = Theme.of(context);
+  assert(theme.textTheme.labelLarge!.debugLabel!.contains('zulipTypography'));
+  return theme.copyWith(
+    elevatedButtonTheme:  ElevatedButtonThemeData( style: _commonButtonStyle(context)),
+    filledButtonTheme:    FilledButtonThemeData(   style: _commonButtonStyle(context)),
+    iconButtonTheme:      IconButtonThemeData(     style: _commonButtonStyle(context)),
+    menuButtonTheme:      MenuButtonThemeData(     style: _commonButtonStyle(context)),
+    outlinedButtonTheme:  OutlinedButtonThemeData( style: _commonButtonStyle(context)),
+    segmentedButtonTheme: SegmentedButtonThemeData(style: _commonButtonStyle(context)),
+    textButtonTheme:      TextButtonThemeData(     style: _commonButtonStyle(context)),
+  );
+}
+
+double kButtonTextLetterSpacingProportion = 0.01;
+
+ButtonStyle _commonButtonStyle(BuildContext context) {
+  // labelLarge is the default for all kinds of buttons that can have text. See:
+  //   [ElevatedButton.defaultStyleOf], [FilledButton.defaultStyleOf],
+  //   [MenuItemButton.defaultStyleOf], [MenuItemButton.defaultStyleOf],
+  //   [OutlinedButton.defaultStyleOf], `defaults` in [SegmentedButton.build],
+  //   and [TextButton.defaultStyleOf].
+  final fontSize = Theme.of(context).textTheme.labelLarge!.fontSize!;
+  return ButtonStyle(
+    textStyle: WidgetStatePropertyAll(TextStyle(
+      // If changing `fontSize` or `letterSpacing`, change the other too.
+      fontSize: fontSize,
+      letterSpacing: proportionalLetterSpacing(context,
+        kButtonTextLetterSpacingProportion, baseFontSize: fontSize))));
+}
diff --git a/test/flutter_checks.dart b/test/flutter_checks.dart
@@ -57,6 +57,7 @@ extension ValueNotifierChecks<T> on Subject<ValueNotifier<T>> {
 
 extension TextChecks on Subject<Text> {
   Subject<String?> get data => has((t) => t.data, 'data');
+  Subject<TextStyle?> get style => has((t) => t.style, 'style');
 }
 
 extension TextFieldChecks on Subject<TextField> {
@@ -65,6 +66,7 @@ extension TextFieldChecks on Subject<TextField> {
 
 extension TextStyleChecks on Subject<TextStyle> {
   Subject<bool> get inherit => has((t) => t.inherit, 'inherit');
+  Subject<double?> get fontSize => has((t) => t.fontSize, 'fontSize');
   Subject<FontWeight?> get fontWeight => has((t) => t.fontWeight, 'fontWeight');
   Subject<double?> get letterSpacing => has((t) => t.letterSpacing, 'letterSpacing');
   Subject<List<FontVariation>?> get fontVariations => has((t) => t.fontVariations, 'fontVariations');
diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart
@@ -3,7 +3,6 @@ import 'dart:convert';
 import 'package:checks/checks.dart';
 import 'package:flutter/material.dart';
 import 'package:flutter/services.dart';
-import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';
 import 'package:flutter_test/flutter_test.dart';
 import 'package:http/http.dart' as http;
 import 'package:zulip/api/model/model.dart';
@@ -12,11 +11,11 @@ import 'package:zulip/model/compose.dart';
 import 'package:zulip/model/localizations.dart';
 import 'package:zulip/model/narrow.dart';
 import 'package:zulip/model/store.dart';
+import 'package:zulip/widgets/app.dart';
 import 'package:zulip/widgets/compose_box.dart';
 import 'package:zulip/widgets/content.dart';
 import 'package:zulip/widgets/icons.dart';
 import 'package:zulip/widgets/message_list.dart';
-import 'package:zulip/widgets/store.dart';
 import 'package:share_plus_platform_interface/method_channel/method_channel_share.dart';
 import '../api/fake_api.dart';
 
@@ -56,14 +55,12 @@ Future<void> setupToMessageActionSheet(WidgetTester tester, {
     messages: [message],
   ).toJson());
 
-  await tester.pumpWidget(
-    MaterialApp(
-      localizationsDelegates: ZulipLocalizations.localizationsDelegates,
-      supportedLocales: ZulipLocalizations.supportedLocales,
-      home: GlobalStoreWidget(
-        child: PerAccountStoreWidget(
-          accountId: eg.selfAccount.id,
-          child: MessageListPage(narrow: narrow)))));
+
+  await tester.pumpWidget(const ZulipApp());
+  await tester.pump();
+  final navigator = await ZulipApp.navigator;
+  navigator.push(MessageListPage.buildRoute(accountId: eg.selfAccount.id,
+    narrow: narrow));
 
   // global store, per-account store, and message list get loaded
   await tester.pumpAndSettle();
diff --git a/test/widgets/autocomplete_test.dart b/test/widgets/autocomplete_test.dart
@@ -1,14 +1,13 @@
 import 'package:checks/checks.dart';
 import 'package:flutter/material.dart';
-import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';
 import 'package:flutter_test/flutter_test.dart';
 import 'package:zulip/api/model/model.dart';
 import 'package:zulip/api/route/messages.dart';
 import 'package:zulip/model/compose.dart';
 import 'package:zulip/model/narrow.dart';
 import 'package:zulip/model/store.dart';
+import 'package:zulip/widgets/app.dart';
 import 'package:zulip/widgets/message_list.dart';
-import 'package:zulip/widgets/store.dart';
 
 import '../api/fake_api.dart';
 import '../example_data.dart' as eg;
@@ -48,18 +47,14 @@ Future<Finder> setupToComposeInput(WidgetTester tester, {
 
   prepareBoringImageHttpClient();
 
-  await tester.pumpWidget(
-    MaterialApp(
-      localizationsDelegates: ZulipLocalizations.localizationsDelegates,
-      supportedLocales: ZulipLocalizations.supportedLocales,
-      home: GlobalStoreWidget(
-        child: PerAccountStoreWidget(
-          accountId: eg.selfAccount.id,
-          child: MessageListPage(
-            narrow: DmNarrow(
-              allRecipientIds: [eg.selfUser.userId, eg.otherUser.userId],
-              selfUserId: eg.selfUser.userId,
-            ))))));
+  await tester.pumpWidget(const ZulipApp());
+  await tester.pump();
+  final navigator = await ZulipApp.navigator;
+  navigator.push(MessageListPage.buildRoute(accountId: eg.selfAccount.id,
+    narrow: DmNarrow(
+      allRecipientIds: [eg.selfUser.userId, eg.otherUser.userId],
+      selfUserId: eg.selfUser.userId,
+    )));
 
   // global store, per-account store, and message list get loaded
   await tester.pumpAndSettle();
diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart
@@ -2,7 +2,6 @@ import 'dart:convert';
 
 import 'package:checks/checks.dart';
 import 'package:flutter/material.dart';
-import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';
 import 'package:flutter_test/flutter_test.dart';
 import 'package:http/http.dart' as http;
 import 'package:zulip/api/model/events.dart';
@@ -13,10 +12,10 @@ import 'package:zulip/api/route/messages.dart';
 import 'package:zulip/model/localizations.dart';
 import 'package:zulip/model/narrow.dart';
 import 'package:zulip/model/store.dart';
+import 'package:zulip/widgets/app.dart';
 import 'package:zulip/widgets/content.dart';
 import 'package:zulip/widgets/icons.dart';
 import 'package:zulip/widgets/message_list.dart';
-import 'package:zulip/widgets/store.dart';
 
 import '../api/fake_api.dart';
 import '../example_data.dart' as eg;
@@ -64,14 +63,11 @@ void main() {
     connection.prepare(json:
       newestResult(foundOldest: foundOldest, messages: messages).toJson());
 
-    await tester.pumpWidget(
-      MaterialApp(
-        localizationsDelegates: ZulipLocalizations.localizationsDelegates,
-        supportedLocales: ZulipLocalizations.supportedLocales,
-        home: GlobalStoreWidget(
-          child: PerAccountStoreWidget(
-            accountId: eg.selfAccount.id,
-            child: MessageListPage(narrow: narrow)))));
+    await tester.pumpWidget(const ZulipApp());
+    await tester.pump();
+    final navigator = await ZulipApp.navigator;
+    navigator.push(MessageListPage.buildRoute(accountId: eg.selfAccount.id,
+      narrow: narrow));
 
     // global store, per-account store, and message list get loaded
     await tester.pumpAndSettle();
diff --git a/test/widgets/theme_test.dart b/test/widgets/theme_test.dart
@@ -0,0 +1,112 @@
+import 'package:checks/checks.dart';
+import 'package:flutter/material.dart';
+import 'package:flutter/rendering.dart';
+import 'package:flutter_test/flutter_test.dart';
+import 'package:zulip/widgets/text.dart';
+import 'package:zulip/widgets/theme.dart';
+
+import '../flutter_checks.dart';
+
+void main() {
+  group('innerThemeData', () {
+    testWidgets('smoke', (tester) async {
+      ThemeData? outerThemeDataValue;
+      ThemeData? value;
+      await tester.pumpWidget(
+        Builder(builder: (context) => MaterialApp(
+          theme: outerThemeData(context),
+          home: Builder(builder: (context) {
+            outerThemeDataValue = Theme.of(context);
+            return Theme(
+              data: innerThemeData(context),
+              child: Builder(builder: (context) {
+                value = Theme.of(context);
+                return const Placeholder();
+              }));
+          }))));
+
+      final outerElevatedButtonLetterSpacing = outerThemeDataValue
+        !.elevatedButtonTheme.style?.textStyle?.resolve({})?.letterSpacing;
+      final elevatedButtonLetterSpacing = value
+        !.elevatedButtonTheme.style?.textStyle?.resolve({})?.letterSpacing;
+      check(outerElevatedButtonLetterSpacing).isNull();
+      check(elevatedButtonLetterSpacing).isNotNull();
+
+      // innerThemeData should extend outerThemeData using [ThemeData.copyWith]
+      // (at least for now, lacking a ThemeData.merge method). Pick a value that
+      // we set in outerThemeData and check that it hasn't been dropped.
+      check(value!.scaffoldBackgroundColor)
+        ..equals(outerThemeDataValue!.scaffoldBackgroundColor)
+        ..equals(const Color(0xfff6f6f6));
+    });
+
+    group('button text size and letter spacing', () {
+      Future<void> testButtonLetterSpacing(
+        String description, {
+        required Widget Function(BuildContext context, String text) buttonBuilder,
+        double? ambientTextScaleFactor,
+      }) async {
+        testWidgets(description, (WidgetTester tester) async {
+          if (ambientTextScaleFactor != null) {
+            tester.platformDispatcher.textScaleFactorTestValue = ambientTextScaleFactor;
+            addTearDown(tester.platformDispatcher.clearTextScaleFactorTestValue);
+          }
+          const buttonText = 'Zulip';
+          double? expectedFontSize;
+          double? expectedLetterSpacing;
+          await tester.pumpWidget(
+            Builder(builder: (context) => MaterialApp(
+              theme: outerThemeData(context),
+              home: Builder(builder: (context) {
+                expectedFontSize = Theme.of(context).textTheme.labelLarge!.fontSize!;
+                expectedLetterSpacing = proportionalLetterSpacing(context,
+                  0.01, baseFontSize: expectedFontSize!);
+                return Theme(data: innerThemeData(context),
+                  child: Builder(builder: (context) =>
+                    buttonBuilder(context, buttonText)));
+              }))));
+
+          final text = tester.renderObject<RenderParagraph>(find.text(buttonText)).text;
+          check(text.style!)
+            ..fontSize.equals(expectedFontSize)
+            ..letterSpacing.equals(expectedLetterSpacing);
+        });
+      }
+
+      testButtonLetterSpacing('with device text size adjusted',
+        ambientTextScaleFactor: 2.0,
+        buttonBuilder: (context, text) => ElevatedButton(onPressed: () {},
+          child: Text(text)));
+
+      testButtonLetterSpacing('ElevatedButton',
+        buttonBuilder: (context, text) => ElevatedButton(onPressed: () {},
+          child: Text(text)));
+
+      testButtonLetterSpacing('FilledButton',
+        buttonBuilder: (context, text) => FilledButton(onPressed: () {},
+          child: Text(text)));
+
+      // IconButton can't have text; skip
+
+      testButtonLetterSpacing('MenuItemButton',
+        buttonBuilder: (context, text) => MenuItemButton(onPressed: () {},
+          child: Text(text)));
+
+      testButtonLetterSpacing('SubmenuButton',
+        buttonBuilder: (context, text) => SubmenuButton(menuChildren: const [],
+          child: Text(text)));
+
+      testButtonLetterSpacing('OutlinedButton',
+        buttonBuilder: (context, text) => OutlinedButton(onPressed: () {},
+          child: Text(text)));
+
+      testButtonLetterSpacing('SegmentedButton',
+        buttonBuilder: (context, text) => SegmentedButton(selected: const {1},
+          segments: [ButtonSegment(value: 1, label: Text(text))]));
+
+      testButtonLetterSpacing('TextButton',
+        buttonBuilder: (context, text) => TextButton(onPressed: () {},
+          child: Text(text)));
+    });
+  });
+}

Original file line number	Diff line number	Diff line change
`@@ -128,7 +128,8 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {`
`128`	`128`	`(_) => widget._declareReady());`
`129`	`129`	`}`
`130`	`130`	`GlobalLocalizations.zulipLocalizations = ZulipLocalizations.of(context);`
`131`		`- return child!;`
	`131`	`+ return Theme(data: innerThemeData(context),`
	`132`	`+ child: child!);`
`132`	`133`	`},`
`133`	`134`
`134`	`135`	`// We use onGenerateInitialRoutes for the real work of specifying the`