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

Markdown autoformat italics bug #2388

Closed
vladikoff opened this issue Mar 7, 2018 · 17 comments · Fixed by #8619
Closed

Markdown autoformat italics bug #2388

vladikoff opened this issue Mar 7, 2018 · 17 comments · Fixed by #8619
Assignees
Labels
intro Good first ticket. package:autoformat squad:core Issue to be handled by the Core team. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@vladikoff
Copy link

Sorry if this was fixed in latest or not, but I wanted to make to discuss this.

From a user:

I was writing some notes and tried writing wrote word1_word2_word3 and while writing it, it did word1word2word3 and word 2 was in italics. I am assuming that's because of the underlines but I am not sure this is intentional behavior. Thanks!

STEPS TO REPRODUCE

  1. Type: word_1 and word_2

EXPECTED RESULTS

  • word_1 and word_2

ACTUAL RESULTS

  • word1 and word2

Ref: mozilla/notes#595

I wonder if auto-format should only format if there are spaces around _ or if it starts the line?

@Reinmar
Copy link
Member

Reinmar commented Mar 7, 2018

I think that GitHub disabled foo_bar_bom at some point because _ is commonly used in variable names. Interestingly, this doesn't apply to foo*bar*bom which works. I guess we could do the same, although, to make it simpler, I'd treat foo_bar_bom and foo*bar*bom in the same way.

The important thing here would be to not filter out too many scenarios because it may be confusing why autoformatting doesn't work in some cases then.

@csmoe
Copy link

csmoe commented Aug 15, 2018

I stuck by this underscore-autofomat issue too

The important thing here would be to not filter out too many scenarios because it may be confusing why autoformatting doesn't work in some cases then.

I think there is no need for filtering since markdown syntax already has BACKSLASH ESCAPES to escape those auto-format characters(*/_), for example, I tried \_ to escape _, but sadly failed. so just bring markdown's backslash escapes here.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-autoformat Oct 9, 2019
@mlewand mlewand added this to the backlog milestone Oct 9, 2019
@mlewand mlewand added status:discussion type:improvement This issue reports a possible enhancement of an existing feature. package:autoformat labels Oct 9, 2019
@DenitsaGencheva
Copy link

Hi guys,
any update on this?
A lot of our clients are complaining because of this issue as they use texts like item_item_item.
We made the following change in the plugin:
this
/(?:^|[^_])()([^_]+)()$/g
we changed to:
/(?:^|\s)()([^_]+)()$/g
but this way it is erased after we update the editor.

@julie0842
Copy link

Hey all, we are running into this issue as well and I just wanted to check to see if there are any updates with this issue? Thanks!

@Reinmar
Copy link
Member

Reinmar commented Nov 30, 2020

This works better in GitHub Writer. I think we could make this regexp narrower.

@Reinmar Reinmar added intro Good first ticket. squad:core Issue to be handled by the Core team. and removed status:discussion labels Nov 30, 2020
@Reinmar Reinmar modified the milestones: backlog, iteration 39 Dec 2, 2020
@maxbarnas
Copy link
Contributor

maxbarnas commented Dec 8, 2020

I checked other Markdown implementations (technical details at the end) and decided to do as below:

  • Simple regex update, that will require a whitespace or punctuation character before opening delimiter should be enough for most cases. 
    • Considering that Autoformat is mostly an aid in typing, not a full-featured Markdown parser, it does not seem viable to copy the entire regex for punctuation from CommonMark. Still, I am not sure how far we should go with that - any ideas?
  • Seems reasonable and should be cheap to update Bold functionality in the same manner.
  • Nice-to-have: Autoformat catches cases where first the opening delimiter (* or _) is typed in, then closing one. Being able to catch a reversed case would be great. 
    • word → word_ → _word_ ⇒ word

Most of the above are based on CommonMark spec. Some key elements of that spec:

  • Delimiter: _ or * (single or many).
  • Whitespace: space, tab, the start of the line, end of the line, etc.
  • Punctuation: A punctuation character is an ASCII punctuation character or anything in the general Unicode categories Pc, Pd, Pe, Pf, Pi, Po, or Ps.
  • ASCII punctuation character: !, ", #, $, %, &, ', (, ), *, +, ,, -, ., / (U+0021–2F), :, ;, <, =, >, ?, @ (U+003A–0040), [, \\, ], ^, _, (U+005B–0060),{,|,}, or~` (U+007B–007E).

@Reinmar
Copy link
Member

Reinmar commented Dec 9, 2020

  • Simple regex update, that will require a whitespace or punctuation character before opening delimiter should be enough for most cases.

What about what's after the closing delimiter?

  1. aa _bb^cc
  2. type _
  3. aa _bb_^cc

It's a less severe case than the one with foo_bar_^, but still might be irritating.

  • Seems reasonable and should be cheap to update Bold functionality in the same manner.

👍

Nice-to-have: Autoformat catches cases where first the opening delimiter (* or _) is typed in, then closing one. Being able to catch a reversed case would be great.

This is a nice catch. It always bothered me that it does not work in our implementation of autoformat. If you could check quickly how big that change would be, that'd help us planning this change for the future.

@maxbarnas
Copy link
Contributor

What about what's after the closing delimiter?

Good point. Another reason to treat whitespace as a separator.

Also, in the example below, you can see better why punctuation characters should join whitespace as a separator for autoformat:

  1. word _word^, word
  2. type _
  3. word word, word

@maxbarnas
Copy link
Contributor

maxbarnas commented Dec 9, 2020

This is a nice catch. It always bothered me that it does not work in our implementation of autoformat. If you could check quickly how big that change would be, that'd help us planning this change for the future.

Regarding this, autoformat seems to grab text from the current line, up to the cursor. Without seeing what is after the cursor we cannot solve the problem of stopping the formatting inside the longer word. This also explains why reversed autoformatting cannot work right now.

The first idea that comes to mind is to grab the entire text line, then check what is after the cursor. If we can find a matching delimiter, followed by whitespace or punctuation, that's great - let's format that, otherwise stop.

Looking ahead only up to the end of the line should stop autoformatting from getting too excited about formatting what's further down in the text :)

@maxbarnas
Copy link
Contributor

maxbarnas commented Dec 9, 2020

Two things I noticed already:

  • With text like a _b_ d^ (which had the autoformatting undone) every time I type something, I still get that _b_ autoformatted. 
    • I am thinking about limiting the scope for autoformatting, nothing concrete yet, unfortunately.
  • Calculation done on the regex match doesn't like the non-capturing groups I used for matching whitespace (and later punctuation). 
    • I can change Autoformatter to expect 5 groups instead of 3: 2 groups for separator matches (whitespace/punctuation), 2 groups for delimiters, and 1 group for content. Wouldn't be that a breaking change?
    • Or maybe work with the code I have now - still trying to figure it out.

@maxbarnas
Copy link
Contributor

At the moment, we expect to have space preceding the opening delimiter (_, *, __ and **). Only then we allow the autoformat to do its job.

Achieving reverted autoformatting starts pretty easy, but, in connection with undo, gets complicated quite fast.

To properly format a reverted case, we have to look ahead of the cursor, possibly up to the end of the line. This breaks undo and makes undone autoformatting format again. We could look ahead and behind the cursor up to a certain point, e.g. first opened/closed delimiter, but the implementation of that becomes too complex for the scope of this bug.

Reinmar added a commit that referenced this issue Dec 11, 2020
Fix (autoformat): Formatting will not be applied to `snake_case_scenarios` anymore. Closes #2388.
@Echecivuole
Copy link

Hello all, is there so any final solution to fix this italic issue with ckeditor5?

@aniketdeotale
Copy link

aniketdeotale commented Apr 21, 2021

Hello, I have the same issue in the project
Input : 'Test_comment_new"
Output : Testcommentnew
where output comment is in itallic

is there any solution for the same?

@maxbarnas
Copy link
Contributor

@Echecivuole and @aniketdeotale the issue is already fixed. Please update your CKEditor package.

@Echecivuole
Copy link

@Echecivuole and @aniketdeotale the issue is already fixed. Please update your CKEditor package.

Hi thanks! Sorry I would need your help on from where and how update my CKEditor package. I have folder in my vendor locale. thanks!

@aniketdeotale
Copy link

@Echecivuole and @aniketdeotale the issue is already fixed. Please update your CKEditor package.

The issue still persist for me even after updating the ckeditor to latest version . Please verify once.

@rcole1919
Copy link

rcole1919 commented Mar 15, 2023

Hi! There is another problem.
In the admin side I am writing word and doing italic for a part of the word like:

something

But on the client side this text looks like:

som_eth_ing

It is happenning because in the part of the word it expects only asterisk symbols

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
intro Good first ticket. package:autoformat squad:core Issue to be handled by the Core team. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants