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

[Jeanette Winzenburg]

On Mon, 24 Feb 2020 17:15:02 GMT, Ajit Ghaisas <aghaisas at openjdk.org> wrote:

>> Bug : https://bugs.openjdk.java.net/browse/JDK-8235480
>> 
>> Fix : Added the missed out RTL checks to the key mappings in TableViewBehaviorBase class.
>> 
>> Testing : Modified unit tests in TableViewKeyInputTest to take orientation as a parameter. The Left/Right key press tests have been modified to address LTR and RTL orientations.
>> 
>> Note : If this test modification is acceptable, I would like to address other similar tests separately. (I will create a test JBS issue and address later)
> 
> 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);
        }
    }

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

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