RFR: 8296266: TextArea: Navigation breaks with RTL text

[Phil Race]

On Tue, 22 Aug 2023 20:46:21 GMT, Andy Goryachev <angorya 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/TextInputControl.java line 759:

> 757:     /**
> 758:      * Returns a cached instance of character break iterator, creating it if necessary.
> 759:      * The instance is initialized with the given text.

"updated with the current text" might be more accurate ? It obviously has to be up to date to be used.
I'm also unsure of the performance consequences of having a BI spanning the entire text of the TextControl, which could be quite large, but that's what the code was already doing I suppose.

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 ?

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

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