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

Fixing selection after mutation is handled. #3060

Closed
scofalik opened this issue Oct 6, 2016 · 7 comments · Fixed by ckeditor/ckeditor5-typing#53
Closed

Fixing selection after mutation is handled. #3060

scofalik opened this issue Oct 6, 2016 · 7 comments · Fixed by ckeditor/ckeditor5-typing#53
Assignees
Labels
package:typing type:bug This issue reports a buggy (incorrect) behavior. type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@scofalik
Copy link
Contributor

scofalik commented Oct 6, 2016

This issue is a result of https://github.com/ckeditor/ckeditor5-engine/issues/399 .

We agreed that right now we will fix problems with selection during typing straight in the Input feature. We decided that we will come up with checks that will try to recognize what happened / what user did by checking what mutations were generated.

Three scenarios that we want to handle right now are:

  • typing,
  • spellchecker (right click, change to given word),
  • autocorrect (i.e. on Safari, after clicking space word is automatically changed).
  1. Typing. If n letters were inserted at the same position as selection position, and 0 letters were removed -> move selection position by n.
  2. Spellchecker. If n letters were inserted and m letters were removed, find word boundary after last change and place selection at that word boundary,
  3. Autocorrect. If n letters were inserted and m letters were removed, and last inserted letter was space character, move selection after that space character.
@scofalik scofalik self-assigned this Oct 7, 2016
@scofalik
Copy link
Contributor Author

scofalik commented Oct 7, 2016

Also we agreed on trying to do one big change that will insert/remove all changed letters instead of multiple one-by-one changes.

@Reinmar
Copy link
Member

Reinmar commented Oct 7, 2016

The above will duplicate a bit work done in https://github.com/ckeditor/ckeditor5-typing/issues/48. In fact, it makes the current code there invalid.

@scofalik
Copy link
Contributor Author

scofalik commented Oct 7, 2016

1bxepk

So, I know we've discussed it...

But.

Spellchecker scenario wouldn't work if the change was like "exmple" -> "example", it would be treated as typing. And autocorrect scenario is very similiar to typing anyway, so there wasn't much to do.

So. As I don't like "targeted" solutions and prefer general ones I implemented solution basing on DOM selection. It works like this.

  1. In MutationObserver, DOM selection is passed together with mutations without any processing. If you don't like it, DOM selection can be also retrieved this way directly in Input:
    domConverter.getCorrespondingDom( mutation.node ).ownerDocument.getSelection().
  2. DOM selection focusNode and focusOffset are converted to view position. This should always work, because we are changing already existing DOM text node. Then view position is converted to model position and it's "cached".
  3. Changes are done to model, as they were.
  4. Model selection is collapsed at calculated earlier model position. If at any point there was an error (DOM->View->Model position conversion) we "fall back" to depending on LiveRanges.

Please don't be mad at me. It works.

BTW, as far as InputCommand is concerned, it can still work as we decided before. It can set selection. We will, however, change that selection in InputFeature. Or, the calculated selection position can be used to properly set data passed to InputCommand.

@Reinmar
Copy link
Member

Reinmar commented Oct 7, 2016

BTW, as far as InputCommand is concerned, it can still work as we decided before. It can set selection. We will, however, change that selection in InputFeature.

How is this going to affect undo? In fact, does it work this way that selection is stored once you press ctrl+z or on some changesDone or other event?

@scofalik
Copy link
Contributor Author

scofalik commented Oct 10, 2016

All those operations act like typing, so let's say that undo works like it works for typing. Some of changes done on undo and redo seems counter-intuitive because of how buffer works. But the results correctness should be satyisfying.

@Reinmar
Copy link
Member

Reinmar commented Oct 10, 2016

I understood nothing. Did you answer my question? :D Or was it a random thought? :P

@scofalik
Copy link
Contributor Author

I forgot you were asking in relation to InputCommand. So, the selection is "remembered" when Batch is created so it does not matter how we change it after we've done changes to the document. We were changing selection after creating Batch in current implementation of typing too.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-typing Oct 9, 2019
@mlewand mlewand added this to the iteration 4 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. 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:bug This issue reports a buggy (incorrect) behavior. 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.

3 participants