content: Consolidate message-content base style and apply it more accurately #698

chrisbobbe · 2024-05-22T01:03:34Z

I have four main goals for this PR:

Stop applying the Zulip-message text color to things that aren't Zulip-message text (and refactor to make these errors easy to avoid)
Don't regress on performance in the message list (for example by making a DefaultTextStyle for every message)
Define and use a consolidated "base message text style" for content widgets to inherit from (which I do by lumping together the text color and the Paragraph styles)
Add a ThemeExtension to our app's ThemeData, as a home for certain content styles (that we don't want to recompute for each message, or that differ between light and dark theme and will need to be lerped for the fade animation)

Along the way, I make some small changes/fixes:

Message sender name was hsl(0deg 0% 15%); it now matches web at hsl(0deg 0% 20%)
content: Markers on list items are smaller than list-item text #697 (which I filed just now)
<br> in a block context, a.k.a. LineBreakNode, has the proper/expected font size and line height, instead of values from a Material default. (Inconspicuous and not worth filing an issue for; I don't know how to make one of these except with help from the server bug that causes Patch up spurious vertical space after quoted code blocks #641.)
Code blocks, and our basic math-block implementation, now have 1.4x line height instead of 1.43x. (This is to match web instead of a Material default, and is inconspicuous.)

(edit: Scroll down for up-to-date screenshots.)

Fixes: #697
Related: #95

gnprice · 2024-05-22T04:51:07Z

Let's skip the refactor that passes ambientTextStyle everywhere:
ec77212 content [nfc]: Apply default color by passing down ambientTextStyle

because it seems like it makes a lot of the code worse by having to pass around a new parameter that mostly just goes through untouched. Doing that through a widget subtree is exactly the sort of thing that InheritedWidget is designed to replace. And I'm dubious this way is necessarily any faster — it's doing a little more work in all those widget constructors within each message, in place of doing a little more work at the root widget of the message.

I think having one DefaultTextStyle at the top of each MessageContent is fine. Widgets are cheap, as long as they're simple widgets.

Avoiding DefaultTextStyle.merge, in favor of giving some already-computed style directly, will be good for explicitness as well as for potentially a small performance gain. ContentTheme looks like a good strategy for doing that.

…ance As Greg points out: zulip#698 (comment) > Avoiding `DefaultTextStyle.merge`, in favor of giving some > already-computed style directly, will be good for explicitness as > well as for potentially a small performance gain. Before this, the inheritance via `DefaultTextStyle.merge` made it kind of tiring to work out what plain-paragraph styling actually was. It was looking up to the nearest DefaultTextStyle, which was actually an AnimatedDefaultTextStyle in the Material widget, that was providing Theme.of(context).textTheme.bodyMedium. That, in turn, was built from various things: - [_M3Typography.englishLike] - [Typography.blackCupertino] or [TextTheme.blackMountainView] (depending on platform) - `zulipTypography` in lib/widgets/text.dart I've followed these sources in writing down this complete style object, to make this a direct, NFC translation.

chrisbobbe · 2024-05-22T21:33:43Z

Sure, makes sense. Revision pushed.

gnprice

Thanks for the revision! Small comments; otherwise LGTM.

gnprice · 2024-05-23T02:33:38Z

lib/widgets/content.dart

@@ -36,7 +105,9 @@ class MessageContent extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return InheritedMessage(message: message,
-      child: BlockContentList(nodes: content.nodes));
+      child: DefaultTextStyle(


The content elements that had been getting these attributes from TextTheme.bodyMedium ( fontSize: 14.0 height: 1.43 ), and are now getting them from here ( fontSize: 17 height: 22 / 17 which is ~1.29 ), are: - line breaks (for `LineBreakNode`), - code blocks (and our simplified math blocks), and - list-item markers. So, those all look different after this change.

In your screenshots, I see the difference in the list-item markers. I don't see a difference in the code blocks; did their font size end up changing, or was that change suppressed by something else?

Oh, oops: yeah, code blocks set their own font size (0.825 * kBaseFontSize), so that doesn't change in this commit. Code blocks don't set their line height, so that does change, from 1.43x to ~1.29x. But then a subsequent commit sets it to 1.4x (to follow web), which is very close to the original 1.43x.

gnprice · 2024-05-23T02:35:20Z

lib/widgets/message_list.dart

                        fontFamily: 'Source Sans 3',
                        fontSize: 18,
                        height: (22 / 18),
+                        color: const HSLColor.fromAHSL(1, 0, 0, 0.2).toColor(),


msglist: Set message-sender-name color to match web … That color: const HSLColor.fromAHSL(1, 0, 0, 0.15).toColor() doesn't match web, it turns out. (Or the old Figma, which uses true black.)

It'd be good to have this change in screenshots, too.

gnprice · 2024-05-23T02:37:39Z

lib/widgets/content.dart

+  /// The [ContentTheme] from the context's active theme.
+  ///
+  /// The [ThemeData] must include [ContentTheme] in [ThemeData.extensions].
+  static ContentTheme of(BuildContext context) {


nit: let's follow the Flutter conventional order for class members:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#order-other-class-members-in-a-way-that-makes-sense

gnprice · 2024-05-23T02:47:48Z

lib/widgets/content.dart

+/// background. For what this is in the message list, see
+/// widgets/message_list.dart.
+class ContentTheme extends ThemeExtension<ContentTheme> {
+  /// The complete [TextStyle] we use for plain, unstyled paragraphs.


- [Typography.blackCupertino] or [TextTheme.blackMountainView] (depending on platform)

(Typography.blackMountainView?)

…ance As Greg points out: zulip#698 (comment) > Avoiding `DefaultTextStyle.merge`, in favor of giving some > already-computed style directly, will be good for explicitness as > well as for potentially a small performance gain. Before this, the inheritance via `DefaultTextStyle.merge` made it kind of tiring to work out what plain-paragraph styling actually was. It was looking up to the nearest DefaultTextStyle, which was actually an AnimatedDefaultTextStyle in the Material widget, that was providing Theme.of(context).textTheme.bodyMedium. That, in turn, was built from various things: - [_M3Typography.englishLike] - [Typography.blackCupertino] or [Typography.blackMountainView] (depending on platform) - `zulipTypography` in lib/widgets/text.dart I've followed these sources in writing down this complete style object, to make this a direct, NFC translation.

chrisbobbe · 2024-05-24T00:20:30Z

Thanks for the review! Revision pushed, and here's a fresh batch of screenshots from it. One interesting finding is that the difference between 1.43x and 1.4x line height (in code blocks) doesn't seem to be visible on my iPhone. Maybe there's a step that rounds to the nearest physical pixel, and maybe it would be observable on a device with higher pixel density than mine.

Before	After

gnprice · 2024-05-24T03:45:57Z

Cool, thanks for the revision and the screenshots!

I see that sender-name-color difference, though it's awfully subtle — I had to look several times before I spotted it.

In the last screenshot, the difference in unordered list-item markers is pretty noticeable. The new, larger size definitely looks better.

All looks good; merging.

We don't actually use DefaultTextStyle to set a different variable-weight font, and the old comment text might make a reader go hunting for where we do that. (What we do is set kDefaultFontFamily in zulipTypography, which feeds into [TextTheme].)

I overlooked this in zulip#686 because this text's color wasn't set at this site explicitly, and my process for that PR was to search case-insensitively for the term [color]. Instead of being set locally, this text's color was being set by the DefaultTextStyle that wraps the whole message list for the sake of coloring message content. That color: const HSLColor.fromAHSL(1, 0, 0, 0.15).toColor() doesn't match web, it turns out. (Or the old Figma, which uses true black.) Web colors this with var(--color-text-sender-name), but that variable is undefined for light theme, so it falls back to --color-text-default which is this color. At this commit, if I change the message list's DefaultTextStyle to set a conspicuous color, like orange, I only see a response from message content, and not from sender names or anything else. That's good, but it would be better to scope the DefaultTextStyle to just the message content, to avoid errors like this in the future. I'll do that later in this series of commits.

This new name is a bit more accurate. For example, an @-mention inside an h1 should have a large font size just because it's in an h1, not because of any text that surrounds the @-mention. There might not even be any such text.

The purpose of this DefaultTextStyle is to color message content. Moving it leafward, so it proximally wraps message content, is helpful to avoid the error of applying the color to other message-list elements that don't want it. A recent commit fixed an instance of this error with sender names. I believe there weren't any other instances, so I've marked this NFC.

This is NFC because baseContentTextStyle just sets the color attribute, and this particular Paragraph only renders a link, which will clobber the default color with the link color. But we're about to move some other text style attributes (size and line height) from Paragraph to baseContentTextStyle, and we don't want the link to lose those style attributes when we make that change.

For non-paragraph content that wants to inherit its font size and line height attributes, it seems at least as natural to inherit them from our plain paragraph style, as from...hmm... TextTheme.bodyMedium, as is happening currently. Doing so should make the content styles more self-contained and decrease the need for nonlocal reasoning when reading the code. These are the elements that inherit both their font-size and line-height attributes: - line breaks (i.e., <br> in a block context; `LineBreakNode`) - list-item markers (e.g., "1." in a numbered list) Instead of font-size 14 and line-height 1.43x, from TextTheme.bodyMedium, those now have font-size 17 and line-height ~1.29x (i.e., 22 / 17). Since the list-item markers change is somewhat conspicuous, I filed zulip#697 ("content: Markers on list items are smaller than list-item text"), which this commit fixes. Code blocks (and our simplified math blocks) set their own font size, but they do inherit their line height. So those now also have a line height of ~1.29x instead of 1.43x. As it happens, web sets a line height of 1.4x for code blocks, which we should probably follow (intentionally). I'll make that adjustment next, but now hopefully this is a helpful structure for our content implementations. Fixes: zulip#697

(And apply the same change to our simplified math blocks, which use this variable too.) This was ~1.29x in the previous commit, and 1.43x before that. (The 1.43x was conveniently very close to 1.4x, but that was basically an accident. It's so close that on my device I can't see a difference in a screenful of code block content...perhaps there's a step that rounds to a physical-pixel count, and the difference might be observable on a device with a higher pixel density than mine.)

See dartdoc for rationale.

…needed This makes the tests more representative, and also more flexible when we want to change the [MaterialApp.theme] that our widgets want to consume.

…ance As Greg points out: zulip#698 (comment) > Avoiding `DefaultTextStyle.merge`, in favor of giving some > already-computed style directly, will be good for explicitness as > well as for potentially a small performance gain. Before this, the inheritance via `DefaultTextStyle.merge` made it kind of tiring to work out what plain-paragraph styling actually was. It was looking up to the nearest DefaultTextStyle, which was actually an AnimatedDefaultTextStyle in the Material widget, that was providing Theme.of(context).textTheme.bodyMedium. That, in turn, was built from various things: - [_M3Typography.englishLike] - [Typography.blackCupertino] or [Typography.blackMountainView] (depending on platform) - `zulipTypography` in lib/widgets/text.dart I've followed these sources in writing down this complete style object, to make this a direct, NFC translation.

I'm not sure if this actually improves how we show non-alphabetic text in any cases at all (so I'm not sure if it's NFC), but anyway, hard-coding this attribute here or anywhere has a smell of being counterproductive for i18n; remove it.

chrisbobbe force-pushed the pr-content-styles branch 2 times, most recently from 2e5a264 to b8dfae2 Compare May 22, 2024 01:08

chrisbobbe requested a review from gnprice May 22, 2024 01:13

chrisbobbe added integration review Added by maintainers when PR may be ready for integration a-content Parsing and rendering Zulip HTML content, notably message contents labels May 22, 2024

chrisbobbe force-pushed the pr-content-styles branch from b8dfae2 to 4d54d4e Compare May 22, 2024 21:33

gnprice reviewed May 23, 2024

View reviewed changes

chrisbobbe force-pushed the pr-content-styles branch from 4d54d4e to 85e9cb5 Compare May 24, 2024 00:19

chrisbobbe added 14 commits May 23, 2024 20:46

content [nfc]: s/surroundingTextStyle/ambientTextStyle/

9d9eafe

This new name is a bit more accurate. For example, an @-mention inside an h1 should have a large font size just because it's in an h1, not because of any text that surrounds the @-mention. There might not even be any such text.

content [nfc]: Inline a variable in GlobalTime.build

c438cc4

content [nfc]: Pull out baseContentTextStyle variable

c0521f7

content [nfc]: Put plain-paragraph text style in a ThemeExtension

8669c4a

See dartdoc for rationale.

content: Give ContentTheme.textStylePlainParagraph a debugLabel

cbeb74a

widget tests: Use whole [zulipThemeData] where [theme.extensions] is …

b73962f

…needed This makes the tests more representative, and also more flexible when we want to change the [MaterialApp.theme] that our widgets want to consume.

gnprice force-pushed the pr-content-styles branch from 85e9cb5 to 6284ed5 Compare May 24, 2024 03:47

gnprice merged commit 6284ed5 into zulip:main May 24, 2024

chrisbobbe deleted the pr-content-styles branch May 24, 2024 03:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

content: Consolidate message-content base style and apply it more accurately #698

content: Consolidate message-content base style and apply it more accurately #698

chrisbobbe commented May 22, 2024 •

edited

Loading

gnprice commented May 22, 2024

chrisbobbe commented May 22, 2024

gnprice left a comment

gnprice May 23, 2024

chrisbobbe May 23, 2024 •

edited

Loading

gnprice May 23, 2024

gnprice May 23, 2024

gnprice May 23, 2024

chrisbobbe commented May 24, 2024

gnprice commented May 24, 2024

content: Consolidate message-content base style and apply it more accurately #698

content: Consolidate message-content base style and apply it more accurately #698

Conversation

chrisbobbe commented May 22, 2024 • edited Loading

gnprice commented May 22, 2024

chrisbobbe commented May 22, 2024

gnprice left a comment

Choose a reason for hiding this comment

gnprice May 23, 2024

Choose a reason for hiding this comment

chrisbobbe May 23, 2024 • edited Loading

Choose a reason for hiding this comment

gnprice May 23, 2024

Choose a reason for hiding this comment

gnprice May 23, 2024

Choose a reason for hiding this comment

gnprice May 23, 2024

Choose a reason for hiding this comment

chrisbobbe commented May 24, 2024

gnprice commented May 24, 2024

chrisbobbe commented May 22, 2024 •

edited

Loading

chrisbobbe May 23, 2024 •

edited

Loading