RFR: 8296266: TextArea: Navigation breaks with RTL text

[Phil Race]

On Mon, 9 Oct 2023 21:58:54 GMT, Phil Race <prr at openjdk.org> wrote:

>> 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).
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextAreaSkin.java line 572:
> 
>> 570:         if (isRTL()) {
>> 571:             // Text node is mirrored.
>> 572:             moveRight = !moveRight;
> 
> This method confuses me as to what it actually wants to do and why
> As in, "move right, no, your other right" (!)
> 
> isRTL() checks the NodeOrientation so it seems in such a case "right" means "left".
> Have you checked the fix with both orientations ?
> Seems to me it reverses the meaning of the arrow keys without regard to the text.
> 
> And he BreakIterator is getting the next logical position, but this method claims to move to the next visual position.
> The method is called nextCharacterVISUALLY, but with this code its actually moving LOGICALLY,
> so I'm not seeing how this works for mixed directional text for either setting of "isRTL()" which seems to me to be very questionable to even check here. IMO the arrow key should work the same way no matter whether the UI is laid out with nodes from RTL or LTR.
> 
> Perhaps the word "Visually" was not the best choice of name here ? And "right" is questionable too. But I'm not 100% sure.
> if you really do want LOGICALLY, then this code will probably work, except I find the isRTL() questionable even then.
> 
> What exactly was wrong in the existing code ? Other than it being very complex. How did its calculations go wrong ?

PS if I just use simple ASCII text and set Node Orientation to RTL, then with your fix
then left arrow moves right and right arrow moves left.
In JDK 8 left arrow moved left and right arrow moved right.
With an FX 17 build I have around, then the nothing moves.
So it looks like things might have been better when this code was written and some other change broke it.
Your fix unbreaks it, but I'm not entirely sure its restoring intended behaviour.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1220#discussion_r1350852862