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

[Ajit Ghaisas]

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

>> 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.

Initially I had thought about adding separate test file for RTL - something like RTLTableViewKeyInputTest - but, realized that although it's a cleaner approach, we would simply duplicate the tests. Also, the fact is only LEFT/RIGHT key navigation is sensitive to NodeOrientation - hence only a subset of tests needed modification. This is the reason I have parameterized the test.

To your specific question, since it is a parameterized test, only possible values are LTR and RTL which are specified as @Parameterized.Parameters. I don't think, we need additional check for some other value.

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

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