content: Fix bugs where `TextSpan` styles clobber each other with `InlineContent.style` attributes #1820

chrisbobbe · 2025-08-19T23:19:57Z

Fixes #806.
Fixes #1812.
Fixes #1817.
Fixes #1818.

There are other issues with inline-span styles:

but I think those all involve WidgetSpans in InlineContent's span tree and can be addressed separately.

Before	After

rajveermalviya

Thanks @chrisbobbe! LGTM, moving over to Greg's review.

gnprice

Thanks for fixing these! Generally all looks good; a couple of nits.

gnprice · 2025-08-20T21:54:38Z

test/widgets/content_test.dart

+    testFontWeight('is bold in bold span',
+      // Regression test for: https://github.com/zulip/zulip-flutter/issues/1812
+      expectedWght: 600,
+      // **bold**
+      content: plainContent('<p><strong><code>bold</code></strong></p>'),


Suggested change

testFontWeight('is bold in bold span',

// Regression test for: https://github.com/zulip/zulip-flutter/issues/1812

expectedWght: 600,

// **bold**

content: plainContent('<code>bold</code>'),

testFontWeight('is bold in bold span',

// Regression test for: https://github.com/zulip/zulip-flutter/issues/1812

expectedWght: 600,

// **`bold`**

content: plainContent('<code>bold</code>'),

right?

gnprice · 2025-08-20T21:59:14Z

test/widgets/content_test.dart

+        content: plainContent('<p><strong><span class="katex">'
+          '<span class="katex-mathml"><math xmlns="http://www.w3.org/1998/Math/MathML"><semantics><mrow><mi>λ</mi></mrow>'
+            '<annotation encoding="application/x-tex"> \\lambda </annotation></semantics></math></span>'
+          '<span class="katex-html" aria-hidden="true">'
+            '<span class="base unknown">' // Server doesn't generate this 'unknown' class.
+            '<span class="strut" style="height:0.6944em;"></span>'
+            '<span class="mord mathnormal">λ</span></span></span></span></strong></p>'),


How about something like:

Suggested change

content: plainContent(''

'<math xmlns="http://www.w3.org/1998/Math/MathML"><semantics><mrow><mi>λ</mi></mrow>'

'<annotation encoding="application/x-tex"> \\lambda </annotation></semantics></math>'

''

'' // Server doesn't generate this 'unknown' class.

''

'λ'),

content: plainContent('$unsupportedKatexHtml'),

where that string is shared between these two new tests and the existing one above? That way it's clear that the bulk of the logic is in common, and one can more easily focus on the bits that differ.

Makes sense; see what you think in my next revision

chrisbobbe · 2025-08-20T22:31:24Z

Thanks for the reviews! Revision pushed.

gnprice

Thanks! Comments below.

gnprice · 2025-08-20T23:35:05Z

test/widgets/content_test.dart

    testWidgets('fallback to displaying KaTeX source if unsupported KaTeX HTML', (tester) async {
-      await prepareContent(tester, plainContent(ContentExample.mathInlineUnknown.html));
-      tester.widget(find.text(r'\lambda'));
+      testContentSmoke(ContentExample.mathInlineUnknown);


This is a test nested in a test, right? So the outer should become a group instead. (Like it does in the next commit anyway.)

gnprice · 2025-08-20T23:37:40Z

test/widgets/content_test.dart

+      testContentSmoke(ContentExample.mathInlineUnknown);
+
+      assert(ContentExample.mathInlineUnknown.html.startsWith('<p>'));
+      assert(ContentExample.mathInlineUnknown.html.endsWith('</p>'));
+      final unsupportedKatexHtml = ContentExample.mathInlineUnknown.html
+        .substring(3, ContentExample.mathInlineUnknown.html.length - 4);


Hmm, I see.

Can this also apply to the test above that has similar HTML?

testWidgets('maintains font-size ratio with surrounding text, when falling back to TeX source', (tester) async { const unsupportedHtml = '' '<math xmlns="http://www.w3.org/1998/Math/MathML"><semantics><mrow><mi>λ</mi></mrow>' '<annotation encoding="application/x-tex"> \\lambda </annotation></semantics></math>' '' '' // Server doesn't generate this 'unknown' class. '' 'λ'; await checkFontSizeRatio(tester, targetHtml: unsupportedHtml, targetFontSizeFinder: mkTargetFontSizeFinderFromPattern(r'\lambda')); });

That's the one I had in mind at #1820 (comment) 🙂 — I guess that was ambiguous.

gnprice · 2025-08-20T23:38:55Z

test/widgets/content_test.dart

+        // **$$ \lambda $$**
+        content: plainContent('<p><strong>$unsupportedKatexHtml</strong></p>'),


nit:

Suggested change

// **$$ \lambda $$**

content: plainContent('$unsupportedKatexHtml'),

content: plainContent('$unsupportedKatexHtml'),

This isn't HTML that we'd expect the server to emit for that input (or any input).

gnprice · 2025-08-20T23:40:05Z

test/widgets/content_test.dart

+        expectedWght: 600,
+        // **$$ \lambda $$**
+        content: plainContent('<p><strong>$unsupportedKatexHtml</strong></p>'),
+        styleFinder: (tester) => mergedStyleOf(tester, r'\lambda')!,


nit: can avoid the need to look up the example to see that \lambda is the right text to look for:

Suggested change

styleFinder: (tester) => mergedStyleOf(tester, r'\lambda')!,

styleFinder: (tester) => mergedStyleOf(tester, ContentExample.mathInlineUnknown.expectedText)!,

(or maybe pull that into a variable next to unsupportedKatexHtml)

chrisbobbe · 2025-08-21T00:21:06Z

Thanks! Revision pushed.

gnprice

Thanks for cleaning up these tests! Just a couple of nits.

gnprice · 2025-08-21T00:23:19Z

test/widgets/content_test.dart


-    testWidgets('displays KaTeX content', (tester) async {


nit:

Looks like the behavior of that test differs from this one, just in not passing `findRichText: true`. But that didn't stop it from finding the expected test and passing, so, shrug.

s/expected test/expected text/ ?

gnprice · 2025-08-21T00:28:35Z

test/widgets/content_test.dart

-          '<span class="mord mathnormal">λ</span></span></span></span>';
-      await checkFontSizeRatio(tester,
-        targetHtml: unsupportedHtml,
-        targetFontSizeFinder: mkTargetFontSizeFinderFromPattern(r'\lambda'));


nit: squash these together:

9cc505f content test [nfc]: Get unsupported-katex HTML from ContentExample
bf823e1 content test [nfc]: Pull out an expectedText variable from ContentExample

since the first one leaves the literal r'\lambda' unexplained

Fixes zulip#1818. The InlineContent widget creates a Text.rich with a tree of `InlineSpan`s, and the widget's `style` param is passed to the root span in that tree. That style's attributes will propagate down the tree as defaults, which means that a non-root node will still be styled with e.g. h1 font size if it doesn't `TextStyle.merge` that root style into its own style, as was happening here. That merging can also be unhelpful, not just unnecessary, because it can clobber attributes that were set at nodes between the current node and the root. This kind of clobbering causes some known issues; the first of these is fixed in this commit, and we'll fix the rest, coming up: - zulip#1818 content: Unicode emoji in strikethrough should have strikethrough line, but doesn't - zulip#1817 content: Bold text in strikethrough should have strikethrough line, but doesn't - zulip#1812 content: Inline code in a bold span should be bold (but isn't) - zulip#806 content: Inline code links should be rendered with the link color We'll fix the others, coming up.

Fixes zulip#1817. Similar reasoning as in the previous commit; see there. The bold span-node was clobbering the strikethrough attribute when that is set by an ancestor strikethrough node. (This is bolderWghtTextStyle's only caller, so it's simple to change its interface this way.)

In the 'inline math' group just above this, we have the following test: testContentSmoke(ContentExample.mathInline); Looks like the behavior of that test differs from this one, just in not passing `findRichText: true`. But that didn't stop it from finding the expected text and passing, so, shrug.

…eX test

NFC because this is the same string.

This just calls `testWidgets` twice; there's no Future that callers should want to await.

Also in rendering inline TeX source when it doesn't parse...but that's decreasingly visible as we implement more TeX. :) Fixes zulip#806. Fixes zulip#1812. Similar reasoning as in the previous commits; see there. The inline-code span-node was clobbering the link-color and font-weight attributes when those are set by ancestor link and emphasis nodes.

chrisbobbe · 2025-08-21T00:32:10Z

Thanks! Revision pushed.

gnprice · 2025-08-21T00:33:29Z

Thanks! Looks good; all clear to merge, pending CI.

chrisbobbe · 2025-08-21T00:53:27Z

Thanks! Done.

chrisbobbe requested a review from rajveermalviya August 19, 2025 23:19

chrisbobbe assigned rajveermalviya Aug 19, 2025

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Aug 19, 2025

rajveermalviya approved these changes Aug 20, 2025

View reviewed changes

rajveermalviya added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Aug 20, 2025

rajveermalviya assigned gnprice and unassigned rajveermalviya Aug 20, 2025

rajveermalviya requested a review from gnprice August 20, 2025 18:22

gnprice reviewed Aug 20, 2025

View reviewed changes

chrisbobbe force-pushed the pr-spans-clobbering-styles-fix branch from b19b946 to 0fe4f77 Compare August 20, 2025 22:31

gnprice reviewed Aug 20, 2025

View reviewed changes

chrisbobbe force-pushed the pr-spans-clobbering-styles-fix branch 2 times, most recently from 7a485d5 to e0e6f12 Compare August 21, 2025 00:20

gnprice reviewed Aug 21, 2025

View reviewed changes

chrisbobbe added 9 commits August 20, 2025 17:31

content test: Add ContentExample.deleted; use in new widget test

0411340

content test [nfc]: Use testContentSmoke shortcut for unsupported KaT…

4b7b1a8

…eX test

content test [nfc]: Move an unsupported-KaTeX test into its group

5095e9a

content test [nfc]: Get unsupported-katex HTML from ContentExample

2acc20c

NFC because this is the same string.

content test [nfc]: Fix testFontWeight's return type

51f3299

This just calls `testWidgets` twice; there's no Future that callers should want to await.

chrisbobbe force-pushed the pr-spans-clobbering-styles-fix branch from e0e6f12 to 06f6281 Compare August 21, 2025 00:32

chrisbobbe merged commit 3dc2669 into zulip:main Aug 21, 2025
1 check passed

chrisbobbe deleted the pr-spans-clobbering-styles-fix branch August 21, 2025 00:53

gnprice mentioned this pull request Aug 21, 2025

content: Include inline styles (italic etc.) in what we pass to WidgetSpans #1821

Open

		// $$ \lambda $$
		content: plainContent('<p><strong>$unsupportedKatexHtml</strong></p>'),

	styleFinder: (tester) => mergedStyleOf(tester, r'\lambda')!,
	styleFinder: (tester) => mergedStyleOf(tester, ContentExample.mathInlineUnknown.expectedText)!,

content: Fix bugs where TextSpan styles clobber each other with InlineContent.style attributes #1820

content: Fix bugs where TextSpan styles clobber each other with InlineContent.style attributes #1820

Uh oh!

Conversation

chrisbobbe commented Aug 19, 2025

Uh oh!

rajveermalviya left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Aug 20, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Aug 21, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Aug 21, 2025

Uh oh!

gnprice commented Aug 21, 2025

Uh oh!

Uh oh!

chrisbobbe commented Aug 21, 2025

Uh oh!

Uh oh!

content: Fix bugs where `TextSpan` styles clobber each other with `InlineContent.style` attributes #1820

content: Fix bugs where `TextSpan` styles clobber each other with `InlineContent.style` attributes #1820