content: Show code, mentions, and times properly even in headings etc.

chrisbobbe · chrisbobbe · commit ab16013e21ae · 2024-03-29T14:46:25.000-07:00
Fixes: zulip#538
diff --git a/lib/widgets/content.dart b/lib/widgets/content.dart
@@ -553,12 +553,19 @@ class InlineContent extends StatelessWidget {
     required this.style,
     required this.nodes,
   }) {
+    assert(style.fontSize != null);
     _builder = _InlineContentBuilder(this);
   }
 
   final GestureRecognizer? recognizer;
   final Map<LinkNode, GestureRecognizer>? linkRecognizers;
+
+  /// A [TextStyle] applied to this content and provided to descendants.
+  ///
+  /// Must set [TextStyle.fontSize]. Some descendant spans will consume it,
+  /// e.g., to make their content slightly smaller than surrounding text.
   final TextStyle style;
+
   final List<InlineContentNode> nodes;
 
   late final _InlineContentBuilder _builder;
@@ -630,18 +637,21 @@ class _InlineContentBuilder {
       return _buildInlineCode(node);
     } else if (node is UserMentionNode) {
       return WidgetSpan(alignment: PlaceholderAlignment.middle,
-        child: UserMention(node: node));
+        child: UserMention(surroundingTextStyle: widget.style, node: node));
     } else if (node is UnicodeEmojiNode) {
       return TextSpan(text: node.emojiUnicode, recognizer: _recognizer);
     } else if (node is ImageEmojiNode) {
       return WidgetSpan(alignment: PlaceholderAlignment.middle,
         child: MessageImageEmoji(node: node));
     } else if (node is MathInlineNode) {
-      return TextSpan(style: _kInlineMathStyle,
+      return TextSpan(
+        style: widget.style
+          .merge(_kInlineMathStyle)
+          .apply(fontSizeFactor: _kInlineCodeFontSizeFactor),
         children: [TextSpan(text: node.texSource)]);
     } else if (node is GlobalTimeNode) {
       return WidgetSpan(alignment: PlaceholderAlignment.middle,
-        child: GlobalTime(node: node));
+        child: GlobalTime(node: node, surroundingTextStyle: widget.style));
     } else if (node is UnimplementedInlineContentNode) {
       return _errorUnimplemented(node);
     } else {
@@ -690,8 +700,13 @@ class _InlineContentBuilder {
 
     // TODO `code`: find equivalent of web's `unicode-bidi: embed; direction: ltr`
 
-    // Use a light gray background, instead of a border.
-    return _buildNodes(style: _kInlineCodeStyle, node.nodes);
+    return _buildNodes(
+      // Use a light gray background, instead of a border.
+      style: widget.style
+        .merge(_kInlineCodeStyle)
+        .apply(fontSizeFactor: _kInlineCodeFontSizeFactor),
+      node.nodes,
+    );
 
     // Another fun solution -- we can in fact have a border!  Like so:
     //   TextStyle(
@@ -711,17 +726,27 @@ class _InlineContentBuilder {
   }
 }
 
+/// The [TextStyle] for inline math, excluding font-size adjustment.
+///
+/// Inline math should use this and also apply [_kInlineCodeFontSizeFactor]
+/// to the font size of the surrounding text
+/// (which might be a Paragraph, a Heading, etc.).
 final _kInlineMathStyle = _kInlineCodeStyle.merge(TextStyle(
   backgroundColor: const HSLColor.fromAHSL(1, 240, 0.4, 0.93).toColor()));
 
+const _kInlineCodeFontSizeFactor = 0.825;
+
+/// The [TextStyle] for inline code, excluding font-size adjustment.
+///
+/// Inline code should use this and also apply [_kInlineCodeFontSizeFactor]
+/// to the font size of the surrounding text
+/// (which might be a Paragraph, a Heading, etc.).
 // Even though [kMonospaceTextStyle] is a variable-weight font,
 // it's acceptable to skip [weightVariableTextStyle] here,
 // assuming the text gets the effect of [weightVariableTextStyle]
 // through inheritance, e.g., from a [DefaultTextStyle].
 final _kInlineCodeStyle = kMonospaceTextStyle
-  .merge(const TextStyle(
-    backgroundColor: Color(0xffeeeeee),
-    fontSize: 0.825 * kBaseFontSize));
+  .merge(const TextStyle(backgroundColor: Color(0xffeeeeee)));
 
 final _kCodeBlockStyle = kMonospaceTextStyle
   .merge(const TextStyle(
@@ -752,22 +777,27 @@ final _kCodeBlockStyle = kMonospaceTextStyle
 // const _kInlineCodeRightBracket = '⟩';
 
 class UserMention extends StatelessWidget {
-  const UserMention({super.key, required this.node});
+  const UserMention({
+    super.key,
+    required this.surroundingTextStyle,
+    required this.node,
+  });
 
+  final TextStyle surroundingTextStyle;
   final UserMentionNode node;
 
   @override
   Widget build(BuildContext context) {
     return Container(
       decoration: _kDecoration,
-      padding: const EdgeInsets.symmetric(horizontal: 0.2 * kBaseFontSize),
+      padding: EdgeInsets.symmetric(horizontal: 0.2 * surroundingTextStyle.fontSize!),
       child: InlineContent(
         // If an @-mention is inside a link, let the @-mention override it.
         recognizer: null,  // TODO make @-mentions tappable, for info on user
         // One hopes an @-mention can't contain an embedded link.
         // (The parser on creating a UserMentionNode has a TODO to check that.)
         linkRecognizers: null,
-        style: Paragraph.textStyle,
+        style: surroundingTextStyle,
         nodes: node.nodes));
   }
 
@@ -834,16 +864,23 @@ class MessageImageEmoji extends StatelessWidget {
 }
 
 class GlobalTime extends StatelessWidget {
-  const GlobalTime({super.key, required this.node});
+  const GlobalTime({
+    super.key,
+    required this.node,
+    required this.surroundingTextStyle,
+  });
 
   final GlobalTimeNode node;
+  final TextStyle surroundingTextStyle;
 
   static final _backgroundColor = const HSLColor.fromAHSL(1, 0, 0, 0.93).toColor();
   static final _borderColor = const HSLColor.fromAHSL(1, 0, 0, 0.8).toColor();
   static final _dateFormat = DateFormat('EEE, MMM d, y, h:mm a'); // TODO(intl): localize date
 
   @override
   Widget build(BuildContext context) {
+    final surroundingFontSize = surroundingTextStyle.fontSize!;
+
     // Design taken from css for `.rendered_markdown & time` in web,
     //   see zulip:web/styles/rendered_markdown.css .
     final text = _dateFormat.format(node.datetime.toLocal());
@@ -855,15 +892,15 @@ class GlobalTime extends StatelessWidget {
           border: Border.all(width: 1, color: _borderColor),
           borderRadius: BorderRadius.circular(3)),
         child: Padding(
-          padding: const EdgeInsets.symmetric(horizontal: 0.2 * kBaseFontSize),
+          padding: EdgeInsets.symmetric(horizontal: 0.2 * surroundingFontSize),
           child: Row(
             mainAxisSize: MainAxisSize.min,
             children: [
-              const Icon(ZulipIcons.clock, size: kBaseFontSize),
+              Icon(ZulipIcons.clock, size: surroundingFontSize),
               // Ad-hoc spacing adjustment per feedback:
               //   https://chat.zulip.org/#narrow/stream/101-design/topic/clock.20icons/near/1729345
               const SizedBox(width: 1),
-              Text(text, style: Paragraph.textStyle),
+              Text(text, style: surroundingTextStyle),
             ]))));
   }
 }
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');
@@ -100,3 +102,7 @@ extension TypographyChecks on Subject<Typography> {
   Subject<TextTheme> get dense => has((t) => t.dense, 'dense');
   Subject<TextTheme> get tall => has((t) => t.tall, 'tall');
 }
+
+extension InlineSpanChecks on Subject<InlineSpan> {
+  Subject<TextStyle?> get style => has((x) => x.style, 'style');
+}
diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart
@@ -3,6 +3,7 @@ import 'dart:io';
 import 'package:checks/checks.dart';
 import 'package:flutter/foundation.dart';
 import 'package:flutter/material.dart';
+import 'package:flutter/rendering.dart';
 import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';
 import 'package:flutter_test/flutter_test.dart';
 import 'package:url_launcher/url_launcher.dart';
@@ -14,6 +15,7 @@ import 'package:zulip/widgets/content.dart';
 import 'package:zulip/widgets/message_list.dart';
 import 'package:zulip/widgets/page.dart';
 import 'package:zulip/widgets/store.dart';
+import 'package:zulip/widgets/text.dart';
 
 import '../example_data.dart' as eg;
 import '../flutter_checks.dart';
@@ -59,10 +61,15 @@ void main() {
   TestZulipBinding.ensureInitialized();
 
   Future<void> prepareContentBare(WidgetTester tester, String html) async {
-    await tester.pumpWidget(MaterialApp(
-      localizationsDelegates: ZulipLocalizations.localizationsDelegates,
-      supportedLocales: ZulipLocalizations.supportedLocales,
-      home: BlockContentList(nodes: parseContent(html).nodes),
+    await tester.pumpWidget(Builder(
+      builder: (context) {
+        return MaterialApp(
+          theme: ThemeData(typography: zulipTypography(context)),
+          localizationsDelegates: ZulipLocalizations.localizationsDelegates,
+          supportedLocales: ZulipLocalizations.supportedLocales,
+          home: Scaffold(body: BlockContentList(nodes: parseContent(html).nodes)),
+        );
+      }
     ));
   }
 
@@ -294,7 +301,69 @@ void main() {
 
   testContentSmoke(ContentExample.emphasis);
 
-  testContentSmoke(ContentExample.inlineCode);
+  group('InlineCodeNode', () {
+    testContentSmoke(ContentExample.inlineCode);
+
+    testWidgets('maintains font-size ratio with surrounding text', (tester) async {
+      /// Simulate a nested span's style by merging all ancestor-span styles,
+      /// starting from the root.
+      ///
+      /// This isn't how the style actually gets applied in the code under test,
+      /// but it should hopefully simulate it closely enough.
+      TextStyle? mergeStylesOuterToInner(InlineSpan outerSpan, InlineSpan innerSpan) {
+        final styles = <TextStyle?>[];
+        bool recurse(InlineSpan span) {
+          styles.add(span.style);
+          if (span == innerSpan) {
+            return false;
+          }
+          final notInSubtree = span.visitDirectChildren(recurse);
+          if (notInSubtree) {
+            styles.removeLast();
+          }
+          return notInSubtree;
+        }
+        recurse(outerSpan);
+
+        return styles.reduce((value, element) => switch ((value, element)) {
+          (TextStyle(), TextStyle()) => value!.merge(element!),
+          (TextStyle(), null)        => value,
+          (null, TextStyle())        => element,
+          (null, null)               => null,
+        });
+      }
+
+      TextStyle? mergedStyleOfSubstring(InlineSpan rootSpan, String substring) {
+        final substringSpan = rootSpan.getSpanForPosition(
+          TextPosition(offset: rootSpan.toPlainText().indexOf(substring)));
+        check(substringSpan).isNotNull();
+        return mergeStylesOuterToInner(rootSpan, substringSpan!);
+      }
+
+      await prepareContentBare(tester,
+        '<h1>header-plain<code>header-code</code></h1>\n'
+        '<p>paragraph-plain<code>paragraph-code</code></p>');
+
+      final headerRootSpan = tester.renderObject<RenderParagraph>(find.textContaining('header')).text;
+      final headerPlainStyle = mergedStyleOfSubstring(headerRootSpan, 'header-plain');
+      final headerCodeStyle = mergedStyleOfSubstring(headerRootSpan, 'header-code');
+
+      final paragraphRootSpan = tester.renderObject<RenderParagraph>(find.textContaining('paragraph')).text;
+      final paragraphPlainStyle = mergedStyleOfSubstring(paragraphRootSpan, 'paragraph-plain');
+      final paragraphCodeStyle = mergedStyleOfSubstring(paragraphRootSpan, 'paragraph-code');
+
+      check(headerPlainStyle).isNotNull().fontSize.isNotNull();
+      check(headerCodeStyle).isNotNull().fontSize.isNotNull();
+      check(paragraphPlainStyle).isNotNull().fontSize.isNotNull();
+      check(paragraphCodeStyle).isNotNull().fontSize.isNotNull();
+
+      final ratioInHeader = headerCodeStyle!.fontSize! / headerPlainStyle!.fontSize!;
+      final ratioInParagraph = paragraphCodeStyle!.fontSize! / paragraphPlainStyle!.fontSize!;
+
+      // Empirically we might have e.g. 0.825 and 0.8250000000000001.
+      check((ratioInHeader - ratioInParagraph).abs()).isLessThan(0.001);
+    });
+  });
 
   testContentSmoke(ContentExample.userMentionPlain);
   testContentSmoke(ContentExample.userMentionSilent);
