Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Treat lists with a single empty item as plain text, not Markdown. #6833

Merged
merged 6 commits into from
Aug 17, 2023

Conversation

dkasak
Copy link
Member

@dkasak dkasak commented Sep 17, 2021

This allows users to send simple messages such as -, +, * and 2021. without Element Web mangling them into a nonsensical empty list. As soon as any non-whitespace content is added to the item, it switches back to treating it as a list of one item.

Fixes element-hq/element-meta#1265.

Signed-off-by: Denis Kasak [email protected]

The following screenshots were produced by sending the following sequence of messages:

-
- foo
+
+ foo
*
* foo
2021.
2021. foo

Before

image

After

image


Here's what your changelog entry will look like:

✨ Features

Preview: https://6144dbd806a1fba26b2c36ec--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

This allows users to send simple messages such as `-`, `+`, `*` and
`2021.` without Element Web mangling them into a nonsensical empty list.
As soon as any non-whitespace content is added to the item, it switches
back to treating it as a list of one item.

Fixes vector-im/element-web#7631.
@dkasak dkasak requested a review from a team as a code owner September 17, 2021 18:01
@dkasak dkasak added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Sep 17, 2021
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Code-wise this is looking correct, though historically we have wanted to avoid diverging from the commonmark spec, so requesting product review.

@turt2live turt2live requested a review from a team September 20, 2021 14:35
@dkasak
Copy link
Member Author

dkasak commented Sep 21, 2021

Thanks. I'd like to note this can be viewed as not really deviating from CommonMark but as an earlier short-circuit deciding that a given input is not Markdown at all. From a product perspective (IMO), this edge case is not really useful to the user at all and is just a weird/surprising quirk.

@chagai95
Copy link
Contributor

Also interested in this fix, maybe we'll pull it into our fork and test it for a while thanks @dkasak

@t3chguy
Copy link
Member

t3chguy commented Sep 29, 2021

Thanks. I'd like to note this can be viewed as not really deviating from CommonMark but as an earlier short-circuit deciding that a given input is not Markdown at all. From a product perspective (IMO), this edge case is not really useful to the user at all and is just a weird/surprising quirk.

Sure, that's how it can be seen, but someone could be using https://spec.commonmark.org/dingus/ to author and preview what they're about to send and now that won't match, so it is still a deviation

@dkasak
Copy link
Member Author

dkasak commented Sep 29, 2021

Though if we are worried about things like that, we already have far larger deviations.

@t3chguy
Copy link
Member

t3chguy commented Sep 29, 2021

Though if we are worried about things like that, we already have far larger deviations.

Yup but this is considered a defect. e.g element-hq/element-web#18299 and element-hq/element-web#8786 and probably more

@dkasak
Copy link
Member Author

dkasak commented Sep 29, 2021

We're veering quite a bit off-topic, but that's actually a different issue. The linked issues are showing a real defect in which multiple paragraphs are incorrectly joined into a single one. This too is a deviation from Commonmark and you'll note dingus does the right thing here.

What I'm talking about is something else. Markdown is supposed to ignore hard newlines inside a single paragraph and reflow the text, but this is purposefully not done in Element (AFAIK) even though it is also most certainly a deviation from Commonmark.

@dkasak
Copy link
Member Author

dkasak commented Dec 8, 2021

@chagai95: Did you perhaps have a chance to test this in your fork? I'm curious if you have feedback.

@chagai95
Copy link
Contributor

chagai95 commented Dec 8, 2021

Nope, sorry I completely forgot about this I'll did it now and it'll be on protection next week 🙃

@chagai95
Copy link
Contributor

It looks like I can not send these messages at all now

@dkasak
Copy link
Member Author

dkasak commented Mar 13, 2022

Yeah, I've heard that something broke. I need to rebase the PR and figure out what's going on.

@chagai95
Copy link
Contributor

Thanks, I'll revert the change on our prod env but I'll gladly test again if you get around to fixing this, just ping me, thx!

@chagai95
Copy link
Contributor

I ended up just fixing on our end it's very simple, don't know why git got it wrong...

@andybalaam
Copy link
Contributor

@dkasak I just discussed this with @daniellekirkwood and we previously discussed as Element Web team, and we are happy that Element Web's use case is sufficiently different from commonmark's normal use case (EW is often for short, unstructured comments and responses whereas commonmark is often applied to entire documents), that we think this deviation is justified.

So TL/DR if you resolve the conflicts I will happily merge.

@weeman1337 weeman1337 requested review from a team and dbkr and removed request for a team August 3, 2023 14:05
@weeman1337 weeman1337 requested a review from t3chguy August 3, 2023 14:05
@weeman1337 weeman1337 marked this pull request as draft August 3, 2023 14:18
@andybalaam andybalaam added this pull request to the merge queue Aug 17, 2023
Merged via the queue into develop with commit 5aefd46 Aug 17, 2023
19 checks passed
@andybalaam andybalaam deleted the dkasak/list-with-single-empty-item-not-markdown branch August 17, 2023 17:08
@andybalaam
Copy link
Contributor

Thank you, finally merged!

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Aug 31, 2023
Changes in [1.11.40](https://github.com/vector-im/element-web/releases/tag/v1.11.40) (2023-08-29)
=================================================================================================

## ✨ Features
 * Hide account deactivation for externally managed accounts ([\#11445](matrix-org/matrix-react-sdk#11445)). Fixes #26022. Contributed by @kerryarchibald.
 * OIDC: Redirect to delegated auth provider when signing out ([\#11432](matrix-org/matrix-react-sdk#11432)). Fixes #26000. Contributed by @kerryarchibald.
 * Disable 3pid fields in settings when `m.3pid_changes` capability is disabled ([\#11430](matrix-org/matrix-react-sdk#11430)). Fixes #25995. Contributed by @kerryarchibald.
 * OIDC: disable multi session signout for OIDC-aware servers in session manager ([\#11431](matrix-org/matrix-react-sdk#11431)). Contributed by @kerryarchibald.
 * Implement updated open dialog method of the Module API ([\#11395](matrix-org/matrix-react-sdk#11395)). Contributed by @dhenneke.
 * Polish & delabs `Exploring public spaces` feature ([\#11423](matrix-org/matrix-react-sdk#11423)).
 * Treat lists with a single empty item as plain text, not Markdown. ([\#6833](matrix-org/matrix-react-sdk#6833)). Fixes element-hq/element-meta#1265.
 * Allow managing room knocks ([\#11404](matrix-org/matrix-react-sdk#11404)). Contributed by @charlynguyen.
 * Pin the action buttons to the bottom of the scrollable dialogs ([\#11407](matrix-org/matrix-react-sdk#11407)). Contributed by @dhenneke.
 * Support Matrix 1.1 (drop legacy r0 versions) ([\#9819](matrix-org/matrix-react-sdk#9819)).

## 🐛 Bug Fixes
 * Fix path separator for Windows based systems ([\#25997](element-hq/element-web#25997)).
 * Fix instances of double translation and guard translation calls using typescript ([\#11443](matrix-org/matrix-react-sdk#11443)).
 * Fix export type "Current timeline" to match its behaviour to its name ([\#11426](matrix-org/matrix-react-sdk#11426)). Fixes #25988.
 * Fix Room Settings > Notifications file upload input being shown superfluously ([\#11415](matrix-org/matrix-react-sdk#11415)). Fixes #18392.
 * Simplify registration with email validation ([\#11398](matrix-org/matrix-react-sdk#11398)). Fixes #25832 #23601 and #22297.
 * correct home server URL ([\#11391](matrix-org/matrix-react-sdk#11391)). Fixes #25931. Contributed by @NSV1991.
 * Include non-matching DMs in Spotlight recent conversations when the DM's userId is part of the search API results ([\#11374](matrix-org/matrix-react-sdk#11374)). Contributed by @mgcm.
 * Fix useRoomMembers missing updates causing incorrect membership counts ([\#11392](matrix-org/matrix-react-sdk#11392)). Fixes #17096.
 * Show error when searching public rooms fails ([\#11378](matrix-org/matrix-react-sdk#11378)).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lists with a single empty item (i.e. +, -, *, 1234.) shouldn't be considered Markdown
9 participants