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

Italic gets replaced by underscore characters if only parts of words are italic #324

Closed
engfragui opened this issue Jun 27, 2023 · 9 comments · Fixed by #490
Closed

Italic gets replaced by underscore characters if only parts of words are italic #324

engfragui opened this issue Jun 27, 2023 · 9 comments · Fixed by #490
Labels
bug Something isn't working good first issue Good for newcomers released Pull requests that have been released to production

Comments

@engfragui
Copy link

engfragui commented Jun 27, 2023

Current behaviour

If I change parts of a sentence to italic, the correct/expected parts of the text look italic in the text editor, however once I submit the text an underscore character shows up and those parts of words are not actually rendered as italic:

249099120-129b2f25-5e6c-43b5-9742-0758b9bf7988.mp4

Expected behaviour

The text appears as italic even after submitting the text (and no underscores appear).

@engfragui engfragui changed the title Italic text replaced by underscore characters around the text if only parts of words are italic Italics replaced by underscore characters if only parts of words are italic Jun 27, 2023
@engfragui engfragui changed the title Italics replaced by underscore characters if only parts of words are italic Italic replaced by underscore characters if only parts of words are italic Jun 27, 2023
@engfragui engfragui changed the title Italic replaced by underscore characters if only parts of words are italic Italic gets replaced by underscore characters if only parts of words are italic Jun 27, 2023
@rfgamaral rfgamaral added bug Something isn't working good first issue Good for newcomers labels Jun 27, 2023
@rfgamaral
Copy link
Member

I'm not exactly sure where the issue lies, but it's most likely related to the new HTML Serializer (ref). Might be worth checking if the old implementation exhibited this issue or not.

Also worth noting that if * was used instead of _ to italicize (i.e. *italic*), this wouldn't be an issue. That to say that this issue is somewhat related to the use of the underscore character.

@MelisUnal
Copy link

Sharing our Japanese translator's findings on this in case it provides some valuable info.

I can reproduce this on Web & Windows. I believe this formatting option is not supported on Android and iOS.

The steps I'm taking: 1. highlight a piece of text, 2. select the formatting option (in this case, italic) from the editor menu that appears.
If a piece of text is English, I can make it Italic, but if it is Japanese, the text remains as is.

Reported in TD copy/bugs reports

@MelisUnal
Copy link

@Doist/frontend-hero Wanted to share with you that our Japanese translator tried using underscore (_) before and after the Japanese text, but unfortunately the text remains as is. For English text, they are able to make it italic using underscore.

@thales-fukuda
Copy link

thales-fukuda commented Oct 10, 2023

I'm not exactly sure where the issue lies, but it's most likely related to the new HTML Serializer (ref). Might be worth checking if the old implementation exhibited this issue or not.

Also worth noting that if * was used instead of _ to italicize (i.e. *italic*), this wouldn't be an issue. That to say that
this issue is somewhat related to the use of the underscore character.

Indeed, I rolled back to b1b6cfd and it had the same bug.

This happens when the italic underscore is in the middle of a word, both marked and remark don't convert it as the asterisk(*) is considered the best practice..

Some solutions for the problem:

A: Change the option emDelimiter of turndown to be *.

It's the easiest but it's probably a no. It would cause breaking changes for someone who is using the library expecting the markdown output to use underscore as the italic character.

B: Look for/create a remark plugin to recognize _ in the middle of words as italics.

This fixes the problem when using the toolbar to italicize text. But it would break if someone writes something in snake case. A text like very_important_task would become veryimportanttask.

C: Create a turndown rule to add * instead of _ for italics inside words.

Maybe the best solution. It reduces the possibility of breaking changes, fixes the problem, and is compatible with snake case.

I would be willing to open a PR with the fix, I already took a look at how to implement it.

@thales-fukuda
Copy link

@Doist/frontend-hero Wanted to share with you that our Japanese translator tried using underscore (_) before and after the Japanese text, but unfortunately the text remains as is. For English text, they are able to make it italic using underscore.

I believe this happens because tiptap doesn't recognize marks inside words when typed or pasted. It's not just italic, if you try to make something inside a word bold by typing it like som**en**thing it doesn't work as well(although it would display as bold after saving on Todoist, as it would be transformed into HTML). I tested both Japanese and English characters and it worked the same.

This can be fixed by creating an extension.

@rfgamaral
Copy link
Member

rfgamaral commented Oct 10, 2023

Hey @thales-fukuda, thank you for showing in interesting in fixing this issue 🙏

Just so I understand solution C, it's basically the same one as A, but in a form of a plugin without changing emDelimiter so that we keep backwards compatibility, correct?

I'm a bit torn between A and C, to be honest. I don't necessarily consider A to be a breaking change because the Markdown output should not be relevant most of the time, it's just the format used to store the content (that you shouldn't directly manipulate, at least in the context that we used Typist at Doist), but as long as parsing the input/output remains the same, and everything works as expected (and it does), I don't consider it a breaking change.

What are your thoughts on that, @thales-fukuda? What about yours, @engfragui? Would love some extra input from someone on the team :)

I would be willing to open a PR with the fix, I already took a look at how to implement it.

I'd be happy to review the PR, but let's first agree on a solution first 👍


This can be fixed by creating an extension.

As for this one, I believe this should be fixed upstream on Tiptap, and not on Typist.

@thales-fukuda
Copy link

@rfgamaral

Just so I understand solution C, it's basically the same one as A, but in a form of a plugin without changing emDelimiter so that we keep backwards compatibility, correct?

Correct. I was not sure about how you guys work with it internally, but if it's not a problem, changing the markdown serializer option would do it.

Also, I was looking at Tiptap issues and apparently, there maybe a problem with Japanese, Chinese, and maybe other Asian IMEs (ueberdosis/tiptap#4499). There is no exploration further into it but it's a good idea to keep a look.

@rfgamaral
Copy link
Member

@thales-fukuda Natalie from our team got assigned to this issue internally, so she took care of it. Thank you so much for your amazing contribution, which really helped to reach a solution sooner rather than later.

@doistbot
Copy link
Member

🎉 This issue has been resolved in version 2.1.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@doistbot doistbot added the released Pull requests that have been released to production label Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers released Pull requests that have been released to production
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants