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

Two-steps caret movement when attribute starts at the beginning of block #4277

Closed
oskarwrobel opened this issue Feb 15, 2018 · 3 comments · Fixed by ckeditor/ckeditor5-engine#1406
Assignees
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@oskarwrobel
Copy link
Contributor

Follow-up #4269.

Caret does not "leave" attribute element while moving left when text starts at the beginning of block element.

<p><a href="">{}foo</a></p>

Pressing left arrow does not move selection before element <a/>.

@Reinmar
Copy link
Member

Reinmar commented Feb 26, 2018

This also doesn't work when the attribute is at the end of the content.

@szymonkups
Copy link
Contributor

szymonkups commented Mar 7, 2018

Another issue:

  1. Place selection at the end of the link.
  2. Press left and right arrow keys couple times (enter and leave link using two-step caret movement).
  3. Press left arrow to put selection inside link.
  4. Try to exit link pressing right arrow.

Result: need to press right arrow key many times to exit the link.

two-step

@Reinmar
Copy link
Member

Reinmar commented Mar 11, 2018

I've dug a bit more into the implementation of restoreGravity() and overrideGravity() and there are a couple of things which I found at least worth checking investigation:

  1. restoreGravity() should log a warning when trying to go below 0. It should also never allow going below 0. It may also happen that situations when restoreGravity() is called more times than overrideGravity() happen so often that we should simply accept this situation (and don't log).
  2. It's very odd that overrideGravity() only set the custom restore callback when overrideGravityCounter == 1. What if we called overrideGravity() more than once in a row?
  3. I'm afraid that the automatic restore doesn't make sense. You can test it like this: add a console log in change:range callback added by overrideGravity(). Put your caret before link and then press right, left, right, left. Now, click somewhere else in the text. Console log will be triggered 2+ times. That's because these callbacks are never removed when you press left arrow key because left arrow key is handled by bindTwoStepCaretToAttribute() which also restores the gravity there. I think that it might be better if that util always handled restore manually.

I'm inclined now towards rewriting this logic with these assumptions:

  • bindTwoStepCaretToAttribute() has its own flag whether it has just overridden gravity or not,
  • bindTwoStepCaretToAttribute() should also take the responsibility to restore the gravity when needed,
  • we completely remove the auto-restoring logic from document selection,
  • we add some assertion logic for cases which should not happen and see if it helps to find bugs.

@oleq oleq assigned oleq and unassigned szymonkups Apr 5, 2018
scofalik referenced this issue in ckeditor/ckeditor5-engine Apr 18, 2018
Fix: The `bindTwoStepCaretToAttribute` behavioral helper should not fail in more complex cases. Closes #1301. Closes #1346. Closes ckeditor/ckeditor5#937.  Closes ckeditor/ckeditor5#922.  Closes ckeditor/ckeditor5#946.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 16 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:engine labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
5 participants