jfx
jfx copied to clipboard
openjdk
8296266: TextArea: Navigation breaks with RTL text
The fix uses character BreakIterator instead of the logic that relies on caretBounds/hitTest/rangeShape in TextInputControl.nextCharacterVisually().
I believe this is a more reliable method of navigation, as it behaves in sync with the jdk break iterator, thought it might work differently around grapheme clusters, considering a recent change JDK-8291660
This change also introduces TextInputControlHelper class (impl. detail) which gives access to character- and word- break iterators cached by TextInputControl (some say these iterators and associated editing logic should be a part of Content implementation, but that's a discussion for another day).
Progress
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [ ] Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)
Issue
- JDK-8296266: TextArea: Navigation breaks with RTL text (Bug - P3)
Reviewers
- Karthik P K (@karthikpandelu - Committer) 🔄 Re-review required (review applies to e41da6df)
- Ajit Ghaisas (@aghaisas - Reviewer) 🔄 Re-review required (review applies to e41da6df)
Contributors
- Karthik P K
<[email protected]>
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1220/head:pull/1220
$ git checkout pull/1220
Update a local copy of the PR:
$ git checkout pull/1220
$ git pull https://git.openjdk.org/jfx.git pull/1220/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1220
View PR using the GUI difftool:
$ git pr show -t 1220
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1220.diff
Webrev
:wave: Welcome back angorya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@andy-goryachev-oracle karthikpandelu was not found in the census.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:
/contributor add @openjdk-bot/contributor add duke/contributor add J. Duke <[email protected]>
User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.
@kevinrushforth The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).
Thank you for testing, @karthikpandelu !
This is not observed if individual character is traversed using right arrow or shortcut option + right arrow...
This statement is not exactly correct. Cmd+right navigates to a (visible) line end, in this case it's in the middle of the text string (index=8). So if you place the cursor after the colon (index=7) and press right arrow, you'll get the split caret in exact the same configuration as cmd+right.
This statement is not exactly correct. Cmd+right navigates to a (visible) line end, in this case it's in the middle of the text string (index=8). So if you place the cursor after the colon (index=7) and press right arrow, you'll get the split caret in exact the same configuration as cmd+right.
Thanks for the details. Is it expected to work that way to indicate the end of line? I think it looks bit confusing
@andy-goryachev-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@andy-goryachev-oracle This change is no longer ready for integration - check the PR body for details.
The solution needs re-evaluation due to unexpected complexity dealing with bidi text - specifically with navigation (visual vs. logical) and behavior at the bidi boundary.
@nlisker would you be able to check out the new logical navigation in this PR?
There is also a similar change in navigation in TextField/PasswordField https://github.com/openjdk/jfx/pull/1222 which depends on this PR.
I tested a TextArea. Here are my observations:
-
There is no way to change the primary direction / justification, which on Windows is done with CTRL+SHIFT. This is a must have for RTL support.
-
The caret doesn't indicate if I'm in an RTL or LTR mode. This is from Notepad, showing the direction:
-
The split caret is quite interesting, it's the first time I am experiencing it. There is still the problem that I don't know in which place the text will appear unless I look at which language I have selected, then remember which of the carets is primary. It also doesn't seem to solve the ambiguity:
- Start with this text and the caret at position 0 (in English LTR mode):
ds גדsaששגד sds - Move 3 times to the right (with the right arrow key). I get this split caret:
- Pressing
aadds the character at the lower caret position: - Instead of adding
a, I can move the caret another 2 spaces to the right and have the same caret display as inii. - Now pressing
aadds the character at the upper caret position:
This means I can't know where the character will be added. I think that the split caret is going to confuse a lot of people. Do you know anyone who has ever seen this type of caret?
- Start with this text and the caret at position 0 (in English LTR mode):
- There is no way to change the primary direction / justification, which on Windows is done with CTRL+SHIFT. This is a must have for RTL support.
What is your hotkey setting for switching languages? On my win11, it's ctrl-shift, so in WordPad ctrl-shift seems to control both input language and alignment, looking almost as if both functions are triggered at the same time.
In the case of JavaFX, there is no function to change primary direction with a key stroke.
2. The caret doesn't indicate if I'm in an RTL or LTR mode.
This is how it works in jfx8.
But here we might have some freedom as we could change the way dual caret looks. The directionality indicator seems like a good idea (try crossing the LTR/RTL boundary in swing - it takes two key presses during which the indicator changes. A single caret, even with the directionality indicator does not solve the insertion point question though. May be both carets in the case of dual caret should have directionality indicator?
3. This means I can't know where the character will be added. I think that the split caret is going to confuse a lot of people. Do you know anyone who has ever seen this type of caret?
I've only seen the split caret in FX.
It seems like an attempt to provide an indication of insertion point, but the way it works is confusing: what is definition of the primary and the secondary?
For example, with the TextArea set to RTL orientation, and text set to something like כעיגכחשENGLISHשגלכחעיגל, if the user clicks on the left side of "E" we'll have this:
So the primary caret is to the left side of "E" visually. Then, type a Hebrew symbol and it gets inserted into position indicated by the secondary caret, unexpectedly (isn't RTL a primary in this case?)
At the same time, if we switch TextArea node orientation back to LTR, it works as expected - latin symbols get inserted where the primary caret points, Hebrew symbols go the secondary position.
What is your hotkey setting for switching languages? On my win11, it's ctrl-shift, so in WordPad ctrl-shift seems to control both input language and alignment, looking almost as if both functions are triggered at the same time.
ctrl+shift only changes the primary direction / justification. alt+shift changes the input language. Maybe in Win 11 this was changed, but prior to that, at least since the days of XP, those were the shortcuts.
A single caret, even with the directionality indicator does not solve the insertion point question though.
But It can. The "jumping" caret in the document you linked, which is also what I suggested prior, does that. It provides unambiguous insertion points.
I've only seen the split caret in FX.
Which is why I think we should not be pushing this indicator. People are used to a single caret that shows them where the next character they type will appear. I think that's what we should give them.
jfx8 already has split caret, so I am not so sure about removing it.
The jumping caret is a rather strange solution, in my opinion (user: do I need to time my key presses to the moment when the caret jumps to the right place?) I am not a LTR user, so my opinion here carries little weight, but I thought addition of directionality indicators solves the problem and lowers confusion slightly - the user still needs to maintain some mental state with regards to editing - backspace / delete.
FYI, backspace and delete are not symmetrical - while delete is expected to delete the whole symbol (which might consist of multiple chars), backspace deletes the previous char, breaking up clusters (Note: FX does not currently support complex grapheme clusters like flags or skin tones).
Well, I don't have any experience with split carets whatsoever, nor any of the people I've asked, so I can't advise on it. If the user doesn't need to look outside of the text control to see which language is selected, and can know unambiguously where the character will appear, it's viable. Then again, if the primary caret tells me where my text will appear, why do I need the secondary caret?
Then again, if the primary caret tells me where my text will appear, why do I need the secondary caret?
Because where the next character will appear depends on the character. LTR chars appear in one spot, RTL in the other. I believe this is an issue even with apps that are purely in one language - because of numbers.
ex: means “Reshet 13” רשת 13
But from what I understand, the primary and secondary carets change. They are not an LTR and RTL carets.
Perhaps you can update this branch with the implementation you have in mind and I will check it.
Perhaps you can update this branch with the implementation you have in mind and I will check it.
Quick and dirty prototype of the pennant-like direction indicator suggested by @prrace (thank you!) https://github.com/andy-goryachev-oracle/jfx/pull/5
You can try this using the MonkeyTester TextArea page because it has Window -> RTL menu option for switching the orientation. https://github.com/andy-goryachev-oracle/MonkeyTest
Perhaps you can update this branch with the implementation you have in mind and I will check it.
Quick and dirty prototype of the pennant-like direction indicator suggested by @prrace (thank you!) andy-goryachev-oracle#5
You can try this using the MonkeyTester TextArea page because it has Window -> RTL menu option for switching the orientation. https://github.com/andy-goryachev-oracle/MonkeyTest
This implementation with the dual caret direction indicator is better, but the problems still remain:
- Visual: The caret, single or split, does not indicate the current input direction (point 2 in https://github.com/openjdk/jfx/pull/1220#issuecomment-1770459622). This is very necessary. This is also why I mentioned that a split caret can be rendered as a single caret whose location depends on the input direction; no need to show a split caret showing me a location where I'm not going to enter characters because the input direction doesn't match.
- Functional: The input is either being done in the wrong place, or the indicator is off.
- Start with the caret at this position:
- Enter a character in LTR, the result makes sense:
- Enter a character in RTL, "how did it get there?":
and why did the space separating the RTL and LTR texts was transported to somewhere else?
- Start with the caret at this position:
- If the text starts with RTL:
the split caret doesn't show correctly, and inserting an LTR character adds it somewhere else, far away from the caret:
For JavaFX to match what every modern application does (at least on Windows), the following need to be guaranteed:
- Keyboard shortcut for changing the text orientation (CTRL+SHIFT on Windows).
- Direction indicator in the single caret. Requiring the user to look elsewhere to check what the input method is is detrimental (and I would go as far as to say unacceptable).
Even the super-simple Notepad does those.
@nlisker:
Thank you!
- when you say "The caret, single or split, does not indicate the current input direction", are you referring to this PR or the branch with the direction indicator? Because there we do have an indicator when the caret is split. When it is not split, why do you need an indicator?
I see for example, MS Word does show an indicator but only when the RTL input is active:
is that the behavior you want, to indicate when RTL input is on? If so, we have a problem - there is no current way for FX to know, so we might need to introduce a new API. Another alternative is to rely on NodeOrientation.
-
You are right. There is something wrong, the split caret is not displayed when expected.
-
I don't know if it's right to handle CTRL - SHIFT: it's an OS function that switches input languages, at least on my win11. Perhaps we could use a different key stroke, but let me ask this question: what exactly do you mean "text orientation"? NodeOrientation? Inserting one of these characters - https://www.w3.org/International/questions/qa-bidi-unicode-controls#basedirection ?
I see for example, MS Word does show an indicator but only when the RTL input is active:
is that the behavior you want, to indicate when RTL input is on? If so, we have a problem - there is no current way for FX to know, so we might need to introduce a new API. Another alternative is to rely on NodeOrientation.
Yes, this is what is expected, as I have shown in point 2. The caret needs to show the input direction (bound to the input direction).
- I don't know if it's right to handle CTRL - SHIFT: it's an OS function that switches input languages, at least on my win11. Perhaps we could use a different key stroke, but let me ask this question: what exactly do you mean "text orientation"? NodeOrientation? Inserting one of these characters - https://www.w3.org/International/questions/qa-bidi-unicode-controls#basedirection ?
I don't know how other OS's handle it. Here is Notepad demonstrating the correct behavior:
LTR orientation:
RTL orientation:
Here is WhatsApp Desktop:
LTR:
RTL:
This is how MS Word shows the orientation change:
LTR:
RTL:
You can see the Text Direction and the Alignment properties change.
Here is me writing this comment in the Vivaldi browser:
LTR:
RTL:
The scrollbar position also changes.
Thank you @nlisker for detailed explanation. But I am confused: I see you indicate that the orientation of the control (watsapp, notepad) changes, but if you look at MS Word 2208 on Windows (that's the version I have on my Win11 system) I see that ctrl-shift changes the orientation of the current paragraph. When you say that the side of the scrollbar changes, it means the whole control changes its orientation. So which one do you want, control or paragraph?
And, in the context of TextArea we have no way to control paragraph attributes, but it's definitely a required function for a rich text control.
As for split caret, I just created JDK-8318840 . But, generally speaking, what do you think of having split caret with direction indicators as in https://github.com/andy-goryachev-oracle/jfx/pull/5 ? Do you think it's a good solution?
Also, perhaps we should show a directional indicator any time we have bidi text in the paragraph, not only when the node orientation is RTL, would you agree?
Thank you @nlisker for detailed explanation. But I am confused: I see you indicate that the orientation of the control (watsapp, notepad) changes, but if you look at MS Word 2208 on Windows (that's the version I have on my Win11 system) I see that ctrl-shift changes the orientation of the current paragraph. When you say that the side of the scrollbar changes, it means the whole control changes its orientation. So which one do you want, control or paragraph?
And, in the context of TextArea we have no way to control paragraph attributes, but it's definitely a required function for a rich text control.
Whether it's the control or paragraph that changes seems to be dependent on the control type. I don't think there's any user expectation there. What is expected is that the text will be aligned accordingly and that the primary direction will match.
As for split caret, I just created JDK-8318840 . But, generally speaking, what do you think of having split caret with direction indicators as in andy-goryachev-oracle#5 ? Do you think it's a good solution?
This was point 1 in https://github.com/openjdk/jfx/pull/1220#issuecomment-1777050203. The split caret doesn't tell me where the next character will appear without checking elsewhere what the input language is, and that's not good. The split caret always has, by definition, one irrelevant (half) caret. If I'm in RTL insertion mode, I don't care about the LTR caret, it's confusing to show it.
This is why I suggested to render the split caret in a way that only shows the correct half of the caret based on the input method.
Also, perhaps we should show a directional indicator any time we have bidi text in the paragraph, not only when the node orientation is RTL, would you agree?
A directional indicator should show whenever an RTL input method is selected, regardless of paragraph or node orientation, or what text is in the paragraph. Check Notepad, for example, for the correct behavior here.