Skip to content

content: Handle blank text nodes after code blocks. #604

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

Merged
merged 1 commit into from
Apr 10, 2024
Merged

content: Handle blank text nodes after code blocks. #604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion lib/model/content.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1124,14 +1124,18 @@ class _ZulipContentParser {
return result;
}

static final _redundantLineBreaksRegexp = RegExp(r'^\n+$');

List<BlockContentNode> parseBlockContentList(dom.NodeList nodes) {
assert(_debugParserContext == _ParserContext.block);
final List<BlockContentNode> result = [];
List<ImageNode> imageNodes = [];
for (final node in nodes) {
// We get a bunch of newline Text nodes between paragraphs.
// A browser seems to ignore these; let's do the same.
if (node is dom.Text && (node.text == '\n')) continue;
if (node is dom.Text && _redundantLineBreaksRegexp.hasMatch(node.text)) {
continue;
}

final block = parseBlockContent(node);
if (block is ImageNode) {
Expand Down
12 changes: 12 additions & 0 deletions test/model/content_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,17 @@ class ContentExample {
'\n</code></pre></div>'),
]);

static const codeBlockFollowedByMultipleLineBreaks = ContentExample(
'blank text nodes after code blocks',
' code block.\n\nsome content',
Copy link
Member

Choose a reason for hiding this comment

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

Interesting! I wonder if this is the explanation for all the times we've seen this symptom. It hasn't felt super common in my experience — which would be consistent with that, because I think this four-space-indent syntax is not super commonly used in Zulip messages (because the triple-backticks syntax is usually more convenient).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I attempted to replicate the issue by copying the messages mentioned in the report. I can confirm that this occurs exclusively when utilizing the four-space-indent syntax. As my attempts to reproduce it using triple-backticks were unsuccessful.

Additionally, it is noteworthy that I couldn't reproduce the issue when typing on the mobile app. It was only after sending the message from the web client that the double line breaks appeared.

Copy link
Member

Choose a reason for hiding this comment

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

Huh interesting. I'd be surprised if the difference between mobile and web has a direct effect on this — both clients are calling the same API to send a message, and if they send the same Markdown content I'd expect the server to turn that into the same HTML. Perhaps the two clients are somehow ending up sending different Markdown content?

If the server does for some reason end up producing different HTML for the same Markdown depending on the client, that'd be a surprising quirk it'd definitely be good to know about. Most likely it'd be a bug we'd want to fix in the server.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Upon further investigation, I've determined that this issue is associated with the 'trim' function utilized prior to dispatching the message in the mobile application:

// lib/widgets/compose_box.dart:69
@override
String _computeTextNormalized() {
  String trimmed = text.trim();
  return trimmed.isEmpty ? kNoTopicTopic : trimmed;
}

I'm uncertain whether this behavior should be altered to align with the web version or if it's acceptable to maintain its current state. I'd recommend creating a new issue to handle this. What are your thoughts, @chrisbobbe?

Copy link
Collaborator

@chrisbobbe chrisbobbe Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the investigation, @Khader-1! It sounds like you've found that the web client doesn't trim leading and trailing whitespace from the raw message content sent in the send-message request. Is that right?

(I think you mean the same method of ComposeContentController:

  @override
  String _computeTextNormalized() {
    return text.trim();
  }

The one you quoted belongs to ComposeTopicController.)

I've always assumed it's a good idea for the client to trim extra whitespace from the start and end of raw message content. It just seems neater that way, to me. If it's true that trimming is preferable (and I might be wrong), I would wonder why the web client doesn't do so. 🙂 I wonder if there's some Markdown syntax that requires leading or trailing whitespace and is defeated when the whitespace is removed. That would be a good reason not to call .trim() as we do currently.

If you want, you could raise the question of why the web app doesn't do trimming, and whether clients should trim or not, on CZO. Perhaps in the #api design stream, because it's about the expectations of the send-message endpoint. But it seems like the answer doesn't need to block progress on the current issue.

// https://chat.zulip.org/#narrow/stream/7-test-here/near/1774823
'<div class="codehilite">'
'<pre><span></span><code>code block.\n</code></pre></div>\n\n'
'<p>some content</p>', [
CodeBlockNode([CodeBlockSpanNode(text: "code block.", type: CodeBlockSpanType.text)]),
ParagraphNode(links: null, nodes: [TextNode("some content")]),
]);

static final mathInline = ContentExample.inline(
'inline math',
r"$$ \lambda $$",
Expand Down Expand Up @@ -865,6 +876,7 @@ void main() {
testParseExample(ContentExample.codeBlockHighlightedMultiline);
testParseExample(ContentExample.codeBlockWithHighlightedLines);
testParseExample(ContentExample.codeBlockWithUnknownSpanType);
testParseExample(ContentExample.codeBlockFollowedByMultipleLineBreaks);

testParseExample(ContentExample.mathBlock);
testParseExample(ContentExample.mathBlockInQuote);
Expand Down
Loading