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

chrisbobbe · 2024-03-01T22:50:00Z

Also, use weightVariableTextStyle in one place we forgot to.

Fixes: #538

chrisbobbe · 2024-03-01T22:53:39Z

Before	After

gnprice

Thanks! The code all looks good with one small nit below.

Then: can you write a test for this?

For example, we could render the content

# one `two`

three `four`

and check that the font sizes on the spans reading "one" and "two" are in the same ratio to each other as the font sizes on the spans reading "three" and "four". Then similarly for @-mentions and global times.

gnprice · 2024-03-04T19:24:18Z

lib/widgets/content.dart

+    required this.node,
+    required this.surroundingTextStyle,
+  });

  final UserMentionNode node;
+  final TextStyle surroundingTextStyle;


nit: I think of the node field of these content widgets as somewhat akin to child — it's the payload of the widget, describing what content it should show, whereas this other field is some metadata about how to show it. So let's put the node at the end.

(From a quick search for "node;" in this file, it looks like these are actually the first examples of a content widget that takes any other field in addition to a "node".)

chrisbobbe · 2024-03-29T00:56:50Z

Thanks for the review! Still working on the test.

The code all looks good with one small nit below.

Cool. I'll go ahead and merge the first five commits, then:

content test: Add ContentExample.globalTime
content test: Add ContentExample.strong, and its widget smoke test
content test: Add ContentExample.emphasis, and its widget smoke test
content test: Add ContentExample.inlineCode, and its widget smoke test
content test: Add ContentExamples for @-mentions, also widget smoke tests

since they're helpful independently of the interesting changes for this bugfix.

gnprice · 2024-03-29T01:03:19Z

Those sound useful! But I don't actually see them in this PR — is there somewhere else you're thinking of?

chrisbobbe · 2024-03-29T01:05:55Z

Oh goodness! 😅 Thanks for noticing that. It turns out I was looking at my local branch, and I forgot that these commits were not included in what you had reviewed already. I'll hold off until I've sent a new revision and you've approved it; that revision will include tests for the bugfix (with the complicated recurse-through-spans-merging-styles helper).

chrisbobbe · 2024-03-29T22:00:05Z

OK, revision pushed, with those complicated tests as described in #544 (review).

…Actually this revision doesn't yet test @-mentions or global times (or inline math) in that way; just inline code. But if the current implementation looks good for testing inline code, I can then apply it to those other things too.

gnprice

Thanks! This looks like a good structure; comments below.

gnprice · 2024-04-01T19:58:19Z

test/model/content_test.dart

+    '<p><span class="user-group-mention silent" data-user-group-id="186">test-empty</span></p>',
+      const UserMentionNode(nodes: [TextNode('test-empty')]));


nit: indentation

Suggested change

'test-empty',

const UserMentionNode(nodes: [TextNode('test-empty')]));

'test-empty',

const UserMentionNode(nodes: [TextNode('test-empty')]));

gnprice · 2024-04-01T19:58:57Z

test/model/content_test.dart

+    const StrongNode(nodes: [TextNode('bold')]),
+  );


nit:

Suggested change

const StrongNode(nodes: [TextNode('bold')]),

);

const StrongNode(nodes: [TextNode('bold')]));

(and similarly for some others, I've noticed)

gnprice · 2024-04-01T20:00:08Z

test/model/content_test.dart

+    testParseExample(ContentExample.userMentionSilent);

    testParseInline('silent user @-mention, class order reversed',
      // "@_**Greg Price**" (hypothetical server variation)
      '<p><span class="silent user-mention" data-user-id="2187">Greg Price</span></p>',
      const UserMentionNode(nodes: [TextNode('Greg Price')]));


I think these classes-flipped examples are a bit easier to read if they're right next to their unflipped counterparts — that way the reader can easily see that that's the only thing different. So it'd be good to convert them at the same time.

gnprice · 2024-04-01T20:05:40Z

lib/widgets/content.dart

-      padding: const EdgeInsets.symmetric(horizontal: 0.2 * kBaseFontSize),
+      padding: EdgeInsets.symmetric(horizontal: 0.2 * surroundingTextStyle.fontSize!),


Interesting, it looks like the revision I previously reviewed didn't have this change. Nor the similar padding change for dates, or the one for the clock icon size.

Would you update the screenshot to reflect this revision?

Ah, yeah. Done.

Cool, thanks.

Looking at that screenshot, I think it's probably best to go ahead and make this adjustment for the clock icon, but not for the padding. The expanded padding feels kind of out of scale (even though in a literal sense it is precisely to scale).

It's reminiscent of the situation described at https://github.com/zulip/zulip-mobile/blob/dff4d4d5ed495aba0e97f105215ce425909e6d53/docs/background/webview.md#why-mostly-px-and-not-rem :

Typical advice for web design is to use rem or em for nearly all lengths. This means that when the font size grows or shrinks, not only the text but the whole layout scales with it, including padding, margin, and images.

But on mobile, if you go to a device's settings and change the font size, you'll find this is not at all what apps do. When you make the font size giant, the text in an app grows with it -- but everything that isn't text stays the same. Icons in the UI; user avatars; padding around text; padding or margin between elements; indentation of items in a list; all stay exactly the same. This is true on Android and iOS, and of flagship apps from Google, Apple, Facebook, and others.

Arguably that'd call for not scaling the clock icon either; but I think the clock icon looks best when it does get scaled. Effectively it feels like part of the text — like a sort of custom emoji.

gnprice · 2024-04-01T20:08:06Z

test/widgets/content_test.dart

+      /// This isn't how the style actually gets applied in the code under test,
+      /// but it should hopefully simulate it closely enough.


Suggested change

/// This isn't how the style actually gets applied in the code under test,

/// but it should hopefully simulate it closely enough.

/// This isn't how the style actually gets applied in the code under test

/// (because that happens inside the engine, in SkParagraph),

/// but it should hopefully simulate it closely enough.

That helps answer the question of why we don't just do what the code under test actually does.

gnprice · 2024-04-01T20:11:13Z

test/widgets/content_test.dart

+          }
+          return notInSubtree;
+        }
+        recurse(outerSpan);


Let's have this use the return value to assert that innerSpan was actually found somewhere.

Also the dartdoc can say explicitly that innerSpan is required to be a descendant of outerSpan — that should help a reader fit the pieces together of what this function is doing.

gnprice · 2024-04-01T20:25:48Z

test/widgets/content_test.dart

+          styles.add(span.style);
+          if (span == innerSpan) {
+            return false;
+          }
+          final notInSubtree = span.visitDirectChildren(recurse);
+          if (notInSubtree) {
+            styles.removeLast();
+          }


Here's a variant that's slightly more efficient:

Suggested change

styles.add(span.style);

if (span == innerSpan) {

return false;

}

final notInSubtree = span.visitDirectChildren(recurse);

if (notInSubtree) {

styles.removeLast();

}

if (span == innerSpan) {

styles.add(span.style);

return false;

}

final notInSubtree = span.visitDirectChildren(recurse);

if (!notInSubtree) {

styles.add(span.style);

}

This way we only start adding to the list when we find the inner span, and never add anything we later have to remove.

The list goes from inner to outer when built this way, rather than outer to inner; we can then say styles.reversed.reduce(…) below.

gnprice · 2024-04-01T20:30:26Z

test/widgets/content_test.dart

+        return styles.reduce((value, element) => switch ((value, element)) {
+          (TextStyle(), TextStyle()) => value!.merge(element!),


After the comment above about the algorithm, I notice that these tests pass just the same if I reverse the order these get merged in. It's fairly subtle to get that ordering the right way around, so it'd be good to have a check to prevent it accidentally breaking. (When merged in the wrong order, all the fonts are the same — so the ratio check always passes, without successfully testing what we want to test.)

Probably the simplest way to do that is to check that the font size is bigger in the header than the paragraph, and bigger in the plain text than the inline code — say by at least 10% in each case. That's definitely a solid expectation for header vs. paragraph, and mostly solid for plain vs. code; and if someday we change the code style so the latter no longer holds, we can figure out a different check.

gnprice · 2024-04-01T20:36:04Z

test/widgets/content_test.dart

+        final styles = <TextStyle?>[];
+        bool recurse(InlineSpan span) {
+          styles.add(span.style);
+          if (span == innerSpan) {


Suggested change

if (span == innerSpan) {

if (identical(span, innerSpan)) {

I.e. the equivalent of JS ===.

Otherwise this calls the operator == override, which can accept more things as equal.

gnprice · 2024-04-01T20:37:45Z

test/widgets/content_test.dart

+      TextStyle? mergedStyleOfSubstring(InlineSpan rootSpan, String substring) {
+        final substringSpan = rootSpan.getSpanForPosition(
+          TextPosition(offset: rootSpan.toPlainText().indexOf(substring)));
+        check(substringSpan).isNotNull();
+        return mergeStylesOuterToInner(rootSpan, substringSpan!);


This works fine for this test, and if you find it works fine for the other test cases then it can stay just as it is.

A potentially cleaner, and certainly more flexible, version would have the recursion take not an InlineSpan to look for, but a predicate to call on each descendant span to see if it's the one we're looking for. Then the predicate here can be like (span) => span.text == substring. That way we recurse through the tree only once, and it opens the way for different predicates for other test cases if needed.

chrisbobbe · 2024-04-02T00:55:26Z

Thanks for the review! Revision pushed, and I've added tests for the sizes of inline math, @-mentions, and global times as well. Those are all still pretty verbose even with the helper code…I'm not sure if that's worth spending time on right now.

gnprice

Thanks! Generally this looks good.

Two small comments below, and I also took a bit of time just now to try factoring out the common logic from these tests. Here's the diffstat, including some comments:

 test/widgets/content_test.dart | 210 +++++++++++++++-------------------------------
 1 file changed, 66 insertions(+), 144 deletions(-)

I'll push that to the branch as additional commits on top; PTAL.

Then when I was testing those test changes, I found that one of these four test cases doesn't work as a test — and that's already true in the PR branch. So that'll be good to fix.

gnprice · 2024-04-02T19:28:19Z

test/model/content_test.dart

+    testParseExample(ContentExample.groupMentionSilentClassOrderReversed);

    testParseInline('silent group @-mention, class order reversed',


This testParseInline was meant to be deleted in favor of the testParseExample, I think.

gnprice · 2024-04-02T19:34:20Z

test/widgets/content_test.dart

+/// Simulate a nested "inner" span's style by merging all ancestor-span
+/// styles, starting from the root.
+///
+/// [isInnerSpan] must return true for a descendant of [outerSpan].


Suggested change

/// [isInnerSpan] must return true for a descendant of [outerSpan].

/// [isInnerSpan] must return true for some descendant of [outerSpan].

(Otherwise it sounds like it's saying that isInnerSpan's job is to tell whether the span it's given is a descendant, and return true just if it is.)

gnprice · 2024-04-02T20:22:34Z

test/widgets/content_test.dart

+      final headerMentionStyle = mergeSpanStylesOuterToInner(headerRootSpan,
+        (span) => span is WidgetSpan && span.child is UserMention);


This test seems not to actually work — if I check out the old code in lib, the test still passes.

Thanks for the catch. Hmm.

The issue is that the InlineSpan responsible for showing the user mention isn't actually where the mention's text style gets applied. That InlineSpan is a WidgetSpan. The widget itself, UserMention, is what sets the style.

WidgetSpan, like any InlineSpan, does have a style: TextStyle field. But it wouldn't be the right place to set the mention's text style. From the WidgetSpan constructor doc:

/// A [TextStyle] may be provided with the [style] property, but only the /// decoration, foreground, background, and spacing options will be used.

So I guess one thing we could do is build mergeSpanStylesOuterToInner in a way that's aware of WidgetSpans and can be told how to extract the relevant TextStyle from a WidgetSpan…

There's ultimately a text span that has the mentioned user's name somewhere inside that UserMention widget, and the text style will live there, right?

Specifically I think if we find the RenderParagraph that text span is part of, and merge together its styles in the same way as before, then that should tell us the effective style for that span. In particular I believe all the defaulting and merging that happens outside of RenderParagraph and SkParagraph winds up going into the TextStyle that gets placed at the root of that RenderParagraph's span tree.

chrisbobbe · 2024-04-02T20:48:25Z

Two small comments below, and I also took a bit of time just now to try factoring out the common logic from these tests. Here's the diffstat, including some comments:
 test/widgets/content_test.dart | 210 +++++++++++++++-------------------------------
 1 file changed, 66 insertions(+), 144 deletions(-)
I'll push that to the branch as additional commits on top; PTAL.

Ah I think you meant to push this but haven't yet; is that right?

gnprice · 2024-04-02T21:04:42Z

Ah indeed, thanks for the ping! Pushed now.

chrisbobbe · 2024-05-01T22:38:11Z

Thanks for the review and those helpful refactoring commits! Revision pushed, with those squashed into the main commit, along with some refactoring to test the things that use WidgetSpan, following #544 (comment) .

gnprice

Thanks for the revision! Generally this all looks good — some small comments below.

gnprice · 2024-05-07T00:54:39Z

test/widgets/content_test.dart

+          theme: ThemeData(typography: zulipTypography(context)),
+          localizationsDelegates: ZulipLocalizations.localizationsDelegates,
+          supportedLocales: ZulipLocalizations.supportedLocales,
+          home: Scaffold(body: BlockContentList(nodes: parseContent(html).nodes)),


nit: mention this in commit message too:

content test: Add zulipTypography in prepareContentBare

(Fine to have it as one commit — both changes are tiny.)

Huh interesting. I can't remember why I added the Scaffold. At the tip of the branch, the tests still pass if I remove it.

I guess it does bring the tests a little closer to being representative, so it doesn't hurt to keep it there…

gnprice · 2024-05-07T00:56:53Z

test/widgets/content_test.dart

+
+  testContentSmoke(ContentExample.emphasis);
+
+  group('InlineCodeNode', () {


nit: the existing groups are named for the widgets, rather than the content nodes, so let's follow that pattern

gnprice · 2024-05-07T01:01:59Z

test/widgets/content_test.dart

    // range of times. See comments in "show dates" test in
    // `test/widgets/message_list_test.dart`.
-    tester.widget(find.textContaining(RegExp(r'^(Tue, Jan 30|Wed, Jan 31), 2024, \d+:\d\d [AP]M$')));
+    final renderedTextRegexp = RegExp(r'^(Tue, Jan 30|Wed, Jan 31), 2024, \d+:\d\d [AP]M$');


This regexp only really makes sense when juxtaposed with the particular time value used in these tests. So if we're factoring out the regexp, let's factor out that time HTML too:

Suggested change

final renderedTextRegexp = RegExp(r'^(Tue, Jan 30|Wed, Jan 31), 2024, \d+:\d\d [AP]M$');

final renderedTextRegexp = RegExp(r'^(Tue, Jan 30|Wed, Jan 31), 2024, \d+:\d\d [AP]M$');

const timeSpanHtml = '<time datetime="2024-01-30T17:33:00Z">2024-01-30T17:33:00Z</time>';

(or perhaps better, HTML first and then comment and regexp)

This means the smoke test will splice that HTML inside a  element; but that's already pretty much what checkFontSizeRatio is going to do.

gnprice · 2024-05-07T01:22:23Z

test/widgets/content_test.dart

+      return switch (substringPattern) {
+        String() => text == substringPattern,
+        _ => substringPattern.allMatches(text).isNotEmpty,


These two cases don't agree in behavior — a string as a Pattern means "has this as a substring", not "equals this".

It looks like this revision doesn't end up actually using the more general Pattern case. So I think the cleanest thing is to go back to having the parameter be String substring, as in the previous revision.

This revision uses the more general Pattern case to find text matching the RegExp we use to recognize a rendered global time:

RegExp(r'^(Tue, Jan 30|Wed, Jan 31), 2024, \d+:\d\d [AP]M$')

Maybe one option is to make the cases agree in behavior, by only considering matches that cover the whole string?

return switch (substringPattern) { String() => text == substringPattern, _ => substringPattern.allMatches(text) .any((match) => match.start == 0 && match.end == text.length), };

Hmm, I see, yeah.

That solution LGTM.

gnprice · 2024-05-07T01:23:37Z

test/widgets/content_test.dart

+  /// Exactly one of `targetString` and `targetPredicate` should be provided,
+  /// and should match the [InlineSpan] rendered from that [InlineContentNode].
+  Future<void> checkFontSizeRatio(WidgetTester tester, {
+    required String targetHtml,
+    required double Function(InlineSpan rootSpan) targetFontSizeFinder,


doc should be updated to reflect targetFontSizeFinder

chrisbobbe · 2024-05-07T04:20:49Z

Thanks for the review! Revision pushed, and please see one reply above: #544 (comment)

We already have a widget smoke test for global times. For that test, it's convenient to declare the Markdown and HTML right alongside the text, instead of here in this other file, so that's why ContentExample.globalTime isn't invoked in test/widgets/content_test.dart.

…ests

Also a Scaffold.

…s etc. Fixes: zulip#538 Co-authored-by: Greg Price <greg@zulip.com>

Before this, the text was sized by Theme.of(context).textTheme.bodyMedium.fontSize, thanks to an AnimatedDefaultTextStyle in the Material widget. That value is 14 when the app's localization state is set to render text as "English-like" (see [Typography]), which at this point I think it always is.

chrisbobbe added the a-content Parsing and rendering Zulip HTML content, notably message contents label Mar 1, 2024

chrisbobbe requested a review from gnprice March 1, 2024 22:50

chrisbobbe mentioned this pull request Mar 1, 2024

content: Use 17 for base font size, and fix code/mentions/times in headings #540

Closed

gnprice reviewed Mar 4, 2024

View reviewed changes

chrisbobbe force-pushed the pr-font-size-in-headings branch from e75adc6 to f0b186d Compare March 29, 2024 21:58

gnprice reviewed Apr 1, 2024

View reviewed changes

chrisbobbe force-pushed the pr-font-size-in-headings branch from f0b186d to def2473 Compare April 2, 2024 00:51

gnprice reviewed Apr 2, 2024

View reviewed changes

chrisbobbe force-pushed the pr-font-size-in-headings branch from 69134f7 to 9d01e85 Compare May 1, 2024 22:36

gnprice reviewed May 7, 2024

View reviewed changes

chrisbobbe force-pushed the pr-font-size-in-headings branch 2 times, most recently from 6d5aad8 to d234672 Compare May 7, 2024 04:20

chrisbobbe added 8 commits May 7, 2024 18:16

content test: Add ContentExample.strong, and its widget smoke test

397fe5e

content test: Add ContentExample.emphasis, and its widget smoke test

3728701

content test: Add ContentExample.inlineCode, and its widget smoke test

049fa35

content test: Add ContentExamples for @-mentions, also widget smoke t…

348548d

…ests

content [nfc]: Make InlineContent.style required

6d0ad45

content test: Add zulipTypography in prepareContentBare

42fc75f

Also a Scaffold.

content test: Make some new groups to add tests to (coming up)

14d8e5e

chrisbobbe and others added 4 commits May 7, 2024 18:16

content test [nfc]: Pull html and a RegExp out to variables

335d2fb

content: Show code/math, mentions, and times properly even in heading…

9d951cb

…s etc. Fixes: zulip#538 Co-authored-by: Greg Price <greg@zulip.com>

content: For headings, use weightVariableTextStyle

89af98c

gnprice force-pushed the pr-font-size-in-headings branch from d234672 to 89af98c Compare May 8, 2024 01:17

gnprice merged commit 89af98c into zulip:main May 8, 2024

chrisbobbe deleted the pr-font-size-in-headings branch May 8, 2024 05:57

chrisbobbe mentioned this pull request May 20, 2024

content: Code blocks don't respond to system bold-text setting #690

Closed

		'<p><span class="user-group-mention silent" data-user-group-id="186">test-empty</span></p>',
		const UserMentionNode(nodes: [TextNode('test-empty')]));

	const StrongNode(nodes: [TextNode('bold')]),
	);
	const StrongNode(nodes: [TextNode('bold')]));

		padding: const EdgeInsets.symmetric(horizontal: 0.2 * kBaseFontSize),
		padding: EdgeInsets.symmetric(horizontal: 0.2 * surroundingTextStyle.fontSize!),

		/// This isn't how the style actually gets applied in the code under test,
		/// but it should hopefully simulate it closely enough.

		return styles.reduce((value, element) => switch ((value, element)) {
		(TextStyle(), TextStyle()) => value!.merge(element!),

		testParseExample(ContentExample.groupMentionSilentClassOrderReversed);

		testParseInline('silent group @-mention, class order reversed',

	/// [isInnerSpan] must return true for a descendant of [outerSpan].
	/// [isInnerSpan] must return true for some descendant of [outerSpan].

		final headerMentionStyle = mergeSpanStylesOuterToInner(headerRootSpan,
		(span) => span is WidgetSpan && span.child is UserMention);


		testContentSmoke(ContentExample.emphasis);

		group('InlineCodeNode', () {

	final renderedTextRegexp = RegExp(r'^(Tue, Jan 30\|Wed, Jan 31), 2024, \d+:\d\d [AP]M$');
	final renderedTextRegexp = RegExp(r'^(Tue, Jan 30\|Wed, Jan 31), 2024, \d+:\d\d [AP]M$');
	const timeSpanHtml = '<time datetime="2024-01-30T17:33:00Z">2024-01-30T17:33:00Z</time>';

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

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

Conversation

chrisbobbe commented Mar 1, 2024

chrisbobbe commented Mar 1, 2024 • edited

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Mar 29, 2024

gnprice commented Mar 29, 2024

chrisbobbe commented Mar 29, 2024 • edited

chrisbobbe commented Mar 29, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Apr 2, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Apr 2, 2024

gnprice commented Apr 2, 2024

chrisbobbe commented May 1, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe May 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented May 7, 2024

chrisbobbe commented Mar 1, 2024 •

edited

chrisbobbe commented Mar 29, 2024 •

edited

chrisbobbe May 7, 2024 •

edited