[Rev 02] RFR: 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation

[Jeanette Winzenburg]

On Tue, 25 Feb 2020 15:04:05 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> The pull request has been updated with 1 additional commit.
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewKeyInputTest.java line 1141:
> 
>> 1140:             keyboard.doLeftArrowPress(KeyModifier.getShortcutKey());
>> 1141:         }
>> 1142:         keyboard.doKeyPress(KeyCode.SPACE,
> 
> having such if blocks in all tests regarding horizontal navigation feels like the parametrization isn't quite complete, IMO: the test method shouldn't need to worry, instead just call semantic horizontal navigation methods.
> 
> A simple change would be to extract that differentiation into a dedicated method, like f.i. forward/backward, test would get simpler (and make it easier to add those that are missing :)
> 
>     @Test
>     public void testForwardFocus() {
>         sm.setCellSelectionEnabled(true);
>         sm.select(0, col0);
>         // current
>         //if (orientation == NodeOrientation.LEFT_TO_RIGHT) {
>         //    keyboard.doRightArrowPress(KeyModifier.getShortcutKey());
>         //} else {
>         //    keyboard.doLeftArrowPress(KeyModifier.getShortcutKey());
>         //}
>         // extracted
>         forward(KeyModifier.getShortcutKey());
>         assertTrue("selected cell must still be selected", sm.isSelected(0, col0));
>         assertFalse("next cell must not be selected", sm.isSelected(0, col1));
>         TablePosition focusedCell = fm.getFocusedCell();
>         assertEquals("focused cell must moved to next", col1, focusedCell.getTableColumn());
>     }
>     
>     /**
>      * Orientation-aware horizontal navigation with arrow keys.
>      * @param modifiers the modifiers to use on keyboard
>      */
>     protected void forward(KeyModifier... modifiers) {
>         if (orientation == NodeOrientation.LEFT_TO_RIGHT) {
>             keyboard.doRightArrowPress(modifiers);
>         } else {
>             keyboard.doLeftArrowPress(modifiers);
>         }
>     }

Also, I'm a bit weary about the "else if" (vs a simple "else") - wouldn't it be some kind of setup error if the node orientation is neither rtl nor ltr? If so, I would add a test to check for it once.

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

PR: https://git.openjdk.java.net/jfx/pull/114