Markdown parses includes commas behind inline URLs into the link

[almereyda]

Steps to reproduce

1. Write an inline link into a Hypothesis comment, and finish with a comment behind it, e.g.
   http://dataprotocols.org/couchdb-replication/,
2. Save the comment.

Expected behaviour

The comma should be excluded from the inline link.

Actual behaviour

The comma is included in the links href, and also underlined together with the link.

Browser/system information

• Firefox 98
• Fedora 35
• Diego's Unofficial Hypothesis extension

Additional details

https://hyp.is/2D6mvKbXEey0Jncw11speg/github.com/upsiflu/zine-store/blob/723456412f804c7260a066799f10728400787334/DOMAIN.md,

See? GitHub does also not include the comma after the link.

This probably also accounts for other characters, like :, ;... . I couldn't find any other suitable prior art from other implementations through searching for ``, as the discussion in

• regex - Can a URL contain a semicolon and still be valid? - Stack Overflow

which gives a rough overview of the thematics at hands.

Might it be safe to ignore certain characters at the end of inline URLs? How do other implementations solve this?

• https://url.spec.whatwg.org/#concept-basic-url-parser

[robertknight]

There is a specification for the variant of Markdown that GitHub uses, which specifically describes its autolink extension:

Trailing punctuation (specifically, ?, !, ., ,, :, *, _, and ~) will not be considered part of the autolink, though they may be included in the interior of the link:

The Hypothesis client uses Showdown for markdown rendering and auto-linking is handled by the simplifiedAutoLink option.

[acelaya]

I have just checked this, and it is no longer happening.

Showdown has a excludeTrailingPunctuationFromURLs option than can be set to true together with simplifiedAutoLink, which solves this, but its currently deprecated because it became the default behavior at some point, so we can close this issue.

As reference: https://github.com/showdownjs/showdown/commit/d3ebff7ef0cde5abfc3874463946d5297fc82e78#diff-876a5e0e5f41ab00063c898d873337ad0ca0682023a436b82a50ef1d4f95fd3aR25

[acelaya]

This bug is reproducible again. I though we might have downgraded showdown, but that's not the case.

After carefully checking showdown release notes I have realized this feature is not yet released (even though it's merged in their main branch), so we still need to use the config flag: https://github.com/hypothesis/client/pull/5165

I'm not sure why it worked for me a couple of days ago 🤷🏼‍♂️