Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9f0757d
katex [nfc]: Consolidate where we cancel out ambient text styles
chrisbobbe Aug 20, 2025
48d4cd5
katex [nfc]: s/textStyle/ambientTextStyle/, like UserMention and Glob…
chrisbobbe Aug 20, 2025
61a118b
content: Include inline styles (italic etc.) in what we pass to Widge…
chrisbobbe Aug 19, 2025
8310d37
content: Make TeX in link spans link-colored
chrisbobbe Aug 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions lib/widgets/content.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -822,7 +822,7 @@ class MathBlock extends StatelessWidget {
child: SingleChildScrollViewWithScrollbar(
scrollDirection: Axis.horizontal,
child: KatexWidget(
textStyle: ContentTheme.of(context).textStylePlainParagraph,
ambientTextStyle: ContentTheme.of(context).textStylePlainParagraph,
nodes: nodes))));
}
}
Expand Down Expand Up @@ -1081,10 +1081,21 @@ class _InlineContentBuilder {
_recognizer = _recognizerStack!.removeLast();
}

InlineSpan _buildNodes(List<InlineContentNode> nodes, {required TextStyle? style}) {
final List<TextStyle> _styleStack = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other potential performance worry here is that we're now allocating these lists for every paragraph, even trivial short paragraphs with no formatting in them.

So that'd also be good to measure the impact of.

If it's nontrivial, then a likely fix would be:

  • Leave widget.style out of the list; only add styles from within the node tree.
  • Make the list nullable, and allocate lazily. (Just like _recognizerStack.)


TextStyle _resolveStyleStack() {
assert(_styleStack.isNotEmpty); // first item is `widget.style`
return _styleStack.reduce((value, element) => value.merge(element));
}

InlineSpan _buildNodes(List<InlineContentNode> nodes, {required TextStyle style}) {
_styleStack.add(style);
final children = nodes.map(_buildNode).toList(growable: false);
_styleStack.removeLast();

return TextSpan(
style: style,
children: nodes.map(_buildNode).toList(growable: false));
children: children);
}

InlineSpan _buildNode(InlineContentNode node) {
Expand Down Expand Up @@ -1123,7 +1134,7 @@ class _InlineContentBuilder {

case UserMentionNode():
return WidgetSpan(alignment: PlaceholderAlignment.middle,
child: UserMention(ambientTextStyle: widget.style, node: node));
child: UserMention(ambientTextStyle: _resolveStyleStack(), node: node));
Comment on lines -1126 to +1137
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contrast between _resolveStyleStack here, and widget.style in other places like for UnicodeEmojiNode and InlineCodeNode, is confusing on first look.

I puzzled over it for a few minutes, basically until I remembered your recent other PR #1820. IIUC, the point is that for a TextSpan, its styles will get automatically merged with those of its ancestors; but a WidgetSpan makes a barrier to that automatic merging.

So that'd be good to clarify. As doc on _resolveStyleStack if nothing else; and maybe add a getter for widget.style, and name that and rename _resolveStyleStack so that they're visibly parallel and the names help indicate why there's alternatives.


case UnicodeEmojiNode():
return TextSpan(text: node.emojiUnicode, recognizer: _recognizer,
Expand All @@ -1145,11 +1156,11 @@ class _InlineContentBuilder {
: WidgetSpan(
alignment: PlaceholderAlignment.baseline,
baseline: TextBaseline.alphabetic,
child: KatexWidget(textStyle: widget.style, nodes: nodes));
child: KatexWidget(ambientTextStyle: _resolveStyleStack(), nodes: nodes));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me a little nervous performance-wise. What if there's a paragraph with 40 different inline math spans in it? (Imagine they're all short, like "x", "a^2", "y = 1", "\sin x".)

I guess it only matters if the whole thing is inside a styled span. But that's not uncommon in math texts: the whole statement of a theorem is commonly set in italics.

Maybe the first thing to do is to just measure, in a profile build, how long it takes to build a paragraph like that. If it's negligible, then great.

If that's more than negligible, then a good solution could be to have _resolveStyleStack memoize the result.


case GlobalTimeNode():
return WidgetSpan(alignment: PlaceholderAlignment.middle,
child: GlobalTime(node: node, ambientTextStyle: widget.style));
child: GlobalTime(node: node, ambientTextStyle: _resolveStyleStack()));

case UnimplementedInlineContentNode():
return _errorUnimplemented(node, context: _context!);
Expand Down Expand Up @@ -1341,7 +1352,7 @@ class GlobalTime extends StatelessWidget {
size: ambientTextStyle.fontSize!,
// (When GlobalTime appears in a link, it should be blue
// like the text.)
color: DefaultTextStyle.of(context).style.color!,
color: ambientTextStyle.color,
ZulipIcons.clock),
// Ad-hoc spacing adjustment per feedback:
// https://chat.zulip.org/#narrow/stream/101-design/topic/clock.20icons/near/1729345
Expand Down
38 changes: 28 additions & 10 deletions lib/widgets/katex.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,36 @@ import 'package:flutter/rendering.dart';

import '../model/content.dart';
import '../model/katex.dart';
import 'content.dart';

/// Creates a base text style for rendering KaTeX content.
///
/// This applies the CSS styles defined in .katex class in katex.scss :
/// This cancels out some attributes that may be ambient from Zulip content
/// (italic, bold, etc.)
/// and applies the CSS styles defined in .katex class in katex.scss :
/// https://github.com/KaTeX/KaTeX/blob/613c3da8/src/styles/katex.scss#L13-L15
///
/// Requires the [style.fontSize] to be non-null.
TextStyle mkBaseKatexTextStyle(TextStyle style) {
return style.copyWith(
fontSize: style.fontSize! * 1.21,
/// Requires the [ambientStyle.fontSize] to be non-null.
TextStyle mkBaseKatexTextStyle(TextStyle ambientStyle) {
return ambientStyle.copyWith(
////// Overrides of our own styles:

// Bold formatting is removed below by setting FontWeight.normal…
// Just for completeness, remove "wght", but it wouldn't do anything anyway
// since KaTeX_Main is not a variable-weight font.
fontVariations: [],
// Italic is removed below.

// Strikethrough is removed below, which affects formatting of rendered
// KatexSpanNodes…but a single strikethrough on the whole KatexWidget will
// be visible as long as it doesn't paint an opaque background. (The line
// "shows through" from an ancestor span.) I think we're happy with this:
// the message author asked for a strikethrough by wrapping the math in ~~,
// but we should render it as one unbroken line, not separate lines on each
// KaTeX span.

////// From the .katex class in katex.scss:

fontSize: ambientStyle.fontSize! * 1.21,
fontFamily: 'KaTeX_Main',
height: 1.2,
fontWeight: FontWeight.normal,
Expand All @@ -31,11 +50,11 @@ TextStyle mkBaseKatexTextStyle(TextStyle style) {
class KatexWidget extends StatelessWidget {
const KatexWidget({
super.key,
required this.textStyle,
required this.ambientTextStyle,
required this.nodes,
});

final TextStyle textStyle;
final TextStyle ambientTextStyle;
final List<KatexNode> nodes;

@override
Expand All @@ -45,8 +64,7 @@ class KatexWidget extends StatelessWidget {
return Directionality(
textDirection: TextDirection.ltr,
child: DefaultTextStyle(
style: mkBaseKatexTextStyle(textStyle).copyWith(
color: ContentTheme.of(context).textStylePlainParagraph.color),
style: mkBaseKatexTextStyle(ambientTextStyle),
child: widget));
}
}
Expand Down
48 changes: 41 additions & 7 deletions test/widgets/content_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,6 +763,14 @@ void main() {
});
});

testWidgets('is italic in italic span', (tester) async {
// Regression test for: https://github.com/zulip/zulip-flutter/issues/1813
await prepareContent(tester,
plainContent('<p><em><span class="user-mention" data-user-id="13313">@Chris Bobbe</span></em></p>'));
final style = mergedStyleOf(tester, '@Chris Bobbe');
check(style!.fontStyle).equals(FontStyle.italic);
});

testFontWeight('silent or non-self mention in plain paragraph',
expectedWght: 400,
// @_**Greg Price**
Expand Down Expand Up @@ -989,13 +997,22 @@ void main() {

testContentSmoke(ContentExample.mathInline);

final mathInlineHtml = '<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"><span class="strut" style="height:0.6944em;"></span><span class="mord mathnormal">λ</span></span></span></span>';

testWidgets('is link-colored in link span', (tester) async {
// Regression test for: https://github.com/zulip/zulip-flutter/issues/1823
await prepareContent(tester,
plainContent('<p><a href="https://example/">$mathInlineHtml</a></p>'));
final style = mergedStyleOf(tester, 'λ');
check(style!.color).equals(const HSLColor.fromAHSL(1, 200, 1, 0.4).toColor());
});

testWidgets('maintains font-size ratio with surrounding text', (tester) async {
const html = '<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"><span class="strut" style="height:0.6944em;"></span><span class="mord mathnormal">λ</span></span></span></span>';
await checkFontSizeRatio(tester,
targetHtml: html,
targetHtml: mathInlineHtml,
targetFontSizeFinder: (rootSpan) {
late final double result;
rootSpan.visitChildren((span) {
Expand Down Expand Up @@ -1081,7 +1098,19 @@ void main() {
check(find.textContaining(renderedTextRegexpTwelveHour)).findsOne();
});

void testIconAndTextSameColor(String description, String html) {
testWidgets('is italic in italic span', (tester) async {
// Regression test for: https://github.com/zulip/zulip-flutter/issues/1813
await prepareContent(tester,
// We use the self-account's time-format setting.
wrapWithPerAccountStoreWidget: true,
initialSnapshot: eg.initialSnapshot(),
plainContent('<p><em>$timeSpanHtml</em></p>'));
final style = mergedStyleOf(tester,
findAncestor: find.byType(GlobalTime), renderedTextRegexp);
check(style!.fontStyle).equals(FontStyle.italic);
});

void testIconAndTextSameColor(String description, String html, {Color? expectedColor}) {
testWidgets('clock icon and text are the same color: $description', (tester) async {
await prepareContent(tester,
// We use the self-account's time-format setting.
Expand All @@ -1097,11 +1126,16 @@ void main() {
check(textColor).isNotNull();

check(icon).color.isNotNull().isSameColorAs(textColor!);
if (expectedColor != null) {
check(icon).color.equals(expectedColor);
}
});
}

testIconAndTextSameColor('common case', '<p>$timeSpanHtml</p>');
testIconAndTextSameColor('inside link', '<p><a href="https://example/">$timeSpanHtml</a></p>');
// Regression test for: https://github.com/zulip/zulip-flutter/issues/1819
testIconAndTextSameColor('inside link', '<p><a href="https://example/">$timeSpanHtml</a></p>',
expectedColor: const HSLColor.fromAHSL(1, 200, 1, 0.4).toColor());

group('maintains font-size ratio with surrounding text', () {
Future<void> doCheck(WidgetTester tester, double Function(GlobalTime widget) sizeFromWidget) async {
Expand Down