ProseMirror / prosemirror

The ProseMirror WYSIWYM editor
http://prosemirror.net/
MIT License
7.61k stars 336 forks source link

RTL issue navigating around marks #1351

Closed klemenoslaj closed 1 year ago

klemenoslaj commented 1 year ago

Keyboard navigation gets locked around any mark when the direction is rtl.

Steps to reproduce:

https://user-images.githubusercontent.com/121867569/214037664-41699aea-da07-423f-8bab-9e1feb498e08.mov

It looks like the prosemirror-view does not recognize the direction and keeps handling navigation to and from marks as ltr.


The following patch appears to fix the rtl direction but doesn't change ltr.

diff --git a/src/capturekeys.ts b/src/capturekeys.ts
index 816022e..fc64c2b 100644
--- a/src/capturekeys.ts
+++ b/src/capturekeys.ts
@@ -258,9 +258,11 @@ export function captureKeyDown(view: EditorView, event: KeyboardEvent) {
   } else if (code == 13 || code == 27) { // Enter, Esc
     return true
   } else if (code == 37 || (browser.mac && code == 66 && mods == "c")) { // Left arrow, Ctrl-b on Mac
-    return selectHorizontally(view, -1, mods) || skipIgnoredNodesLeft(view)
+    const dir = getComputedStyle(view.dom).direction === "ltr" ? -1 : 1;
+    return selectHorizontally(view, dir, mods) || skipIgnoredNodesLeft(view)
   } else if (code == 39 || (browser.mac && code == 70 && mods == "c")) { // Right arrow, Ctrl-f on Mac
-    return selectHorizontally(view, 1, mods) || skipIgnoredNodesRight(view)
+    const dir = getComputedStyle(view.dom).direction === "ltr" ? 1 : -1;
+    return selectHorizontally(view, dir, mods) || skipIgnoredNodesRight(view)
   } else if (code == 38 || (browser.mac && code == 80 && mods == "c")) { // Up arrow, Ctrl-p on Mac
     return selectVertically(view, -1, mods) || skipIgnoredNodesLeft(view)
   } else if (code == 40 || (browser.mac && code == 78 && mods == "c")) { // Down arrow, Ctrl-n on Mac

NOTE: The patch accounts only for the root element direction. For a more robust solution the direction should probably be checked for the currently selected paragraph.

marijnh commented 1 year ago

I am under the impression that on Windows, the convention is still to have arrow keys move by logical index rather than visual position. Chrome seems to do that on Linux nowadays too—not sure about MacOS. And, as you mentioned, LTR text within an RTL container will also make it harder to figure out which way the cursor should be moving.

I spent a bit of time looking into this, but it seems like a lot more is needed to solve it in a reliable way. Screen position queries (getBoundingClientRect on text ranges) are also doing weird things in RTL text on both Chrome and Firefox, making those also unsuitable as a way to figure out the direction at a given document position. Deriving the direction from the surrounding text would require a full implementation of the bidi algorithm, it seems, as well as a more complicated model of cursor positions (bidi direction changes mean one document position can correspond to multiple visual positions).

So, unfortunately, it seems like decent bidi text support in ProseMirror is still quite far off. I'll try to put some more thought into how to make this case less annoying next week.

marijnh commented 1 year ago

That took a lot longer than hoped—apologies. Attached patch, though not perfect, should improve this a lot.

klemenoslaj commented 1 year ago

That took a lot longer than hoped—apologies. Attached patch, though not perfect, should improve this a lot.

Yeah, using it for a while now with patch-package. I expect there are a bunch of things this does not cover tho.