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

Typing's undo step should be broken by applying an attribute #3043

Closed
Reinmar opened this issue Jul 5, 2016 · 6 comments · Fixed by ckeditor/ckeditor5-typing#80
Closed
Assignees
Labels
package:typing type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Jul 5, 2016

  1. Open http://tests.ckeditor.dev:1030/build/amd/tests/basic-styles/manual/basic-styles
  2. Type few letters (make sure to type in a place where italic isn't applied).
  3. Press italic button.
  4. Type few more letters.
  5. Press undo button.

Only the letters typed in step 4 should be undone.

This issue is very similar to #3042. Changing selection attributes should reset a buffer, just like moving the selection should.

@maxbarnas
Copy link
Contributor

I noticed, that undo operation deletes just part of the text typed in step 4 above, when typed text is quite long (dozen or more characters).

@Reinmar
Copy link
Member Author

Reinmar commented Sep 20, 2016

That's the expected result. It's so called undo step – https://github.com/ckeditor/ckeditor5-typing/blob/7c446342e8a742b77ab260ad4a7f22d51b58e0b8/src/input.js#L36

@maxbarnas
Copy link
Contributor

I kind of suspected this. Thanks for pointing out the actual piece of code responsible for that.

maxbarnas referenced this issue in ckeditor/ckeditor5-typing Sep 20, 2016
@maxbarnas
Copy link
Contributor

Prototype of solution is available in t/21 branch.

Listener of changesDone event wraps all the change:attribute events to allow proper unlocking ChangeBuffer.

Listening to change:range does not affect the fix, nor locking buffer in ChangeBuffer#_handleKeydown.

This prototype also fixes problem when changing selection: https://github.com/ckeditor/ckeditor5-typing/issues/20.

@f1ames f1ames self-assigned this Feb 15, 2017
@f1ames
Copy link
Contributor

f1ames commented Feb 15, 2017

Pushed changes to t/21. The only thing missing are unit/integration input tests. Since ckeditor/ckeditor5-typing#48 with InputCommand should be ready soon, I am not sure if it makes sense to create missing tests now without it and then rewrite it using InputCommand.

@f1ames
Copy link
Contributor

f1ames commented Mar 6, 2017

Reinmar referenced this issue in ckeditor/ckeditor5-typing Mar 9, 2017
Fix: New undo step should be created on selection change or applying an attribute. Closes #20. Closes #21.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-typing Oct 9, 2019
@mlewand mlewand added this to the iteration 9 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:improvement This issue reports a possible enhancement of an existing feature. 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:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
4 participants