Skip to content

Commit d9ab49b

Browse files
committed
text: Add and use localizedTextBaseline
This seems like a handy function to have around; there were a handful of places that were wanting something like this. In b75908b, we aimed to make ContentTheme.textStylePlainParagraph a very direct, NFC translation of some Typography defaults, but we accidentally fell short a little bit: I had forgotten that we support Japanese, and that commit just hardcoded TextBaseline.alphabetic, which was a regression. Since the "geometry TextThemes" for M3 only differ in `textBaseline` [1], I believe that commit would have been a pure translation if we had used this new function. (We did follow up with 6284ed5, removing the textBaseline field altogether. But that put the style at odds with text basically everywhere else, which inherits a textBaseline from Typography.) I was hoping to see an improvement in how Japanese text is rendered after this change, but I wasn't able to; I didn't notice any change in appearance between TextBaseline.alphabetic and TextBaseline.ideographic (or a TextStyle that omits the field, in the case of ContentTheme.textStylePlainParagraph.) Anyway, if this is because of incomplete support for `ideographic`, at least we'll be well-prepared to take improvements as they arrive. Someone did post some screenshots with both values rendering identically: flutter/flutter#94680 [1] The Material 2 spec apparently called for more differences that were removed in Material 3; see the docs for Typography.dense2021 and tall2021.
1 parent 3196263 commit d9ab49b

File tree

5 files changed

+67
-7
lines changed

5 files changed

+67
-7
lines changed

lib/widgets/content.dart

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ class ContentTheme extends ThemeExtension<ContentTheme> {
3939
color: const HSLColor.fromAHSL(1, 0, 0, 0.15).toColor(),
4040
fontSize: kBaseFontSize,
4141
letterSpacing: 0,
42+
textBaseline: localizedTextBaseline(context),
4243
height: (22 / kBaseFontSize),
4344
leadingDistribution: TextLeadingDistribution.even,
4445
decoration: TextDecoration.none,
@@ -345,7 +346,7 @@ class ListItemWidget extends StatelessWidget {
345346
return Row(
346347
mainAxisAlignment: MainAxisAlignment.start,
347348
crossAxisAlignment: CrossAxisAlignment.baseline,
348-
textBaseline: TextBaseline.alphabetic,
349+
textBaseline: localizedTextBaseline(context),
349350
children: [
350351
SizedBox(
351352
width: 20, // TODO handle long numbers in <ol>, like https://github.com/zulip/zulip/pull/25063

lib/widgets/message_list.dart

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,10 @@ class MessageListAppBarTitle extends StatelessWidget {
120120

121121
final Narrow narrow;
122122

123-
Widget _buildStreamRow(ZulipStream? stream, String text) {
123+
Widget _buildStreamRow(BuildContext context, {
124+
ZulipStream? stream,
125+
required String text,
126+
}) {
124127
// A null [Icon.icon] makes a blank space.
125128
final icon = (stream != null) ? iconDataForStream(stream) : null;
126129
return Row(
@@ -129,7 +132,7 @@ class MessageListAppBarTitle extends StatelessWidget {
129132
// For screenshots of some experiments, see:
130133
// https://github.com/zulip/zulip-flutter/pull/219#discussion_r1281024746
131134
crossAxisAlignment: CrossAxisAlignment.baseline,
132-
textBaseline: TextBaseline.alphabetic,
135+
textBaseline: localizedTextBaseline(context),
133136
children: [
134137
Icon(size: 16, icon),
135138
const SizedBox(width: 8),
@@ -149,13 +152,13 @@ class MessageListAppBarTitle extends StatelessWidget {
149152
final store = PerAccountStoreWidget.of(context);
150153
final stream = store.streams[streamId];
151154
final streamName = stream?.name ?? '(unknown channel)';
152-
return _buildStreamRow(stream, streamName);
155+
return _buildStreamRow(context, stream: stream, text: streamName);
153156

154157
case TopicNarrow(:var streamId, :var topic):
155158
final store = PerAccountStoreWidget.of(context);
156159
final stream = store.streams[streamId];
157160
final streamName = stream?.name ?? '(unknown channel)';
158-
return _buildStreamRow(stream, "$streamName > $topic");
161+
return _buildStreamRow(context, stream: stream, text: "$streamName > $topic");
159162

160163
case DmNarrow(:var otherRecipientIds):
161164
final store = PerAccountStoreWidget.of(context);
@@ -899,7 +902,7 @@ class MessageWithPossibleSender extends StatelessWidget {
899902
senderRow = Row(
900903
mainAxisAlignment: MainAxisAlignment.spaceBetween,
901904
crossAxisAlignment: CrossAxisAlignment.baseline,
902-
textBaseline: TextBaseline.alphabetic,
905+
textBaseline: localizedTextBaseline(context),
903906
children: [
904907
Flexible(
905908
child: GestureDetector(

lib/widgets/profile.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ class _ProfileDataTable extends StatelessWidget {
195195

196196
items.add(Row(
197197
crossAxisAlignment: CrossAxisAlignment.baseline,
198-
textBaseline: TextBaseline.alphabetic,
198+
textBaseline: localizedTextBaseline(context),
199199
children: [
200200
SizedBox(width: 100,
201201
child: Text(style: _TextStyles.customProfileFieldLabel(context),

lib/widgets/text.dart

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -373,3 +373,24 @@ double proportionalLetterSpacing(
373373
final effectiveTextScaler = textScaler ?? MediaQuery.textScalerOf(context);
374374
return effectiveTextScaler.scale(baseFontSize) * proportion;
375375
}
376+
377+
/// The most suitable [TextBaseline] for the current language.
378+
///
379+
/// The result is chosen based on information from [MaterialLocalizations]
380+
/// about the language's script.
381+
/// If [MaterialLocalizations] doesn't have that information,
382+
/// gives [TextBaseline.alphabetic].
383+
// Adapted from [Theme.of], which localizes text styles according to the
384+
// locale's [ScriptCategory]. With M3 defaults, this just means varying
385+
// [TextStyle.textBaseline] the way we do here.
386+
TextBaseline localizedTextBaseline(BuildContext context) {
387+
final materialLocalizations =
388+
Localizations.of<MaterialLocalizations>(context, MaterialLocalizations);
389+
final scriptCategory = materialLocalizations?.scriptCategory
390+
?? ScriptCategory.englishLike;
391+
return switch (scriptCategory) {
392+
ScriptCategory.dense => TextBaseline.ideographic,
393+
ScriptCategory.englishLike => TextBaseline.alphabetic,
394+
ScriptCategory.tall => TextBaseline.alphabetic,
395+
};
396+
}

test/widgets/text_test.dart

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,12 @@ import 'package:checks/checks.dart';
22
import 'package:collection/collection.dart';
33
import 'package:flutter/material.dart';
44
import 'package:flutter_test/flutter_test.dart';
5+
import 'package:zulip/widgets/app.dart';
6+
import 'package:zulip/widgets/page.dart';
57
import 'package:zulip/widgets/text.dart';
68

79
import '../flutter_checks.dart';
10+
import '../model/binding.dart';
811

912
// From trying the options on an iPhone 13 Pro running iOS 16.6.1:
1013
const kTextScaleFactors = <double>[
@@ -15,6 +18,8 @@ const kTextScaleFactors = <double>[
1518
];
1619

1720
void main() {
21+
TestZulipBinding.ensureInitialized();
22+
1823
group('zulipTypography', () {
1924
Future<Typography> getZulipTypography(WidgetTester tester, {
2025
required bool platformRequestsBold,
@@ -382,4 +387,34 @@ void main() {
382387
expected: clampingTextScaler.scale(14) * 0.01);
383388
}
384389
});
390+
391+
group('localizedTextBaseline', () {
392+
Future<void> testLocalizedTextBaseline(Locale locale, TextBaseline expected) async {
393+
testWidgets('gives $expected for $locale', (tester) async {
394+
addTearDown(testBinding.reset);
395+
tester.platformDispatcher.localeTestValue = locale;
396+
tester.platformDispatcher.localesTestValue = [locale];
397+
addTearDown(tester.platformDispatcher.clearLocaleTestValue);
398+
addTearDown(tester.platformDispatcher.clearLocalesTestValue);
399+
400+
await tester.pumpWidget(const ZulipApp());
401+
await tester.pump();
402+
403+
final navigator = await ZulipApp.navigator;
404+
navigator.push(MaterialWidgetRoute(page: Builder(builder: (context) =>
405+
Text('123', style: TextStyle(textBaseline: localizedTextBaseline(context))))));
406+
await tester.pumpAndSettle();
407+
408+
final TextStyle? style = tester.widget<Text>(find.text('123')).style;
409+
final actualTextBaseline = style!.textBaseline!;
410+
check(actualTextBaseline).equals(expected);
411+
});
412+
}
413+
414+
testLocalizedTextBaseline(const Locale('en'), TextBaseline.alphabetic);
415+
testLocalizedTextBaseline(const Locale('ja'), TextBaseline.ideographic);
416+
417+
// "und" is a special language code meaning undefined; see [Locale]
418+
testLocalizedTextBaseline(const Locale('und'), TextBaseline.alphabetic);
419+
});
385420
}

0 commit comments

Comments
 (0)