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

[IME] Styling is lost after making composition with enabled inline style #3139

Closed
Mgsy opened this issue Apr 15, 2019 · 10 comments · Fixed by ckeditor/ckeditor5-engine#1730
Closed
Assignees
Labels
package:typing type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@Mgsy
Copy link
Member

Mgsy commented Apr 15, 2019

Originally reported here - https://github.com/ckeditor/ckeditor5-build-classic/issues/69.

Steps to reproduce

  1. Put the caret in the editor.
  2. Change language to Hiragana.
  3. Activate bold.
  4. Start composition.

Current result

Styling is lost.

It occurs in all browsers.

@imyyg
Copy link

imyyg commented Apr 16, 2019

Great God, when will this problem be solved? Or is there any temporary solution? Hope to teach @Mgsy

@Mgsy
Copy link
Member Author

Mgsy commented Apr 16, 2019

Great God, when will this problem be solved? Or is there any temporary solution? Hope to teach @Mgsy

Actually, it turned out that this is a regression and this case was working in CKEditor 5 v11.2.0. We're going to fix it in this iteration and include it to the next CKEditor 5 release.

This bug has been introduced along with this change - ckeditor/ckeditor5-engine@4f9ac0e.

@oskarwrobel oskarwrobel self-assigned this Apr 16, 2019
@Reinmar
Copy link
Member

Reinmar commented Apr 17, 2019

This is a serious issue. @oskarwrobel, is now working on finding a solution. We'll post it here and perhaps release a quick small release of ckeditor5-engine to fix it.

@oskarwrobel
Copy link
Contributor

Selection attributes are refreshed after each operation to be sure that selection is always up to date (https://github.com/ckeditor/ckeditor5-engine/issues/1673). A consequence is that attributes are removed from the selection while the composition is in progress.

What's going on?

  1. Executing bold command in the empty editor sets bold attribute as the selection attribute, priority of attribute is normal (not low) because it is a direct change.
  2. Typing the first character adds it to the editor with a bold attribute and moves the selection after the character what resets the priority of selection attributes to low (it's no longer a directly set attribute but an attribute set because the previous character is marked as bold).
  3. Adding the next character composes it with the previous one, so the previous one is removed, the selection is refreshed and bold is removed from the selection attributes because there is no text with bold around the selection at this stage. Finally, the new, composed character is inserted without any attribute.

@oskarwrobel
Copy link
Contributor

We decided to not refresh the selection after each operation (as it was before). It sounds good to have selection up to date after each operation, but it requires some changes for spell checking and composition to remember attributes before replacing text.

pjasiun referenced this issue in ckeditor/ckeditor5-engine Apr 23, 2019
Fix: Prevented from losing selection attributes between operations - IME. Closes https://github.com/ckeditor/ckeditor5-typing/issues/188.
pjasiun referenced this issue in ckeditor/ckeditor5-typing Apr 23, 2019
Tests: Added test for checking if an attribute is not lost when composition replaces a text. Part of https://github.com/ckeditor/ckeditor5-typing/issues/188.
@Reinmar
Copy link
Member

Reinmar commented Apr 23, 2019

Do you think we could release ckeditor5-engine@13.1.1 with just this change? How unsafe it is?

@Reinmar
Copy link
Member

Reinmar commented Apr 23, 2019

👆 @pjasiun @scofalik @Mgsy @oskarwrobel

@pjasiun
Copy link

pjasiun commented Apr 23, 2019

I believe the fix should be safe. I am not sure how our environment will handle the fact that the single package is released. Since we use "^13.1.0" we should be safe. So theoretically there should be no issues. On the other hand, my intuition tells me that it is a bad idea and something will go wrong, so I would not do it.

@oskarwrobel
Copy link
Contributor

From the engine perspective, this fix should be safe.

@Mgsy
Copy link
Member Author

Mgsy commented Apr 23, 2019

I played with the editor a bit during testing and haven't experienced any weird behaviour or regression.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-typing Oct 9, 2019
@mlewand mlewand added this to the iteration 24 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:typing labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:typing type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants