Skip to content
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.

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

fix: non-spec emphasis (fix #1806 #2708) #2742

Closed
wants to merge 1 commit into from

Conversation

kshnurov
Copy link
Contributor

#2572 introduced a dirty fix for a very rare non-spec emphasis and lead to many regular emphasis being replaced with html tags, which is no good (#1806 #2708).
This PR fixes that by making sure only non-compliant emphasis will be replaced with html tags.

Please check if the PR fulfills these requirements

  • It's the right issue type on the title
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • It does not introduce a breaking change or has a description of the breaking change

@jwlee1108
Copy link
Contributor

@jajugoguma check, please.
@kshnurov Thank you for your contribution. It will take a long time to check this because we are working on other projects. we'll check this PR by mid-October.

@kshnurov
Copy link
Contributor Author

kshnurov commented Sep 26, 2022

After doing some testing I've found some very common cases that are completely broken either in master:

    Expected: "test ***test another* test**"
    Received: "test ***test another* test</strong>"
    Expected: "test **test [some](link)** test"
    Received: "test <strong>test [some](link)** test"

or in this PR:

    Expected: "test **test [some](link).** test"
    Received: "test **test [some](link).</strong> test"

Looking at how this editor works with marks and opening/closing tags I don't think it's possible to fix that without a major rework.
I suggest reverting #2572 completely and ignore the rare cases it was introduced for, and here's why:

  1. This is a WYSIWYG editor, it's output will be used on the web, which means it will be converted to HTML and that HTML will be valid. There's much more broken things with fix: convert by using HTML tag when it is not between spaces #2572 in place than with having non-spec markdown in some very rare cases (which will be rendered correctly anyway).
  2. I've checked how CKEditor works with such non-spec emphasis - it ignores them, i.e. produces non-spec markdown but valid HTML.

@jwlee1108 @jajugoguma what do you think? If you agree, please revert #2572 and release a new version.

@kshnurov
Copy link
Contributor Author

@jwlee1108 @jajugoguma it's been a month, how's it going? :)

@kshnurov
Copy link
Contributor Author

@jwlee1108 @jajugoguma 2 months. There's 200+ issues labeled as bug. Shouldn't this repo be marked as unmaintained, so people won't rely on it in production?

benediktwerner added a commit to benediktwerner/lila that referenced this pull request Dec 21, 2022
benediktwerner added a commit to benediktwerner/lila that referenced this pull request Dec 21, 2022
@jpradelle
Copy link

I think @kshnurov is right, #2572 should be reverted.
I think having HTML in markdown output is an issue, or at least it would be great to have a strict markdown mode without any HTML in it.

@kshnurov
Copy link
Contributor Author

Resolved in #2754

@kshnurov kshnurov closed this Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants