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

Overriding selection gravity breaks keyboard navigation in RTL content #4528

Closed
oleq opened this issue Jul 31, 2019 · 5 comments
Closed

Overriding selection gravity breaks keyboard navigation in RTL content #4528

oleq opened this issue Jul 31, 2019 · 5 comments
Labels
package:engine resolution:expired This issue was closed due to lack of feedback. status:discussion status:stale type:bug This issue reports a buggy (incorrect) behavior.

Comments

@oleq
Copy link
Member

oleq commented Jul 31, 2019

  1. Get yourself an RTL–friendly environment (dir="rtl" on editable, text-align: right). Best try t/1151: Implemented the right–to–left (RTL) languages support for the UI and the content #1881.
  2. Use an editor with the following data
    <h2><strong>لغة</strong></h2><p>اللغة نسق من الإشارات والرموز، يشكل أداة من أدوات المعرفة، وتعتبر اللغة أهم وسائل التفاهم والاحتكاك بين أفراد المجتمع في جميع ميادين الحياة. وبدون اللغة يتعذر نشاط الناس المعرفي. وترتبط اللغة بالتفكير ارتباطًا وثيقًا؛ فأفكار الإنسان تصاغ دومًا في قالب لغوي، حتى في حال تفكيره الباطني. ومن خلال اللغة فقط تحصل الفكرة على وجودها الواقعي. كما ترمز اللغة إلى الأشياء المنعكسة فيها.</p>
  3. Set selection after bold heading.
  4. Override selection in the console
    editor.model.change( writer => writer.overrideSelectionGravity() );
    
  5. Use the Left Arrow to move to the next paragraph.

Expected

The caret moves to the next paragraph (as if no overrideSelectionGravity() was called).

Kapture 2019-07-31 at 14 52 16

Actual

The caret is stuck.

Kapture 2019-07-31 at 14 56 05

More info

  1. Some text attribute is a must-have, let it be bold or link or whatever. Without it, calling overrideSelectionGravity() does not break anything.
  2. Similar things happen at the beginning of the line, the caret is stuck. Probably the same issue.
  3. Found when working on RTL support #1151.

cc @scofalik @jodator

@oleq
Copy link
Member Author

oleq commented Jul 31, 2019

P.S.: I used bold to showcase the issue because we don't use overrideSelectionGravity() around bold text by default. It shows that the bug is generic.

In fact, the real problem is with links, which do use overrideSelectionGravity()to implement the two–step caret movement. It's the link UX that is broken in RTL editors.

@jodator
Copy link
Contributor

jodator commented Jul 31, 2019

I can only confirm such behavior. I didn't dig into the overrideSelectionGravity() internals. I wonder only if we should check when to use overrrideSelectionGravity() in LTR and when in RTL (link was only broken on the edge AFAICS). I wonder if the fix would require a special check for RTL in this logic or is this more general problem?

@oleq
Copy link
Member Author

oleq commented Jul 31, 2019

The thing is, whether LTR or RTL, once overrideSelectionGravity() was executed, upon the next change on the selection it should be restored.

TBH, I'm not sure what overrideSelectionGravity() has to do with the keyboard navigation in the document. It blows my mind because gravity should be about selection attributes only, nothing else.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added status:discussion type:bug This issue reports a buggy (incorrect) behavior. package:engine labels Oct 9, 2019
@Reinmar Reinmar added this to the backlog milestone Nov 18, 2019
@pomek pomek removed this from the backlog milestone Feb 21, 2022
@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added the resolution:expired This issue was closed due to lack of feedback. label Nov 4, 2023
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine resolution:expired This issue was closed due to lack of feedback. status:discussion status:stale type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

6 participants