Skip to content

KaTeX (1.75/n): Support horizontal offset and pre-req for vertical offsets #1452

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.

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

Merged
merged 7 commits into from
Jul 10, 2025

Conversation

rajveermalviya
Copy link
Member

@rajveermalviya rajveermalviya commented Apr 1, 2025

Pre-req for #1698

Related: #46

Comment on lines 372 to 381
sealed class KatexNode extends ContentNode {
const KatexNode({super.debugHtmlNode});
}

class KatexSpanNode extends KatexNode {
Copy link
Member

Choose a reason for hiding this comment

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

One point that I noticed while developing #1478 (and checking how my draft of that branch interacted with this PR): it'd be good for this commit:
e268041 content: Handle vertical offset spans in KaTeX content

to get split up like so:

  • First, an NFC prep commit introduces the distinction between KatexNode and KatexSpanNode. At this stage, the latter is the only subclass of the former.
  • Then another commit makes the substantive changes this commit is about, including introducing the sibling subclasses KatexVlistNode and KatexVlistRowNode.

One reason that'd be useful is that the split between KatexNode and KatexSpanNode is somewhat nontrivial in itself: some of the references to KatexNode continue to say KatexNode, while others switch to saying KatexSpanNode, so the commit is expressing meaningful information by its choices of which references go which way.

@rajveermalviya rajveermalviya force-pushed the pr-tex-content-2 branch 2 times, most recently from b17033a to 16cb28f Compare April 24, 2025 08:21
@gnprice gnprice added the maintainer review PR ready for review by Zulip maintainers label Apr 29, 2025
@gnprice
Copy link
Member

gnprice commented Apr 29, 2025

It seems like this had slipped through the cracks :-) but it looks ready for review, so I applied the label.

Also rebased now that #1478 is merged, so this now contains only the changes that are specific to this PR.

Copy link
Member

@PIG208 PIG208 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I think the last few commits will need some tests for the more complicated parts. I went over the changes and left some comments, but haven't extensively tested the changes manually yet.

Comment on lines 525 to 526
_logError('KaTeX: Unsupported CSS property: $property of '
'type ${expression.runtimeType}');
Copy link
Member

Choose a reason for hiding this comment

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

One other possibility for this to be logged is when _getEm returns null. I think for debugging purpose, including the value expression might be helpful.

if (expression is css_visitor.EmTerm && expression.value is num) {
return (expression.value as num).toDouble();
}
return null;
Copy link
Member

Choose a reason for hiding this comment

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

Let's perhaps have a short dartdoc on what this does and what null means, since the if (…Em != null) continue;'s make this return value quite relevant.

Comment on lines 570 to 579
final double? heightEm;
final double? marginRightEm;
final double? topEm;
final double? verticalAlignEm;
Copy link
Member

Choose a reason for hiding this comment

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

I think it's a good idea to separate them into groups; to add on this, how about having a short comment before each group explaining how we group them together (like we do with design variables)?

}

return Container(
margin: styles.marginRightEm != null && !styles.marginRightEm!.isNegative
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, !styles.marginRightEm!.isNegative seems a bit contrived. I think isNegative negated is not as clear as >= 0. However, do we need the margin when marginRightEm is 0?

Comment on lines 481 to 480
styles: inlineStyles != null
? styles.merge(inlineStyles)
: styles,
Copy link
Member

Choose a reason for hiding this comment

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

Having tests for this (the case when there are both inline styles and others) might be useful. Not sure how common that is.

Comment on lines 987 to 998
child: RichText(text: TextSpan(
children: List.unmodifiable(row.nodes.map((e) {
return WidgetSpan(
alignment: PlaceholderAlignment.baseline,
baseline: TextBaseline.alphabetic,
child: switch (e) {
KatexSpanNode() => _KatexSpan(e),
KatexVlistNode() => _KatexVlist(e),
KatexNegativeMarginNode() => _KatexNegativeMargin(e),
});
})))));
Copy link
Member

Choose a reason for hiding this comment

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

looks like this can be replaced with _KatexNodeList(nodes: row.nodes)

Comment on lines 1013 to 1024
child: Text.rich(TextSpan(
children: List.unmodifiable(node.nodes.map((e) {
return WidgetSpan(
alignment: PlaceholderAlignment.baseline,
baseline: TextBaseline.alphabetic,
child: switch (e) {
KatexSpanNode() => _KatexSpan(e),
KatexVlistNode() => _KatexVlist(e),
KatexNegativeMarginNode() => _KatexNegativeMargin(e),
});
})))));
Copy link
Member

Choose a reason for hiding this comment

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

_KatexNodeList also seems helpful here.

}

class _KatexNegativeMargin extends StatelessWidget {
const _KatexNegativeMargin(this.node);
Copy link
Member

Choose a reason for hiding this comment

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

Because the name implies that the margin should be negative, maybe we should also add assertions to ensure that that is true.

Comment on lines 125 to 180
List<KatexNode> _parseChildSpans(dom.Element element) {
return List.unmodifiable(element.nodes.map((node) {
if (node case dom.Element(localName: 'span')) {
return _parseSpan(node);
} else {
var resultSpans = <KatexNode>[];
for (final node in element.nodes.reversed) {
if (node is! dom.Element || node.localName != 'span') {
throw KatexHtmlParseError();
}
}));

final span = _parseSpan(node);
resultSpans.add(span);

if (span is KatexSpanNode) {
final marginRightEm = span.styles.marginRightEm;
if (marginRightEm != null && marginRightEm.isNegative) {
final previousSpansReversed =
resultSpans.reversed.toList(growable: false);
resultSpans = [];
resultSpans.add(KatexNegativeMarginNode(
marginRightEm: marginRightEm,
nodes: previousSpansReversed));
}
}
}

return resultSpans.reversed.toList(growable: false);
}
Copy link
Member

Choose a reason for hiding this comment

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

Neat solution! A bit tricky to understand. I think we can use QueueList (and with .addFirst) to avoid the hassle of reversing and unreversing the list.

Copy link
Member

Choose a reason for hiding this comment

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

It will be helpful to have tests for this.

final pstrutStyles = _parseSpanInlineStyles(pstrutSpan)!;
final pstrutHeight = pstrutStyles.heightEm ?? 0;

// TODO handle negative right-margin inline style on row nodes.
Copy link
Member

Choose a reason for hiding this comment

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

Do we consider this complete after the final commit ("content: Support negative right-margin on KaTeX spans")? I'm not too sure if something else is left to be done here.

This seems to contradict with the earlier assumption that vlist elements contain only the height style.

@rajveermalviya rajveermalviya force-pushed the pr-tex-content-2 branch 4 times, most recently from 49d724d to c917d14 Compare May 19, 2025 18:10
@rajveermalviya
Copy link
Member Author

Thanks for the review @PIG208! Pushed a new revision, PTAL.

I removed the commit for supporting negative margins from this PR, and will create a separate PR that introduces it again.

@rajveermalviya rajveermalviya requested a review from PIG208 May 20, 2025 17:59
Copy link
Member

@PIG208 PIG208 left a comment

Choose a reason for hiding this comment

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

Thanks! Left some comments.

While testing this, I ran into some issues with text scaling (the relevant commit is "content: Scale inline KaTeX content based on the surrounding text"); posted screenshots in one of the comments below.

Otherwise, this works pretty well!

Comment on lines 222 to 223
? List<String>.unmodifiable(element.className.split(' '))
: const <String>[];
Copy link
Member

Choose a reason for hiding this comment

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

nit: indentation

(ContentExample.mathBlockKatexVertical5, [
('a', Offset(0.0, 4.16), Size(10.88, 25.0)),
('b', Offset(10.88, -0.65), Size(8.82, 25.0)),
('c', Offset(19.70, 4.16), Size(8.90, 25.0)),
Copy link
Member

Choose a reason for hiding this comment

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

Interesting setup! This resembles the golden tests that use images instead of offset/size values to verify rendering results.

Future.value(fontFile.readAsBytesSync().buffer.asByteData());
await (FontLoader(fontName)..addFont(bytes)).load();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: newline at the end

@@ -1406,3 +1475,21 @@ void main() {
});
});
}

Future<void> _loadKatexFonts() async {
Copy link
Member

Choose a reason for hiding this comment

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

We have a similar thing in emoji_reactoins_test.dart:

  Future<void> prepare() async {
    addTearDown(testBinding.reset);
    await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
    store = await testBinding.globalStore.perAccount(eg.selfAccount.id);

    await store.addUser(eg.selfUser);

    // TODO do this more centrally, or put in reusable helper
    final Future<ByteData> font = rootBundle.load('assets/Source_Sans_3/SourceSans3VF-Upright.otf');
    final fontLoader = FontLoader('Source Sans 3')..addFont(font);
    await fontLoader.load();
  }

Sharing the helper is not necessary for this PR, but it looks like something we can extract to reuse in the future.

break;

// TODO handle skipped class declarations between .mspace and
// .thinbox .
Copy link
Member

Choose a reason for hiding this comment

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

nit: should this be .msupsub?

@@ -850,8 +860,7 @@ class _Katex extends StatelessWidget {
return Directionality(
textDirection: TextDirection.ltr,
child: DefaultTextStyle(
style: kBaseKatexTextStyle.copyWith(
color: ContentTheme.of(context).textStylePlainParagraph.color),
style: mkBaseKatexTextStyle(textStyle),
Copy link
Member

Choose a reason for hiding this comment

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

Not entirely sure why, but math nested in headings doesn't seem to be visible:

@@ -850,8 +860,7 @@ class _Katex extends StatelessWidget {
return Directionality(
textDirection: TextDirection.ltr,
child: DefaultTextStyle(
style: kBaseKatexTextStyle.copyWith(
color: ContentTheme.of(context).textStylePlainParagraph.color),
style: mkBaseKatexTextStyle(textStyle),
Copy link
Member

Choose a reason for hiding this comment

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

Another issue is that the math content seems to get scaled more than normal text, and the smaller the text is (in normal font size), the more scaled it gets.

normal larger font
image image

Comment on lines 190 to 195
rows.add(KatexVlistRowNode(
verticalOffsetEm: topEm + pstrutHeight,
node: KatexSpanNode(
styles: styles,
text: null,
nodes: _parseChildSpans(otherSpans))));
Copy link
Member

Choose a reason for hiding this comment

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

nit: indentation

return widget;

if (styles.verticalAlignEm != null && styles.heightEm != null) {
assert(widget == defaultWidget);
Copy link
Member

Choose a reason for hiding this comment

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

I think this assertion is based on the broader assumption that some inline styles never co-occur to vertical-align or height. This seems like something we can comment on.

@gnprice gnprice assigned chrisbobbe and unassigned PIG208 May 29, 2025

fontSize = 4.976 * fontSize;
checkKatexText(tester, '2',
fontFamily: 'KaTeX_Main',
fontSize: fontSize,
fontHeight: kBaseKatexTextStyle.height!);
fontHeight: baseTextStyle.height!);
});

testWidgets('displays KaTeX content with different delimiter sizing', (tester) async {
Copy link
Member

Choose a reason for hiding this comment

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

+    // TODO: Re-enable this test after adding support for parsing
+    //       `vertical-align` in inline styles. Currently it fails
+    //       because `strut` span has `vertical-align`.
+    //
+    // testWidgets('displays KaTeX content with different delimiter sizing', (tester) async {

Instead of commenting out the whole test (which makes the diff big), pass skip: true. For examples, see git grep -A2 skip: test/.

The other reason skip: is better than commenting out is that it makes the test case still exist in the test framework and get counted; the total number of skipped tests is printed after the number of passed and failed tests. That's helpful for occasionally sweeping to confirm we don't have tests accidentally left skipped that might point to live bugs.

styles: KatexSpanStyles(fontSizeEm: 0.5), // .reset-size6.size1
text: '0',
nodes: null),
]),
]),
]);

static final mathBlockKatexNestedSizing = ContentExample(
static const mathBlockKatexNestedSizing = ContentExample(
Copy link
Member

Choose a reason for hiding this comment

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

+  // TODO: Re-enable this test after adding support for parsing
+  //       `vertical-align` in inline styles. Currently it fails
+  //       because `strut` span has `vertical-align`.
+  //
+  // static const mathBlockKatexDelimSizing = ContentExample(

Similarly, use skip: true instead of commenting.

For this test, that requires a bit of setup to make this file's helpers and the "all content examples are tested" check handle the skip: argument. I've just pushed a small added commit at the end of this PR branch to do that:
f9a79d8 content test [nfc]: Enable skips in testParseExample and testParse

So please rebase that to earlier in the branch as needed, and then write:

  testParseExample(ContentExample.mathBlockKatexNestedSizing, skip: true);

@rajveermalviya
Copy link
Member Author

This is ready for review again @chrisbobbe, PTAL.

@rajveermalviya rajveermalviya force-pushed the pr-tex-content-2 branch 6 times, most recently from 9193ba4 to 9dde631 Compare July 8, 2025 00:20
@rajveermalviya
Copy link
Member Author

Thanks for the review @gnprice! Pushed an update, PTAL.

@rajveermalviya rajveermalviya requested a review from gnprice July 8, 2025 00:23
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks for the revision! I've again read through all but the last two commits:
f17ab35 content [nfc]: Remove the inline property in _Katex widget
be0b048 content test: Add offset and size based widget tests for KaTeX content
b14dac2 content: Add a workaround for incorrect sizing in WidgetSpan, for KaTeX
dd64b6e content: Scale inline KaTeX content based on the surrounding text
0a4b6a6 content test: Add widget tests for KaTeX content inside a quote
7d66347 content: Handle 'strut' span in KaTeX content
beec446 content: Handle positive margin-right and margin-left in KaTeX spans
6f85e1a content: Handle 'top' property in KaTeX span inline style

This is a lot closer to merge now — the nontrivial comments below are mostly about asking for more of a test on one of these commits.

Then still ahead are the last two commits, on vlists:
eabeb05 content: Handle vertical offset spans in KaTeX content
9dde631 content: Error message for unexpected CSS class in vlist inner span

Comment on lines 823 to 826
return Center(
child: SingleChildScrollViewWithScrollbar(
scrollDirection: Axis.horizontal,
child: Katex(
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as at #1609 (comment) , from when this commit was in a previous version of #1609.

(But now in this PR presumably I'll see what motivated trying this change.)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, well, I don't see that yet. :-)

What happens if you drop this commit?

Copy link
Member Author

Choose a reason for hiding this comment

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

This commit is pre-req for 4bea0c3.

Allowing to get the rect for Katex widget without the Center(SingleChildScrollViewWithScrollbar( widgets affecting it, otherwise the offsets would take into account the whole test window width.

         (ContentExample.mathBlockKatexSizing, skip: false, [
-          ('1', Offset(0.00, 2.24), Size(25.59, 61.00)),
-          ('2', Offset(25.59, 9.90), Size(21.33, 51.00)),
-          ('3', Offset(46.91, 16.30), Size(17.77, 43.00)),
-          ('4', Offset(64.68, 21.63), Size(14.80, 36.00)),
-          ('5', Offset(79.48, 26.07), Size(12.34, 30.00)),
-          ('6', Offset(91.82, 29.77), Size(10.28, 25.00)),
-          ('7', Offset(102.10, 31.62), Size(9.25, 22.00)),
-          ('8', Offset(111.35, 33.47), Size(8.23, 20.00)),
-          ('9', Offset(119.58, 35.32), Size(7.20, 17.00)),
-          ('0', Offset(126.77, 39.02), Size(5.14, 12.00)),
+          ('1', Offset(334.02, 2.24), Size(25.59, 61.00)),
+          ('2', Offset(359.61, 9.90), Size(21.33, 51.00)),
+          ('3', Offset(380.94, 16.30), Size(17.77, 43.00)),
+          ('4', Offset(398.71, 21.63), Size(14.81, 36.00)),
+          ('5', Offset(413.52, 26.07), Size(12.34, 30.00)),
+          ('6', Offset(425.87, 29.77), Size(10.29, 25.00)),
+          ('7', Offset(436.15, 31.62), Size(9.26, 22.00)),
+          ('8', Offset(445.41, 33.47), Size(8.23, 20.00)),
+          ('9', Offset(453.64, 35.32), Size(7.20, 17.00)),
+          ('0', Offset(460.84, 39.02), Size(5.14, 12.00)),
         ]),

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I see.

OK, please file a follow-up issue to track the bug described in #1609 (comment) . Then this code can just get a TODO comment mentioning that issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I think wrapping SingleChildScrollViewWithScrollbar in Directionality again should fix this potential bug.

diff --git a/lib/widgets/content.dart b/lib/widgets/content.dart
index 396bbbba..27426792 100644
--- a/lib/widgets/content.dart
+++ b/lib/widgets/content.dart
@@ -821,10 +821,12 @@ class MathBlock extends StatelessWidget {
     }
 
     return Center(
-      child: SingleChildScrollViewWithScrollbar(
-        scrollDirection: Axis.horizontal,
-        child: _Katex(
-          nodes: nodes)));
+      child: Directionality(
+        textDirection: TextDirection.ltr,
+        child: SingleChildScrollViewWithScrollbar(
+          scrollDirection: Axis.horizontal,
+          child: _Katex(
+            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.

Sure, that sounds good. Better than leaving a follow-up issue, in fact :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added this fix in 9de7110.

Comment on lines 847 to 848
@visibleForTesting
class Katex extends StatelessWidget {
Copy link
Member

Choose a reason for hiding this comment

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

nit: this name feels a bit general for the public namespace (and fortunately it has only a few mentions, so there's minimal cost to a little verbosity)

Suggested change
@visibleForTesting
class Katex extends StatelessWidget {
@visibleForTesting
class KatexWidget extends StatelessWidget {

child: switch (e) {
KatexSpanNode() => _KatexSpan(e),
});
// Workaround a bug where text inside a WidgetSpan could be scaled
Copy link
Member

Choose a reason for hiding this comment

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

nit: "work around" for the verb

Comment on lines 1266 to 1359
child: _Katex(inline: true, nodes: nodes));
child: Katex(textStyle: widget.style, 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.

Huh, does that do it? I wouldn't expect a quote to interact with this bug — those don't change the text size.

(Rereading, I'm not sure that comment by Zixuan was about the bug this fixes, rather than a different issue. What I'm hoping for in this comment is a test case that would fail if we didn't scale inline TeX to match the surrounding font size. Put another way, a test case that matches what's in the commit message:

content: Scale inline KaTeX content based on the surrounding text

This applies the correct font scaling if the KaTeX content is
inside a header.

)

Comment on lines 578 to 580
(ContentExample.mathBlockInQuote, skip: false, [
('λ', Offset(0.00, 2.24), Size(11.99, 25.00)),
]),
Copy link
Member

Choose a reason for hiding this comment

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

(cont'd from #1452 (comment))

This sort of test may work in that it breaks if the bug is re-introduced — but then the next key thing a test needs to do, when it breaks due to a regression, is to convince that future developer that there really is a regression, and to help them work out what it is. If it doesn't convince them, they're most likely to just update the test to expect the new values, and the test didn't succeed in preventing the regression.

So a test is most effective if it can tell a crisp story about what it's checking, and why its expectations really should be expected. These test cases centered on a list of numbers typically aren't good at that, though they can still be the best available strategy for some things.

For this test case, the potential regression we're looking for is that the font size doesn't stay in sync with the font size of the surrounding text. This test file has a helper function for just that sort of check: checkFontSizeRatio. Can you write a test using that? Then that would make it a lot clearer what the narrative is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the checkFontSizeRatio test squashed in 05b51db.

Comment on lines 1266 to 1359
child: _Katex(inline: true, nodes: nodes));
child: Katex(textStyle: widget.style, 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.

… Also a test that checks a given commit works correctly should be squashed into the same commit 🙂 (or sometimes should come before and get updated in the main commit), rather than come after it.

Comment on lines +581 to +582
(ContentExample.mathBlockKatexSizing, skip: false, [
('1', Offset(0.00, 2.24), Size(25.59, 61.00)),
('2', Offset(25.59, 10.04), Size(21.33, 51.00)),
('3', Offset(46.91, 16.55), Size(17.77, 43.00)),
('4', Offset(64.68, 21.98), Size(14.80, 36.00)),
Copy link
Member

Choose a reason for hiding this comment

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

Huh, it's interesting that in this commit:

content: Scale inline KaTeX content based on the surrounding text

This applies the correct font scaling if the KaTeX content is
inside a header.

the changes that happen in the tests are entirely about changing the vertical offsets of these characters — no changes to the sizes, nor the horizontal offsets. Do you understand why the vertical offsets are changing when the sizes aren't?

In any case, the lack of size-related test changes seems to confirm that this commit (which is about getting the sizes right) still needs testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

The change in offset is because of TextLeadingDistribution.even. Moved it to a separate commit c748bce.

: null,
child: widget);

if (margin != null) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: put the margin computation above next to the spot where margin is used, here


final fontFamily = styles.fontFamily;
final fontSize = switch (styles.fontSizeEm) {
double fontSizeEm => fontSizeEm * em,
null => null,
};
if (fontSize != null) em = fontSize;
Copy link
Member

Choose a reason for hiding this comment

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

Cool, this is a good idea for making sure we don't forget fontSize ?? in any em references below.

Comment on lines 968 to 977
margin = EdgeInsets.zero;
if (marginRight != null) {
assert(marginRight >= 0);
margin += EdgeInsets.only(right: marginRight);
}
if (marginLeft != null) {
assert(marginLeft >= 0);
margin += EdgeInsets.only(left: marginLeft);
Copy link
Member

Choose a reason for hiding this comment

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

Cool, thanks. Does that mean that the marginLeftEm code here never gets exercised? In that case let's have an assert to that effect.

@rajveermalviya rajveermalviya force-pushed the pr-tex-content-2 branch 2 times, most recently from 0bc5ab3 to 04dd753 Compare July 9, 2025 14:13
@rajveermalviya
Copy link
Member Author

Thanks for the review @gnprice! Pushed an update, PTAL.

@rajveermalviya rajveermalviya requested a review from gnprice July 9, 2025 14:56
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks for the revision! These commits all look good now, except a few small comments below:
f17ab35 content [nfc]: Remove the inline property in _Katex widget
4bea0c3 content test: Add offset and size based widget tests for KaTeX content
0c05d58 content: Add a workaround for incorrect sizing in WidgetSpan, for KaTeX
c748bce content: Update base KaTeX text style to be explicit
05b51db content: Scale inline KaTeX content based on the surrounding text
6460981 content: Handle 'strut' span in KaTeX content
f8f8d78 content: Handle positive margin-right and margin-left in KaTeX spans

That leaves the last two commits still ahead for me to review:
2e2e4fa content: Handle vertical offset spans in KaTeX content
04dd753 content: Error message for unexpected CSS class in vlist inner span

Let's split those to a separate PR, then. That way we can merge the rest of these changes very soon.

fontWeight: FontWeight.normal,
fontStyle: FontStyle.normal,
textBaseline: TextBaseline.alphabetic,
leadingDistribution: TextLeadingDistribution.even,
Copy link
Member

Choose a reason for hiding this comment

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

content: Update base KaTeX text style to be explicit

This fixes two potential bugs:

- First about `leadingDistribution` where previously it was taking
the default value of `TextLeadingDistribution.proportional`, now it
uses `TextLeadingDistribution.even` which seems to be the default
strategy used by CSS, and it doesn't look like `katex.scss`
overrides it.

The vertical offsets being updated in tests are because of this fix.

This one is an actual bug, right? (Not just potential.) The changes in those vertical offsets demonstrate it was having a real effect — things were sitting a little too high, and also not quite aligned correctly with each other.

It is a small bug, though: at least in the test data, the differences are all less than one logical pixel (so up to a couple of physical pixels).

Copy link
Member

Choose a reason for hiding this comment

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

- Second about `textBaseline` where on some locale systems the
default value for this could be `TextBaseline.ideographic`, and for
KaTeX we always want `TextBaseline.alphabetic`.

This one I guess is "potential" because I'm not sure whether the two baselines end up having a different effect with the fonts that KaTeX uses.

Comment on lines 968 to 977
margin = EdgeInsets.zero;
if (marginRight != null) {
assert(marginRight >= 0);
margin += EdgeInsets.only(right: marginRight);
}
if (marginLeft != null) {
assert(marginLeft >= 0);
margin += EdgeInsets.only(left: marginLeft);
Copy link
Member

Choose a reason for hiding this comment

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

OK, sounds good.

Comment on lines 823 to 826
return Center(
child: SingleChildScrollViewWithScrollbar(
scrollDirection: Axis.horizontal,
child: Katex(
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I see.

OK, please file a follow-up issue to track the bug described in #1609 (comment) . Then this code can just get a TODO comment mentioning that issue.

Comment on lines +1077 to +1058
await checkFontSizeRatio(tester,
targetHtml: html,
targetFontSizeFinder: (rootSpan) {
Copy link
Member

Choose a reason for hiding this comment

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

Cool, this looks good.

@rajveermalviya rajveermalviya force-pushed the pr-tex-content-2 branch 2 times, most recently from d8b0539 to b05061b Compare July 9, 2025 22:54
@rajveermalviya rajveermalviya changed the title KaTeX (2/n): Support horizontal and vertical offsets for spans KaTeX (1.75/n): Support horizontal offset and pre-req for vertical offsets Jul 9, 2025
@rajveermalviya
Copy link
Member Author

Thanks for the review @gnprice! Pushed an update, PTAL.

I've removed those two last commits from this PR, and sent #1698 for those.

@rajveermalviya rajveermalviya requested a review from gnprice July 9, 2025 22:59
And inline the behaviour for `inline: false` in MathBlock widget.
And remove font and fontsize based tests, as the newer rect
offset/size based tests are more accurate anyway.
This fixes a bug about `leadingDistribution` where previously it
was taking the default value of
`TextLeadingDistribution.proportional`, now it uses
`TextLeadingDistribution.even` which seems to be the default
strategy used by CSS, and it doesn't look like `katex.scss`
overrides it.

The vertical offsets being updated in tests are because of this fix.

Another potential bug fix is about `textBaseline` where on some
locale systems the default value for this could be
`TextBaseline.ideographic`, and for KaTeX we always want
`TextBaseline.alphabetic`.
This applies the correct font scaling if the KaTeX content is
inside a header.
In KaTeX HTML it is used to set the baseline of the content in a
span, so handle it separately here.
@gnprice gnprice force-pushed the pr-tex-content-2 branch from b05061b to 1fe817c Compare July 10, 2025 00:49
@gnprice
Copy link
Member

gnprice commented Jul 10, 2025

Thanks for all the revisions!

This looks good — just rebased, and will merge after CI passes.

@gnprice gnprice merged commit 1fe817c into zulip:main Jul 10, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants