content: Handle multiple math blocks in `<p>` #1156

rajveermalviya · 2024-12-13T15:35:15Z

PIG208 · 2025-01-02T02:29:19Z

My test message renders like this:

Screenshot

I think this is a result of parsing each span as a MathBlockNode, which is probably fine, but visually looks a bit off with all the borders. Making an alternative design should be out of scope for the fix, though.

PIG208 · 2025-01-02T02:17:42Z

lib/model/content.dart

+      // it seems like a glitch in the server's Markdown processing,
+      // so hopefully there just aren't any further such glitches.
+      if (child case dom.Element(localName: 'br')) continue;
+      if (child case dom.Text(text: '\n' || '\n\n')) continue;


Looks like this no longer checks if the <br>\n is at the end. Would it be unexpected to have them appear in the middle?

\n\n on the other hand, appear to be somehow separate from the quirk commented here.

Separated the handling of both these cases.

PIG208 · 2025-01-02T02:31:23Z

test/model/content_test.dart

+        '<span class="katex-html" aria-hidden="true"><span class="base"><span class="strut" style="height:0.6944em;"></span><span class="mord mathnormal">b</span></span></span></span></span></p>', [
+      MathBlockNode(texSource: 'a'),
+      MathBlockNode(texSource: 'b'),
+    ]);


Let's also have a test with multiple math blocks in a quote.

PIG208 · 2025-01-02T02:33:04Z

Thanks for the PR! Apparently I've missed this one as it has been sitting for some time. Overall this works fine for me. Left some comments.

rajveermalviya · 2025-01-06T17:51:41Z

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

PIG208

Looks good to me! Left some small comments.

PIG208 · 2025-01-07T02:59:21Z

test/model/content_test.dart

@@ -522,6 +539,26 @@ class ContentExample {
      '<br>\n</p>\n</blockquote>',
    [QuotationNode([MathBlockNode(texSource: r'\lambda')])]);

+  static const mathBlocksMultipleInQuote = ContentExample(
+    'math blocks, multiple in quote',
+    "````quote\n```math\na\n\nb```````",


Need a \n between ``` and ````

"````quote\n```math\na\n\nb```\n````"

PIG208 · 2025-01-07T03:05:45Z

lib/model/content.dart

+      // each node is interleaved by '\n\n'. Whitespaces are ignored in HTML
+      // on web but each node has `display: block`, which renders each node
+      // on a newline. So do the same here.
+      if (child case dom.Text(text: '\n\n')) continue;


I can see how the first sentence is relevant. Regarding "do the same here", continue itself does not seem to address the part about rendering each node with display: block, as that is more relevant to blocks.add(MathBlockNode(texSource: texSource, debugHtmlNode: debugHtmlNode));.

Added more info about how it tries to replicate web behavior.

PIG208 · 2025-01-08T01:59:29Z

Looks good to me! Thanks for the update.

gnprice

Thanks @rajveermalviya for taking care of this, and thanks @PIG208 for the previous reviews!

Generally this looks good. Comments below, including one fun edge case in the actual content syntax.

gnprice · 2025-01-10T04:19:34Z

lib/model/content.dart

+        result.addAll(parseMathBlocks(node));
+        continue;
+      }


The state this loop maintains is a bit more complex than this — there's result but also imageNodes, and when adding to result we need to flush imageNodes first, like is done at the bottom of the loop body.

For an example message where that makes a difference:
https://chat.zulip.org/#narrow/channel/7-test-here/topic/Greg/near/2035891

https://upload.wikimedia.org/wikipedia/commons/7/78/Verregende_bloem_van_een_Helenium_%27El_Dorado%27._22-07-2023._%28d.j.b%29.jpg ```math a ``` https://upload.wikimedia.org/wikipedia/commons/thumb/7/71/Zaadpluizen_van_een_Clematis_texensis_%27Princess_Diana%27._18-07-2023_%28actm.%29_02.jpg/1280px-Zaadpluizen_van_een_Clematis_texensis_%27Princess_Diana%27._18-07-2023_%28actm.%29_02.jpg

It should be image, then math block, then image, but this version makes it math block, then a row with both images.

For another comparison, see the _isPossibleInlineNode case in the function above this, which has a loop with a very similar structure.

Speaking of which, let's make the same changes in that other method parseImplicitParagraphBlockContentList as in this method parseBlockContentList, so that they remain as similar as possible and differ only in the ways they need to differ for their different jobs.

Probably there should be a refactoring in this code's future so that these two methods have less duplication. But that refactoring will be easier if we minimize the extent they've diverged from each other by then.

(I think it's currently not possible to exercise the case where this logic would matter in parseImplicitParagraphBlockContentList, because it seems to be currently not possible to get a math block inside a list item in Zulip. But CommonMark allows it — see this example — and in principle it's a useful capability which probably should be allowed, so perhaps in the future it will be.)

gnprice · 2025-01-10T04:51:18Z

lib/model/content.dart

+        blocks.add(UnimplementedBlockContentNode(htmlNode: child));
+        continue;
+      }
+      blocks.add(MathBlockNode(texSource: texSource, debugHtmlNode: debugHtmlNode));


This debugHtmlNode should be child (or null, depending), rather than element, because child is what this particular MathBlockNode is parsed from.

gnprice · 2025-01-10T04:54:03Z

lib/model/content.dart

@@ -1451,6 +1451,61 @@ class _ZulipContentParser {
    return tableNode ?? UnimplementedBlockContentNode(htmlNode: tableElement);
  }

+  List<BlockContentNode> parseMathBlocks(dom.Element element) {


nit:

Suggested change

List<BlockContentNode> parseMathBlocks(dom.Element element) {

List<BlockContentNode> parseMathBlocks(dom.NodeList nodes) {

After my preceding comment about debugHtmlNode, this function doesn't actually make any use of element except via element.nodes, so this simplifies things a bit.

gnprice · 2025-01-10T05:07:06Z

lib/model/content.dart

+    })());
+    final debugHtmlNode = kDebugMode ? element : null;
+
+    final blocks = <BlockContentNode>[];


My performance-paranoia for this parsing code doesn't like the idea of allocating this list, just to immediately empty it into the surrounding list, particularly in the common case where there's only going to be a single math block in the <p> element. (And here "common case" means "everything but the weird edge case".)

But I think avoiding that would require a larger refactor, so let's go ahead with this way. It's probably fine (that is, it's probably not a big performance impact in practice). And at least it only affects messages with math blocks in them, which isn't a super common feature in the first place.

gnprice · 2025-01-10T05:11:20Z

lib/model/content.dart

+      if (child case dom.Text(text: '\n\n')) continue;
+
+      if (child is! dom.Element) {
+        blocks.add(UnimplementedBlockContentNode(htmlNode: child));
+        continue;
+      }
+      if (child.className != 'katex-display') {
+        blocks.add(UnimplementedBlockContentNode(htmlNode: child));
+        continue;


My performance-paranoia about this code also doesn't like having all these checks for the first child, where we already know the answers to them; particularly since that'll almost always be the only child.

But for this one, there is a clean solution, so let's do that. Before the loop, operate on just the first child; it'll need an as dom.Element, but none of the rest of these checks. And it'll duplicate the last few lines of the loop body, but that's OK.

Then the loop runs from 1 to < length, so will normally have no iterations at all.

Added the suggested optimization for the first child.

rajveermalviya · 2025-02-12T14:31:08Z

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

gnprice

Thanks for the revision! This logic generally all looks good; small comments below.

Would you also split the changes into two commits? I think that would help with thinking through exactly what cases are affected. Specifically:

A first commit is nearly NFC, and moves the logic from parseBlockContent up to its callers. I think this can be NFC apart from some unimplemented-content cases. (It wouldn't change which possible messages contain unimplemented content, but might change the details of what the unimplemented nodes look like.)
Then the second commit makes the desired substantive change here, supporting multiple blocks in one p element.

gnprice · 2025-02-13T03:08:15Z

lib/model/content.dart

+      if (child case dom.Element(localName: 'span', className: 'katex-display')) {
+        final texSource = _parseMath(child, block: true);
+        if (texSource != null) {
+          result.add(MathBlockNode(texSource: texSource, debugHtmlNode: child));


Ah, #1156 (comment) was maybe ambiguous: this should be child when in debug mode (rather than the parent node as it was in the previous revision), but debugHtmlNode should always be null when not in debug mode.

That way we avoid holding onto these DOM nodes after we're done parsing the content — I think they could be a significant amount of memory.

gnprice · 2025-02-13T04:02:09Z

lib/model/content.dart

-            // The case with the `<br>\n` can happen when at the end of a quote;
-            // it seems like a glitch in the server's Markdown processing,
-            // so hopefully there just aren't any further such glitches.
-            return parseMathBlock(child);


Was this the only call site to parseMathBlock? That can be removed, then.

(Doing so in the same commit as the other changes is helpful for understanding those other changes, even, because it makes visible in the same diff the old code that the new code corresponds to.)

gnprice · 2025-02-13T04:09:55Z

lib/model/content.dart

+      // on web but each node has `display: block`, which renders each node
+      // on a newline. Since the emitted MathBlockNode are BlockContentNode,
+      // we skip these newlines here to replicate the same behavior as on web.


nit:

Suggested change

// on web but each node has `display: block`, which renders each node

// on a newline. Since the emitted MathBlockNode are BlockContentNode,

// we skip these newlines here to replicate the same behavior as on web.

// on web but each node has `display: block`, which renders each node

// on a new line. Since the emitted MathBlockNode are BlockContentNode,

// we skip these newlines here to replicate the same behavior as on web.

A "newline" is a particular character (or in some contexts a particular pair of characters); a "new line" is just a line that's new (for any of the meanings that "line" or "new" could have, depending on context). So "newline" is right in the last sentence here but not the one before it.

gnprice · 2025-02-13T04:15:18Z

lib/model/content.dart

+      if (node case
+        dom.Element(localName: 'p', nodes: [
+          dom.Element(localName: 'span', className: 'katex-display'), ...])) {
+        if (currentParagraph.isNotEmpty) consumeParagraph();


nit: condition should be indented a level deeper than body (so it doesn't look like the start of the body)

gnprice · 2025-02-13T04:17:28Z

lib/model/content.dart

+      if (node case
+        dom.Element(localName: 'p', nodes: [
+          dom.Element(localName: 'span', className: 'katex-display'), ...])) {


Let's check className on the p element, too, like the existing logic did. In general our parser keeps to specific expectations of how className should look, as well as localName, in order for some HTML to be recognized as a pattern it knows about.

So combined with the nit about indentation:

Suggested change

if (node case

dom.Element(localName: 'p', nodes: [

dom.Element(localName: 'span', className: 'katex-display'), ...])) {

if (node case dom.Element(localName: 'p', className: '', nodes: [

dom.Element(localName: 'span', className: 'katex-display'), ...])) {

gnprice · 2025-02-13T04:23:35Z

lib/model/content.dart

+    if (nodes
+          case [..., dom.Element(localName: 'br'), dom.Text(text: '\n')]
+    ) {


nit: this fits on one line, right?

rajveermalviya · 2025-02-13T07:29:30Z

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

gnprice

Thanks for the revision! This all looks great; just a couple of small comments below about the commit structure.

gnprice · 2025-02-14T05:37:50Z

lib/model/content.dart

+        } else {
+          if (currentParagraph.isNotEmpty) consumeParagraph();
+          if (imageNodes.isNotEmpty) consumeImageNodes();
+          result.add(UnimplementedBlockContentNode(htmlNode: node));
+          continue;


I believe this first commit is not quite NFC because the behavior changes in this case. In the old code, I believe this would end up with a ParagraphNode containing an UnimplementedInlineContentNode (and maybe other nodes), instead of with this one UnimplementedBlockContentNode.

That's the non-NFC-ness I was expecting at #1156 (review) . The change is fine; just the commit message should be adjusted: remove the NFC tag, and have the body explain that it's NFC except for this particular behavior change.

(I did work through the logic just now and satisfy myself that there's no other behavior change in this commit. So this splitting of commits was helpful, thanks.)

gnprice · 2025-02-14T05:41:49Z

lib/model/content.dart

+          if (currentParagraph.isNotEmpty) consumeParagraph();
+          if (imageNodes.isNotEmpty) consumeImageNodes();
+          result.add(parseMathBlock(child));
+          continue;


nit: How about moving the two "if not empty, consume" lines above this if/else, and the continue below it?

That would deduplicate them between the if and the else cases, showing that those parts don't vary. And it would also reduce the diff in the second commit.

rajveermalviya · 2025-02-14T08:16:13Z

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

gnprice · 2025-02-14T23:44:14Z

Thanks! Looks good; merging.

Prepares for zulip#1130, this commit is almost NFC with only difference that, in an error case we previously emitted a `ParagraphNode` containing an `UnimplementedInlineContentNode` (along with any adjacent nodes), now we emit a single `UnimplementedBlockContentNode` instead.

Fixes: zulip#1130

rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label Dec 13, 2024

rajveermalviya assigned PIG208 Dec 30, 2024

PIG208 reviewed Jan 2, 2025

View reviewed changes

rajveermalviya force-pushed the pr-multiple-tex-nodes branch 2 times, most recently from 9d222da to f4dde28 Compare January 6, 2025 17:49

rajveermalviya requested a review from PIG208 January 6, 2025 17:51

PIG208 reviewed Jan 7, 2025

View reviewed changes

rajveermalviya force-pushed the pr-multiple-tex-nodes branch from f4dde28 to 5f0bfa1 Compare January 7, 2025 17:18

PIG208 added the integration review Added by maintainers when PR may be ready for integration label Jan 8, 2025

PIG208 removed their assignment Jan 8, 2025

PIG208 requested a review from gnprice January 8, 2025 01:59

PIG208 removed the maintainer review PR ready for review by Zulip maintainers label Jan 8, 2025

PIG208 assigned gnprice Jan 8, 2025

gnprice reviewed Jan 10, 2025

View reviewed changes

This was referenced Jan 10, 2025

Split inline from block parser class, and other small refactors #1270

Merged

Mark as resolved/unresolved prep 1 #1242

Merged

rajveermalviya force-pushed the pr-multiple-tex-nodes branch 2 times, most recently from 127395d to d072321 Compare February 12, 2025 14:09

rajveermalviya requested a review from gnprice February 12, 2025 14:31

rajveermalviya force-pushed the pr-multiple-tex-nodes branch from d072321 to 84ff50f Compare February 12, 2025 16:37

gnprice reviewed Feb 13, 2025

View reviewed changes

rajveermalviya force-pushed the pr-multiple-tex-nodes branch 3 times, most recently from 664ab54 to b6036c9 Compare February 13, 2025 07:27

rajveermalviya requested a review from gnprice February 13, 2025 07:29

gnprice reviewed Feb 14, 2025

View reviewed changes

rajveermalviya force-pushed the pr-multiple-tex-nodes branch from b6036c9 to 1d737e4 Compare February 14, 2025 08:15

rajveermalviya requested a review from gnprice February 14, 2025 08:16

rajveermalviya added 2 commits February 14, 2025 15:44

content: Handle multiple math blocks in <p>

866faf5

Fixes: zulip#1130

gnprice force-pushed the pr-multiple-tex-nodes branch from 1d737e4 to 866faf5 Compare February 14, 2025 23:46

gnprice merged commit 866faf5 into zulip:main Feb 14, 2025

rajveermalviya deleted the pr-multiple-tex-nodes branch February 17, 2025 07:13

	List<BlockContentNode> parseMathBlocks(dom.Element element) {
	List<BlockContentNode> parseMathBlocks(dom.NodeList nodes) {

content: Handle multiple math blocks in <p> #1156

content: Handle multiple math blocks in <p> #1156

Conversation

rajveermalviya commented Dec 13, 2024

PIG208 commented Jan 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Jan 2, 2025

rajveermalviya commented Jan 6, 2025

PIG208 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

PIG208 commented Jan 8, 2025

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

Choose a reason for hiding this comment

rajveermalviya commented Feb 12, 2025

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

rajveermalviya commented Feb 13, 2025

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

rajveermalviya commented Feb 14, 2025

gnprice commented Feb 14, 2025

content: Handle multiple math blocks in `<p>` #1156

content: Handle multiple math blocks in `<p>` #1156