On Mon, 9 Oct 2023 22:14:10 GMT, Phil Race <p...@openjdk.org> wrote:
>> 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.
what if you have a mixture of RTL and LTR text in JDK8?
I am going to double check the behavior in JTextArea as well.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1220#discussion_r1350861969
