Add support for isolates in bidi context#5447
Add support for isolates in bidi context#5447alexisdoualle wants to merge 18 commits intocodemirror:masterfrom
Conversation
| return coords(ch, prop)[prop] | ||
| } | ||
|
|
||
| if (cm.state.newIsolate) { lineObj.order = cm.state.newIsolate = null; } // remove previous order in case an isolate marker was added |
There was a problem hiding this comment.
This will only work for one line, right? Whereas all order caches should be cleared. Or am I missing something?
There was a problem hiding this comment.
Yes, I only saw the need to clear the cache for the current line. Why should all caches be cleared? Isn't the order specific to a single line?
| if (endStyle) fullStyle += endStyle | ||
| let token = elt("span", [content], fullStyle, css) | ||
| if (title) token.title = title | ||
| if (isolate) { |
There was a problem hiding this comment.
Will this still work when there are multiple tokens in the range? It seems like it'd create a <bidi> tag for each token separately, which I believe won't get the result we want.
There was a problem hiding this comment.
You're right, I'll try to find a solution to that. Is there an option to prevent certain markers from overlapping each other?
There was a problem hiding this comment.
I added a function to check that isolate ranges do not overlap, in the same style as conflictingCollapsedRange() 3b6c52c
| else | ||
| rect = getUsefulRect(range(node, start, end).getClientRects(), bias) | ||
| else { | ||
| if (atomic) {start = 0, end = place.coverEnd - place.coverStart} // cover the entire isolate segment |
There was a problem hiding this comment.
Atomic currently restricts cursor motion, but not measuring of characters inside of the span --- and I think it should stay that way.
There was a problem hiding this comment.
The "atomic" parameter was first called "isolate" (I probably should have left it that way), because I only added it for the new option I'm working on. I renamed it to "atomic" because it was only needed when an isolate segment is also atomic, to prevent the cursor from appearing within the atomic isolate segment, and instead make it appear on the far side of it.
|
I understand the need for the kind of feature you are proposing here, but I don't think this implementation is solid enough to include—both conceptually and in terms of implementation. We're working on a redesign of the library where the code will (always) be a contentEditable piece of DOM, and we'll be able to offload cursor motion and such to the browser, which might make this easier to implement. Until then (and it'll take a while for this to become production-ready), it may not be viable to get isolates in the editor. |
|
Thank you for the feedback. I understand that this implementation isn't really up to par; any ideas for improvement are greatly appreciated. What would be needed to have it included to CM? I will continue to work on it, even if it won't be merged into the code base, because it's the only way I found to work with HTML tags in bidi text. Good news about that redesign, is there a way to monitor its progress? |
Not yet, but there will be an announcement, at which point we'll move development to github, in the coming months. |
|
The current state of work is available on https://github.com/codemirror/codemirror.next. |
3 participants
I'm working on a feature to support isolate segments in bidirectional text.
I added an option to markText(), "isolate", taking two possible values: "ltr" or "rtl". This has the effect of wrapping the segment in a <bdi> tag, and setting the direction of the marked text to "ltr" or "rtl". The feature works especially well with the "atomic" option, for instance to isolate HTML tags in bidi text, which is often a pain.
I added this feature to fix an issue which can be recreated like this: take the bidi demo (https://codemirror.net/demo/bidi.html), and at the end of line 6 (to the left of it, visually), type any English word such as "hello". This will make the last few characters on the line go from this:
to this:
This is only visual, the logical order is as expected.
The recommended way of circumventing this issue is to wrap the desired text in a <bdi> tag, which is what the new option achieves.
I created a fiddle as a demo: https://jsfiddle.net/yuLhqvo7/44/
To make it work I had to modify the bidi algorithm a bit, to wrap isolates in the same bidi segment. I had to modify the N1 part of the algorithm also to take isolate ranges into account when working with neutral characters.
One of the biggest challenges was to adapt the cursor's behavior to work with these isolate segments. The cursor needs to take into account the fact that an isolate segment can be atomic. This implementation seems to do the job, but I wouldn't be surprised if I missed some use cases.
This feature works very well for what I want to achieve, but I'm sure it could be improved. All tests passed using the most recent Chrome and Firefox Versions - at least the ones that passed using the latest Code Mirror release.