From 7c6b0888926d2198b4df9e8230475a9c1af56a5f Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 24 Apr 2024 17:56:44 -0700 Subject: [PATCH] ui: Set letter spacing to 1% in buttons Fixes: #548 --- lib/widgets/app.dart | 3 +- lib/widgets/theme.dart | 39 ++++++++++++ test/flutter_checks.dart | 2 + test/widgets/theme_test.dart | 112 +++++++++++++++++++++++++++++++++++ 4 files changed, 155 insertions(+), 1 deletion(-) create mode 100644 test/widgets/theme_test.dart diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 46b7d33cf6..cbe7bdee35 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -128,7 +128,8 @@ class _ZulipAppState extends State 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/theme.dart b/lib/widgets/theme.dart index 8d4ab5ea51..87a688fda8 100644 --- a/lib/widgets/theme.dart +++ b/lib/widgets/theme.dart @@ -37,3 +37,42 @@ 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)), + ); +} + +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, + 0.01, baseFontSize: fontSize)))); +} diff --git a/test/flutter_checks.dart b/test/flutter_checks.dart index bc7e9d83a7..9410a13b4a 100644 --- a/test/flutter_checks.dart +++ b/test/flutter_checks.dart @@ -57,6 +57,7 @@ extension ValueNotifierChecks on Subject> { extension TextChecks on Subject { Subject get data => has((t) => t.data, 'data'); + Subject get style => has((t) => t.style, 'style'); } extension TextFieldChecks on Subject { @@ -65,6 +66,7 @@ extension TextFieldChecks on Subject { extension TextStyleChecks on Subject { Subject get inherit => has((t) => t.inherit, 'inherit'); + Subject get fontSize => has((t) => t.fontSize, 'fontSize'); Subject get fontWeight => has((t) => t.fontWeight, 'fontWeight'); Subject get letterSpacing => has((t) => t.letterSpacing, 'letterSpacing'); Subject?> get fontVariations => has((t) => t.fontVariations, 'fontVariations'); diff --git a/test/widgets/theme_test.dart b/test/widgets/theme_test.dart new file mode 100644 index 0000000000..3889135df3 --- /dev/null +++ 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 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(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))); + }); + }); +}