From df381331ba6380b36bec675c13d1854107c16351 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 4 Dec 2024 14:53:37 -0800 Subject: [PATCH 1/8] text [nfc]: Fix a straggler reference to defaultFontFamilyCallback's old name --- lib/widgets/text.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/widgets/text.dart b/lib/widgets/text.dart index 13a1b7c390..629b6b658c 100644 --- a/lib/widgets/text.dart +++ b/lib/widgets/text.dart @@ -21,7 +21,7 @@ import 'package:flutter/material.dart'; /// For example, the base style for message content; /// see [ContentTheme.textStylePlainParagraph]. /// -/// Applies [kDefaultFontFamily] and [kDefaultFontFamilyFallback], +/// Applies [kDefaultFontFamily] and [defaultFontFamilyFallback], /// being faithful to the Material-default font weights /// by running them through [weightVariableTextStyle]. /// (That is needed because [kDefaultFontFamily] is a variable-weight font). From 380e64e560fe1708d808fc5395726cd1f94cb835 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 4 Dec 2024 15:00:57 -0800 Subject: [PATCH 2/8] text: Use exhaustive switch to decide on Noto Color Emoji use This way we're making an explicit choice on all platforms. This change is NFC on iOS and Android, our supported targets. --- lib/widgets/text.dart | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/lib/widgets/text.dart b/lib/widgets/text.dart index 629b6b658c..8ba34439a3 100644 --- a/lib/widgets/text.dart +++ b/lib/widgets/text.dart @@ -153,12 +153,21 @@ const kDefaultFontFamily = 'Source Sans 3'; /// The [TextStyle.fontFamilyFallback] for use with [kDefaultFontFamily]. List get defaultFontFamilyFallback => [ - // 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', + if (_useNotoEmoji) 'Noto Color Emoji', ]; +/// Whether to use the Noto Color Emoji font for showing emoji. +bool get _useNotoEmoji => switch (defaultTargetPlatform) { + // iOS doesn't support any of the formats Noto Color Emoji is available in. + // If we use it on iOS, we'll get blank spaces where we could have had + // Apple-style emojis. We presume the same is true of macOS. + TargetPlatform.iOS || TargetPlatform.macOS => false, + // The font certainly works on Android. + // We presume it works on the other platforms. + TargetPlatform.android || TargetPlatform.linux + || TargetPlatform.fuchsia || TargetPlatform.windows => true, +}; + /// A mergeable [TextStyle] with 'Source Code Pro' and platform-aware fallbacks. /// /// Callers should also call [weightVariableTextStyle] and merge that in too, From bd0aed41bb35b522a97d29c68d553f3b5b2e4660 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 4 Dec 2024 15:06:45 -0800 Subject: [PATCH 3/8] text [nfc]: Explicitly fall back to Apple Color Emoji This is NFC because on the affected platforms, the system will already fall back automatically to Apple Color Emoji when a character doesn't have a glyph found in any of the specified fonts but does in the Apple Color Emoji font. --- lib/widgets/text.dart | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/widgets/text.dart b/lib/widgets/text.dart index 8ba34439a3..e50ac40668 100644 --- a/lib/widgets/text.dart +++ b/lib/widgets/text.dart @@ -153,19 +153,24 @@ const kDefaultFontFamily = 'Source Sans 3'; /// The [TextStyle.fontFamilyFallback] for use with [kDefaultFontFamily]. List get defaultFontFamilyFallback => [ - if (_useNotoEmoji) 'Noto Color Emoji', + _useAppleEmoji ? 'Apple Color Emoji' : 'Noto Color Emoji', ]; -/// Whether to use the Noto Color Emoji font for showing emoji. -bool get _useNotoEmoji => switch (defaultTargetPlatform) { +/// Whether to use the Apple Color Emoji font for showing emoji. +/// +/// When false, we use Noto Color Emoji instead. +bool get _useAppleEmoji => switch (defaultTargetPlatform) { // iOS doesn't support any of the formats Noto Color Emoji is available in. // If we use it on iOS, we'll get blank spaces where we could have had // Apple-style emojis. We presume the same is true of macOS. - TargetPlatform.iOS || TargetPlatform.macOS => false, - // The font certainly works on Android. + // Conversely, both platforms provide Apple Color Emoji. So we use that. + TargetPlatform.iOS || TargetPlatform.macOS => true, + + // The Noto Color Emoji font works fine on Android. // We presume it works on the other platforms. + // Conversely Apple Color Emoji isn't an option on any of these. TargetPlatform.android || TargetPlatform.linux - || TargetPlatform.fuchsia || TargetPlatform.windows => true, + || TargetPlatform.fuchsia || TargetPlatform.windows => false, }; /// A mergeable [TextStyle] with 'Source Code Pro' and platform-aware fallbacks. From 3a26d8282e90ff1c0b90c08491a4cb4805fb08ef Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 5 Dec 2024 16:59:15 -0800 Subject: [PATCH 4/8] content test [nfc]: Try to clarify mergedStyleOfSubstring a bit The implementation is a bit gnarly, so it's important that the doc be clear. --- test/widgets/content_test.dart | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart index acb3b3bce1..3541acb58d 100644 --- a/test/widgets/content_test.dart +++ b/test/widgets/content_test.dart @@ -65,16 +65,17 @@ TextStyle? mergeSpanStylesOuterToInner( }); } -/// The "merged style" ([mergeSpanStylesOuterToInner]) of a nested span. -TextStyle? mergedStyleOfSubstring(InlineSpan rootSpan, Pattern substringPattern) { +/// The "merged style" ([mergeSpanStylesOuterToInner]) of a text span +/// whose whole text matches the given pattern, under the given root span. +TextStyle? mergedStyleOfSubstring(InlineSpan rootSpan, Pattern spanPattern) { return mergeSpanStylesOuterToInner(rootSpan, (span) { if (span is! TextSpan) return false; final text = span.text; if (text == null) return false; - return switch (substringPattern) { - String() => text == substringPattern, - _ => substringPattern.allMatches(text) + return switch (spanPattern) { + String() => text == spanPattern, + _ => spanPattern.allMatches(text) .any((match) => match.start == 0 && match.end == text.length), }; }); From 3c5915b1707bdd1af81c8b10da7f2ab3c60691d7 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 5 Dec 2024 17:08:09 -0800 Subject: [PATCH 5/8] content test [nfc]: Add mergedStyleOf helper, simplifying callers --- test/widgets/content_test.dart | 51 ++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart index 3541acb58d..fa19cb520b 100644 --- a/test/widgets/content_test.dart +++ b/test/widgets/content_test.dart @@ -67,6 +67,8 @@ TextStyle? mergeSpanStylesOuterToInner( /// The "merged style" ([mergeSpanStylesOuterToInner]) of a text span /// whose whole text matches the given pattern, under the given root span. +/// +/// See also [mergedStyleOf], which can be more convenient. TextStyle? mergedStyleOfSubstring(InlineSpan rootSpan, Pattern spanPattern) { return mergeSpanStylesOuterToInner(rootSpan, (span) { @@ -81,6 +83,23 @@ TextStyle? mergedStyleOfSubstring(InlineSpan rootSpan, Pattern spanPattern) { }); } +/// The "merged style" ([mergeSpanStylesOuterToInner]) of a text span +/// whose whole text matches the given pattern, somewhere in the tree. +/// +/// This finds the relevant [Text] widget by a search for [spanPattern]. +/// If [findAncestor] is non-null, the search will only consider descendants +/// of widgets matching [findAncestor]. +TextStyle? mergedStyleOf(WidgetTester tester, Pattern spanPattern, { + Finder? findAncestor, +}) { + var findTextWidget = find.textContaining(spanPattern); + if (findAncestor != null) { + findTextWidget = find.descendant(of: findAncestor, matching: findTextWidget); + } + final rootSpan = tester.renderObject(findTextWidget).text; + return mergedStyleOfSubstring(rootSpan, spanPattern); +} + /// A callback that finds some target subspan within the given span, /// and reports the target's font size. typedef TargetFontSizeFinder = double Function(InlineSpan rootSpan); @@ -486,18 +505,12 @@ void main() { testFontWeight('syntax highlighting: non-bold span', expectedWght: 400, content: plainContent(ContentExample.codeBlockHighlightedShort.html), - styleFinder: (tester) { - final root = tester.renderObject(find.textContaining('class')).text; - return mergedStyleOfSubstring(root, 'class')!; - }); + styleFinder: (tester) => mergedStyleOf(tester, 'class')!); testFontWeight('syntax highlighting: bold span', expectedWght: 700, content: plainContent(ContentExample.codeBlockHighlightedShort.html), - styleFinder: (tester) { - final root = tester.renderObject(find.textContaining('A')).text; - return mergedStyleOfSubstring(root, 'A')!; - }); + styleFinder: (tester) => mergedStyleOf(tester, 'A')!); }); testContentSmoke(ContentExample.mathBlock); @@ -550,8 +563,7 @@ void main() { testContentSmoke(ContentExample.strong); TextStyle findWordBold(WidgetTester tester) { - final root = tester.renderObject(find.textContaining('bold')).text; - return mergedStyleOfSubstring(root, 'bold')!; + return mergedStyleOf(tester, 'bold')!; } testFontWeight('in plain paragraph', @@ -906,11 +918,8 @@ void main() { find.descendant(of: find.byType(GlobalTime), matching: find.byIcon(ZulipIcons.clock))); - final textSpan = tester.renderObject( - find.descendant(of: find.byType(GlobalTime), - matching: find.textContaining(renderedTextRegexp) - )).text; - final textColor = mergedStyleOfSubstring(textSpan, renderedTextRegexp)!.color; + final textColor = mergedStyleOf(tester, + findAncestor: find.byType(GlobalTime), renderedTextRegexp)!.color; check(textColor).isNotNull(); check(icon).color.isNotNull().isSameColorAs(textColor!); @@ -939,11 +948,8 @@ void main() { testWidgets('text is scaled', (tester) async { await doCheck(tester, (widget) { - final textSpan = tester.renderObject( - find.descendant(of: find.byWidget(widget), - matching: find.textContaining(renderedTextRegexp) - )).text; - return mergedStyleOfSubstring(textSpan, renderedTextRegexp)!.fontSize!; + return mergedStyleOf(tester, findAncestor: find.byWidget(widget), + renderedTextRegexp)!.fontSize!; }); }); @@ -1096,10 +1102,7 @@ void main() { // | 1 | 2 | 3 | 4 | content: plainContent(ContentExample.tableWithSingleRow.html), expectedWght: 700, - styleFinder: (tester) { - final root = tester.renderObject(find.textContaining('a')).text; - return mergedStyleOfSubstring(root, 'a')!; - }); + styleFinder: (tester) => mergedStyleOf(tester, 'a')!); testWidgets('header row background color', (tester) async { await prepareContent(tester, plainContent(ContentExample.tableWithSingleRow.html)); From 19e83b54bc5a081244b5913fd5a8ae6173d560dc Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 5 Dec 2024 17:10:49 -0800 Subject: [PATCH 6/8] content test: Use mergedStyleOf for mentions, too This change isn't quite NFC: textStyleFromWidget will now match even if the Text widget has additional text in it beyond the targeted span. That seems fine, though: it's not likely to happen; and if for some reason it does, that's not really what this test is about. --- test/widgets/content_test.dart | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart index fa19cb520b..4fb5e01b61 100644 --- a/test/widgets/content_test.dart +++ b/test/widgets/content_test.dart @@ -683,11 +683,8 @@ void main() { } TextStyle textStyleFromWidget(WidgetTester tester, UserMention widget, String mentionText) { - final fullNameSpan = tester.renderObject( - find.descendant( - of: find.byWidget(widget), matching: find.text(mentionText)) - ).text; - return mergedStyleOfSubstring(fullNameSpan, mentionText)!; + return mergedStyleOf(tester, + findAncestor: find.byWidget(widget), mentionText)!; } testWidgets('maintains font-size ratio with surrounding text', (tester) async { From 9391867c64e46af0cacb8190561348c6379a1f6e Mon Sep 17 00:00:00 2001 From: Rajesh Malviya Date: Thu, 5 Dec 2024 01:39:31 +0530 Subject: [PATCH 7/8] emoji: Use "Apple Color Emoji" font on iOS/macOS for UnicodeEmojiWidget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some unicode characters, like U+2764 (❤) or U+00AE (®) can have glyphs in non-Emoji fonts, resulting in incorrect rendering of such characters, where we specifically want an emoji to be displayed. So, explicitly mention "Apple Color Emoji" to be the font used on iOS/macOS for displaying the unicode emoji. This resolves part of #1104, namely for reaction chips and autocomplete results. [greg: wrote test] Fixes-partly: #1104 --- lib/widgets/emoji.dart | 8 +++++--- test/widgets/emoji_reaction_test.dart | 18 ++++++++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/lib/widgets/emoji.dart b/lib/widgets/emoji.dart index d8af9827d7..dafeb7b6d8 100644 --- a/lib/widgets/emoji.dart +++ b/lib/widgets/emoji.dart @@ -50,8 +50,8 @@ class UnicodeEmojiWidget extends StatelessWidget { case TargetPlatform.iOS: case TargetPlatform.macOS: - // We expect the font "Apple Color Emoji" to be used. There are some - // surprises in how Flutter ends up rendering emojis in this font: + // We use the font "Apple Color Emoji". There are some surprises in how + // Flutter ends up rendering emojis in this font: // - With a font size of 17px, the emoji visually seems to be about 17px // square. (Unlike on Android, with Noto Color Emoji, where a 14.5px font // size gives an emoji that looks 17px square.) See: @@ -71,7 +71,9 @@ class UnicodeEmojiWidget extends StatelessWidget { SizedBox(height: boxSize, width: boxSize), PositionedDirectional(start: 0, child: Text( textScaler: textScaler, - style: TextStyle(fontSize: size), + style: TextStyle( + fontFamily: 'Apple Color Emoji', + fontSize: size), strutStyle: StrutStyle(fontSize: size, forceStrutHeight: true), emojiDisplay.emojiUnicode)), ]); diff --git a/test/widgets/emoji_reaction_test.dart b/test/widgets/emoji_reaction_test.dart index eafe7a6795..85b41f4002 100644 --- a/test/widgets/emoji_reaction_test.dart +++ b/test/widgets/emoji_reaction_test.dart @@ -18,6 +18,7 @@ import '../flutter_checks.dart'; import '../model/binding.dart'; import '../model/test_store.dart'; import '../test_images.dart'; +import 'content_test.dart'; import 'test_app.dart'; import 'text_test.dart'; @@ -259,6 +260,23 @@ void main() { .isSameColorAs(EmojiReactionTheme.dark().bgUnselected); }); + testWidgets('use emoji font', (tester) async { + await prepare(); + await store.addUser(eg.selfUser); + await setupChipsInBox(tester, reactions: [ + Reaction.fromJson({ + 'user_id': eg.selfUser.userId, + 'emoji_name': 'heart', 'emoji_code': '2764', 'reaction_type': 'unicode_emoji'}), + ]); + + check(mergedStyleOf(tester, '\u{2764}')).isNotNull() + .fontFamily.equals(switch (defaultTargetPlatform) { + TargetPlatform.android => 'Noto Color Emoji', + TargetPlatform.iOS => 'Apple Color Emoji', + _ => throw StateError('unexpected platform in test'), + }); + }, variant: const TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS})); + // TODO more tests: // - Tapping a chip does the right thing // - When an image emoji fails to load, falls back to :text_emoji: From f7421bf968e6feb7eb8af17f08d254c4e9096646 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 4 Dec 2024 17:14:07 -0800 Subject: [PATCH 8/8] content: Use emoji font to show emoji nodes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For emoji like ❤ U+2764 HEAVY BLACK HEART, this causes us to show a colorful, larger glyph like ❤️ from the emoji font, instead of a glyph like ❤︎ that comes from a plain-text font and has the color and size of plain text. Fixes: #1104 --- lib/widgets/content.dart | 15 ++++++++++++++- lib/widgets/text.dart | 6 +++++- test/widgets/content_test.dart | 13 +++++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/lib/widgets/content.dart b/lib/widgets/content.dart index e401e5004e..f3b369e876 100644 --- a/lib/widgets/content.dart +++ b/lib/widgets/content.dart @@ -54,6 +54,8 @@ class ContentTheme extends ThemeExtension { textStylePlainParagraph: _plainParagraphCommon(context).copyWith( color: const HSLColor.fromAHSL(1, 0, 0, 0.15).toColor(), debugLabel: 'ContentTheme.textStylePlainParagraph'), + textStyleEmoji: TextStyle( + fontFamily: emojiFontFamily, fontFamilyFallback: const []), codeBlockTextStyles: CodeBlockTextStyles.light(context), textStyleError: const TextStyle(fontSize: kBaseFontSize, color: Colors.red) .merge(weightVariableTextStyle(context, wght: 700)), @@ -85,6 +87,8 @@ class ContentTheme extends ThemeExtension { textStylePlainParagraph: _plainParagraphCommon(context).copyWith( color: const HSLColor.fromAHSL(1, 0, 0, 0.85).toColor(), debugLabel: 'ContentTheme.textStylePlainParagraph'), + textStyleEmoji: TextStyle( + fontFamily: emojiFontFamily, fontFamilyFallback: const []), codeBlockTextStyles: CodeBlockTextStyles.dark(context), textStyleError: const TextStyle(fontSize: kBaseFontSize, color: Colors.red) .merge(weightVariableTextStyle(context, wght: 700)), @@ -113,6 +117,7 @@ class ContentTheme extends ThemeExtension { required this.colorTableHeaderBackground, required this.colorThematicBreak, required this.textStylePlainParagraph, + required this.textStyleEmoji, required this.codeBlockTextStyles, required this.textStyleError, required this.textStyleErrorCode, @@ -152,6 +157,9 @@ class ContentTheme extends ThemeExtension { /// should not need styles from other sources, such as Material defaults. final TextStyle textStylePlainParagraph; + /// The [TextStyle] to use for Unicode emoji. + final TextStyle textStyleEmoji; + final CodeBlockTextStyles codeBlockTextStyles; final TextStyle textStyleError; final TextStyle textStyleErrorCode; @@ -201,6 +209,7 @@ class ContentTheme extends ThemeExtension { Color? colorTableHeaderBackground, Color? colorThematicBreak, TextStyle? textStylePlainParagraph, + TextStyle? textStyleEmoji, CodeBlockTextStyles? codeBlockTextStyles, TextStyle? textStyleError, TextStyle? textStyleErrorCode, @@ -222,6 +231,7 @@ class ContentTheme extends ThemeExtension { colorTableHeaderBackground: colorTableHeaderBackground ?? this.colorTableHeaderBackground, colorThematicBreak: colorThematicBreak ?? this.colorThematicBreak, textStylePlainParagraph: textStylePlainParagraph ?? this.textStylePlainParagraph, + textStyleEmoji: textStyleEmoji ?? this.textStyleEmoji, codeBlockTextStyles: codeBlockTextStyles ?? this.codeBlockTextStyles, textStyleError: textStyleError ?? this.textStyleError, textStyleErrorCode: textStyleErrorCode ?? this.textStyleErrorCode, @@ -250,6 +260,7 @@ class ContentTheme extends ThemeExtension { colorTableHeaderBackground: Color.lerp(colorTableHeaderBackground, other.colorTableHeaderBackground, t)!, colorThematicBreak: Color.lerp(colorThematicBreak, other.colorThematicBreak, t)!, textStylePlainParagraph: TextStyle.lerp(textStylePlainParagraph, other.textStylePlainParagraph, t)!, + textStyleEmoji: TextStyle.lerp(textStyleEmoji, other.textStyleEmoji, t)!, codeBlockTextStyles: CodeBlockTextStyles.lerp(codeBlockTextStyles, other.codeBlockTextStyles, t), textStyleError: TextStyle.lerp(textStyleError, other.textStyleError, t)!, textStyleErrorCode: TextStyle.lerp(textStyleErrorCode, other.textStyleErrorCode, t)!, @@ -1031,7 +1042,9 @@ class _InlineContentBuilder { child: UserMention(ambientTextStyle: widget.style, node: node)); case UnicodeEmojiNode(): - return TextSpan(text: node.emojiUnicode, recognizer: _recognizer); + return TextSpan(text: node.emojiUnicode, recognizer: _recognizer, + style: widget.style + .merge(ContentTheme.of(_context!).textStyleEmoji)); case ImageEmojiNode(): return WidgetSpan(alignment: PlaceholderAlignment.middle, diff --git a/lib/widgets/text.dart b/lib/widgets/text.dart index e50ac40668..03fa0f32bd 100644 --- a/lib/widgets/text.dart +++ b/lib/widgets/text.dart @@ -153,9 +153,13 @@ const kDefaultFontFamily = 'Source Sans 3'; /// The [TextStyle.fontFamilyFallback] for use with [kDefaultFontFamily]. List get defaultFontFamilyFallback => [ - _useAppleEmoji ? 'Apple Color Emoji' : 'Noto Color Emoji', + emojiFontFamily, ]; +String get emojiFontFamily { + return _useAppleEmoji ? 'Apple Color Emoji' : 'Noto Color Emoji'; +} + /// Whether to use the Apple Color Emoji font for showing emoji. /// /// When false, we use Noto Color Emoji instead. diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart index 4fb5e01b61..bee5325714 100644 --- a/test/widgets/content_test.dart +++ b/test/widgets/content_test.dart @@ -877,6 +877,19 @@ void main() { testContentSmoke(ContentExample.emojiUnicode); testContentSmoke(ContentExample.emojiUnicodeMultiCodepoint); testContentSmoke(ContentExample.emojiUnicodeLiteral); + + testWidgets('use emoji font', (tester) async { + // Compare [ContentExample.emojiUnicode]. + const emojiHeartHtml = + '

:heart:

'; + await prepareContent(tester, plainContent(emojiHeartHtml)); + check(mergedStyleOf(tester, '\u{2764}')).isNotNull() + .fontFamily.equals(switch (defaultTargetPlatform) { + TargetPlatform.android => 'Noto Color Emoji', + TargetPlatform.iOS => 'Apple Color Emoji', + _ => throw StateError('unexpected platform in test'), + }); + }, variant: const TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS})); }); group('inline math', () {