Fix formatted_body being parsed as Markdown #6357
Conversation
Background: Clients write Markdown and convert it to HTML before sending the event. All events are formatted as HTML. However, if an HTML formatted event happened to include markdown characters, Element Android would incorrectly render that markdown. For example, an event with formatted_body: "*test*" should be displayed as literally *test* with no effects, but Element Android incorrectly displayed it as test in italics. This commit fixes this behaviour, making Element Android not parse Markdown in HTML messages. From the perspective of most users it will appear that backslash escapes now work properly (even though this wasn't the real issue).
|
The CI fail isn't my fault. Can I get a code review, please? |
|
great catch 💯 I've also confirmed the fix locally via the /devtools {
"body": "raw text",
"formatted_body": "<b>formatted bold</b> **bold MD tags**",
"msgtype": "m.text",
"format": "org.matrix.custom.html"
}
{
"body": "raw text",
"formatted_body": "<code>__italic__</code> <code>**bold**</code>",
"msgtype": "m.text",
"format": "org.matrix.custom.html"
}
{
"body": "raw text",
"formatted_body": "<h1>html h1</h1>",
"msgtype": "m.text",
"format": "org.matrix.custom.html"
}
{
"body": "raw text",
"formatted_body": "# html h1",
"msgtype": "m.text",
"format": "org.matrix.custom.html"
}
|
| .usePlugin(MarkwonInlineParserPlugin.create(MarkwonInlineParser.factoryBuilderNoDefaults())) | ||
| .usePlugin(object : AbstractMarkwonPlugin() { | ||
| override fun configure(registry: Registry) { | ||
| registry.require(MarkwonInlineParserPlugin::class.java, { plugin: MarkwonInlineParserPlugin -> |
There was a problem hiding this comment.
from my understanding, we can combine the inline parser plugin creation with the configuration
.usePlugin(
MarkwonInlineParserPlugin.create(
MarkwonInlineParser.factoryBuilderNoDefaults().addInlineProcessor(HtmlInlineProcessor()),
)
)
| }) | ||
| .usePlugin(object : AbstractMarkwonPlugin() { | ||
| override fun configureParser(builder: Parser.Builder) { | ||
| builder.enabledBlockTypes(Collections.emptySet()) |
There was a problem hiding this comment.
with kotlin the idiomatic way to declare an empty set is with kotlin.collections emptySet()
As requested in PR feedback.
|
I made the changes you requested and verified on my phone that it still works the same. If you'd like, I can also add code comments to the parts of the file I touched explaining what the code does. I didn't see any other comments in that file, though, so I don't know what your organisation's style is. (Maybe you all rely on git blame instead.) The CI fails are once again not my fault. |
this isn't a blocker for me, comments could be handy here as the code is quite decoupled from its usages, in the future a suite of markdown related instrumentation tests would help us avoid regressions in the area and explicitly call out all the formatting cases |
Initially reported in element-hq#6445. Fixes element-hq#6445. This was a regression from element-hq#6357. The fix is to enable Markwon's HTML entities processor.
Initially reported in element-hq#6445. Fixes element-hq#6445. This was a regression from element-hq#6357. The fix is to enable Markwon's HTML entities processor. Change-Id: I65588d6df24257851490161f4672f7461e6e5fc1
Initially reported in element-hq#6445. Fixes element-hq#6445. This was a regression from element-hq#6357. The fix is to enable Markwon's HTML entities processor. Change-Id: I65588d6df24257851490161f4672f7461e6e5fc1
Type of change
Bugfix
Content
Background: Clients write Markdown and convert it to HTML before sending the event. All events are formatted as HTML. However, if an HTML formatted event happened to include markdown characters, Element Android would incorrectly render that markdown.
For example, an event with formatted_body: "test" should be displayed as literally test with no effects, but Element Android incorrectly displayed it as test in italics.
This commit fixes this behaviour, making Element Android not parse Markdown in HTML messages.
From the perspective of most users it will appear that backslash escapes now work properly (even though this wasn't the real issue).
Motivation and context
#5657
Tests
I created 4 messages with the following formatted_body:
*normal*,<em>italics</em>,* normal,<ul><li>list</li></ul>and I checked that they rendered the same on Android as they do on Desktop. (Desktop is in line with my expectations and does not appear to have any rendering bugs here.)(I tried various more complex tests, but the ones mentioned above are the important ones that reproduce the behaviour and can be used to test this change.)
It might be good to have automated tests here but I don't know how to do those. If you want to do automated tests then I'll help you by submitting all the messages I used for testing as well as their expected outcomes. Markwon itself has a test suite so we could probably copy the structure of that.
But I'd honestly rather this PR was just merged soonish so that I don't have to be as annoyed while I'm using the app normally.
Tested devices
Physical OnePlus 5T, LineageOS 18.1 (Android 11)
Checklist
Notes
Since the changelog.d file contents are most likely to be seen by end users, I put a more natural explanation of the problem in there. The technical explanation about formatted_body and how this problem is really caused is in the commit message. I hope this is all good.
Thanks everybody.
Signed-off-by: Cadence Ember [email protected]