content: Make bold bolder even in spoiler headers and h1/h2/etc. #706

chrisbobbe · 2024-05-24T21:27:36Z

This fixes #705 by making boldness context-dependent, much like we did for some font sizes in #544.

That user-facing change is nice but fairly subtle. I'm also hoping this will be helpful by adding content tests for boldness in the first place; we didn't have any before. And I've set a pattern in the UserMention tests that should help reduce confusion when we write boldness tests for #647. (Confusion mainly related to the fact that UserMention is in a WidgetSpan.)

The need for more helpers in lib/widgets/text.dart is another reminder that it'll be great to have flutter/flutter#148026 fixed upstream. But (as with the existing helpers there) I'm hoping this handles things in a way that makes it straightforward to migrate once that's done.

Fixes: #705

Before	After

chrisbobbe · 2024-05-24T21:30:04Z

Screenshots for the last commit. ("h1: regular" has weight 900, and "bold" has weight 1000…I don't see much of a difference there ~~, but that's fine~~. This might be a reason to drop the last commit; distinguishing a bold span from surrounding text seems important and worth preserving as completely as possible.)

Before	After

chrisbobbe · 2024-05-24T21:36:16Z

Oh, and here are versions of all the above screenshots with the system "bold text" setting on:

Before	After

gnprice

Thanks! Various nits below.

Screenshots for the last commit. ("h1: regular" has weight 900, and "bold" has weight 1000…I don't see much of a difference there ~~, but that's fine~~. This might be a reason to drop the last commit; distinguishing a bold span from surrounding text seems important and worth preserving as completely as possible.)

Hmm, yeah. What does a screenshot look like without that last commit? (That last commit being:
a4604c7 content: Make h1 font weight combine with spoiler-header font weight
)

gnprice · 2024-05-28T19:05:03Z

lib/widgets/text.dart

+/// setting, so if the [TextStyle] was built using [weightVariableTextStyle],
+/// this value might be larger than the `wght` that was passed to that function.
+double? wghtFromTextStyle(TextStyle style) {
+  double? result = style.fontVariations?.singleWhereOrNull((v) => v.axis == 'wght')?.value;


Suggested change

double? result = style.fontVariations?.singleWhereOrNull((v) => v.axis == 'wght')?.value;

double? result = style.fontVariations?.firstWhereOrNull((v) => v.axis == 'wght')?.value;

The "single" behavior means that if for some reason there are multiple wght variations, this would ignore them all and fall back to fontWeight. That doesn't seem intended.

I don't feel a strong need to have this code check that there aren't multiple of them. (In fact it looks like its one caller already has an assert to that effect.) If we're not going to check, then it should just do the simplest thing, which is to break the loop and return as soon as it finds one. That's what the "first" version does.

Makes sense; I agree.

gnprice · 2024-05-28T19:17:24Z

test/widgets/content_test.dart

@@ -154,6 +155,43 @@ void main() {
    });
  }

+  /// To check font weight, set up tests with and without bold-text setting.


The "To check" here reads like it's an instruction: "If you want to check font weight, set up tests thus-and-so."

A direct fix for that would be:

Set up tests with and without bold-text setting, to check font weight.

But I feel like what I want in the summary line is more of a description of what the said tests will test. Here's a version:

Suggested change

/// To check font weight, set up tests with and without bold-text setting.

/// Test the font weight found by [styleFinder] in the rendering of [content].

///

/// The weight will be expected to be [expectedWght] when the system

/// bold-text setting is not set, and to vary appropriately when it is set.

gnprice · 2024-05-28T19:21:24Z

test/widgets/content_test.dart

+  /// [styleFinder] must return the [TextStyle] containing the "wght"
+  /// (in [TextStyle.fontVariations]) and the [TextStyle.fontWeight]
+  /// to be checked.
+  testFontWeight(String description, {


nit: explicit return type (so it doesn't look like a function call)

gnprice · 2024-05-28T19:21:36Z

test/widgets/content_test.dart

+  group('strong (bold)', () {
+    testContentSmoke(ContentExample.strong);
+
+    TextStyle findWordBold (tester) {


gnprice · 2024-05-28T19:27:30Z

lib/widgets/content.dart

+    assert(
+      style.fontSize != null
+      && (
+        style.debugLabel!.contains('weightVariableTextStyle')


nit: just use two asserts; formatting gets simpler/shorter, and the two conditions aren't closely related anyway

gnprice · 2024-05-28T19:30:44Z

lib/widgets/content.dart

-      return _buildStrong(node);
+      return _buildStrong(ambientTextStyle: widget.style, node);


No need to pass this to one of this class's own methods — it can look at widget.style directly, just like this _buildNode method can.

gnprice · 2024-05-28T19:33:59Z

test/widgets/content_test.dart

+        // **bold**
+        content: plainContent('<$name><strong>bold</strong></$name>'),


This comment is inaccurate because there's the heading element.

Fine to have this automatic generation of six test cases, just need to adjust the comment. E.g.:

Suggested change

// **bold**

content: plainContent('<$name><strong>bold</strong></$name>'),

// # **bold**, ## **bold**, ### **bold**, etc.

content: plainContent('<$name><strong>bold</strong></$name>'),

chrisbobbe · 2024-05-31T05:57:37Z

Hmm, yeah. What does a screenshot look like without that last commit? (That last commit being:
a4604c7 content: Make h1 font weight combine with spoiler-header font weight
)

Sure, here it is:

Without that commit	With

Bold text setting on:	Bold text setting on:

chrisbobbe · 2024-05-31T06:38:10Z

Thanks for the review! Revision pushed, and see the screenshots I posted just before this. I'd be happy to remove that last commit, this keeping clearer emphasis on bold spans when they appear in h1/h2/etc. in spoiler headers.

gnprice

Thanks!

Looks like the last commit doesn't affect the bold-text-setting-on case (the screenshots look identical to me except for the signal bars at top right), and for the default case I think it's probably better without. So let's drop that commit.

Other than that, one nit below, and then please go ahead and merge!

gnprice · 2024-06-05T03:29:48Z

lib/widgets/content.dart

-  InlineSpan _buildStrong(StrongNode node) => _buildNodes(node.nodes,
-    style: weightVariableTextStyle(_context!, wght: 600));
+  InlineSpan _buildStrong(StrongNode node) =>
+    _buildNodes(style: bolderWghtTextStyle(widget.style, by: 200),
+      node.nodes);


nit: unnecessary churn here, and makes it look different from the similar neighboring methods; can keep node.nodes first and the existing formatting

Not because we want to make changes that apply here, but just because this fits conceptually, and this function's dartdoc may be useful for the reader.

It's reasonable to expect callers to avoid passing around out-of-bounds "wght" values. If we're at the point of passing an out-of-bounds value to this function, chances are we're also rendering some text with that same value, and we shouldn't do that.

… soon

Fixes: zulip#705

…ontext

chrisbobbe · 2024-06-05T16:47:55Z

Thanks! Done.

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label May 24, 2024

chrisbobbe requested a review from gnprice May 24, 2024 21:27

gnprice reviewed May 28, 2024

View reviewed changes

chrisbobbe force-pushed the pr-make-boldness-additive branch from a4604c7 to 24a0036 Compare May 31, 2024 06:37

chrisbobbe force-pushed the pr-make-boldness-additive branch from 24a0036 to 0e911fe Compare May 31, 2024 06:41

gnprice reviewed Jun 5, 2024

View reviewed changes

chrisbobbe added 9 commits June 5, 2024 12:23

text [nfc]: Use wghtFromFontWeight in a place where it makes sense

7a2861e

Not because we want to make changes that apply here, but just because this fits conceptually, and this function's dartdoc may be useful for the reader.

text: Add wghtFromTextStyle

1840f5e

text [nfc]: Give bolderWght a by param

4ee051e

content test: Make group for strong (bold)

4909fed

content test [nfc]: Pull out some helpers in UserMention tests to use…

a7b317b

… soon

content test: Start testing font weight

01555e8

text: Add bolderWghtTextStyle

f02c867

content: Make **bold** bolder even in spoiler headers and h1/h2/etc.

6d246ae

Fixes: zulip#705

chrisbobbe force-pushed the pr-make-boldness-additive branch 2 times, most recently from ac5d18b to a659ef8 Compare June 5, 2024 16:46

content [nfc]: Comment about not setting Heading weight relative to c…

1531b9e

…ontext

chrisbobbe force-pushed the pr-make-boldness-additive branch from a659ef8 to 1531b9e Compare June 5, 2024 16:47

chrisbobbe merged commit 1531b9e into zulip:main Jun 5, 2024
1 check passed

chrisbobbe deleted the pr-make-boldness-additive branch June 5, 2024 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

content: Make bold bolder even in spoiler headers and h1/h2/etc. #706

content: Make bold bolder even in spoiler headers and h1/h2/etc. #706

chrisbobbe commented May 24, 2024 •

edited

Loading

chrisbobbe commented May 24, 2024 •

edited

Loading

chrisbobbe commented May 24, 2024

gnprice left a comment

gnprice May 28, 2024

chrisbobbe May 31, 2024

gnprice May 28, 2024

gnprice May 28, 2024

gnprice May 28, 2024

gnprice May 28, 2024

gnprice May 28, 2024

gnprice May 28, 2024

chrisbobbe commented May 31, 2024

chrisbobbe commented May 31, 2024

gnprice left a comment

gnprice Jun 5, 2024

chrisbobbe commented Jun 5, 2024

	double? result = style.fontVariations?.singleWhereOrNull((v) => v.axis == 'wght')?.value;
	double? result = style.fontVariations?.firstWhereOrNull((v) => v.axis == 'wght')?.value;

-  /// To check font weight, set up tests with and without bold-text setting.
+  /// Test the font weight found by [styleFinder] in the rendering of [content].
+  ///
+  /// The weight will be expected to be [expectedWght] when the system
+  /// bold-text setting is not set, and to vary appropriately when it is set.

		return _buildStrong(node);
		return _buildStrong(ambientTextStyle: widget.style, node);

		// bold
		content: plainContent('<$name><strong>bold</strong></$name>'),

content: Make **bold** bolder even in spoiler headers and h1/h2/etc. #706

content: Make **bold** bolder even in spoiler headers and h1/h2/etc. #706

Conversation

chrisbobbe commented May 24, 2024 • edited Loading

chrisbobbe commented May 24, 2024 • edited Loading

chrisbobbe commented May 24, 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

chrisbobbe commented May 31, 2024

chrisbobbe commented May 31, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jun 5, 2024

content: Make bold bolder even in spoiler headers and h1/h2/etc. #706

content: Make bold bolder even in spoiler headers and h1/h2/etc. #706

chrisbobbe commented May 24, 2024 •

edited

Loading

chrisbobbe commented May 24, 2024 •

edited

Loading