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

[Jeanette Winzenburg]

On Fri, 6 Mar 2020 21:38:15 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   extra space removed
>
> I think the fix looks good and the approach you took for the new test looks good to me. If @kleopatra is OK with it,
> then please proceed.
> I left a couple minor comments.

> 
>     1. I liked the approach proposed in PoC except the need to send KeyMapping as a parameter to the test. It unnecessarily
>     makes test look complex.
>        We already have backward() and forward() helper functions in this test class. The Key presses can easily be decided
>        based on orientation in these methods. I have modified the PoC test provided by you to make it simpler.
> 

Well, I disagree with your notion of "simpler" - replacing conditional blocks in tests by parameterized tests is the
whole point of these. But at the end of the day, it's probably a matter of personal preferences :)

>     2. Also, in PoC, testChangeOrientationSimpleForwardSelect does not respect forward selection after Node orientation is
>     changed. The comment says it all -
>        `// same arrow as initial,now should have inverse effect`
>        The forward movement should still be forward even after Node orientation change and not inverse.
> 

the difference between my and your code in changeNodeOrientation is that mine only changes the table's orientation and
yours additionally changes the parameter nodeOrientation - resulting in your navigational methods using keys based on
the orientation while mine uses those for the old orientation.  Changing the parameter smells, IMO.

> 
> I have done modifications to the PoC code provided and added this test in latest commit.
> Please review.
> Once we agree on the approach-
> 
>     * I will add other tests for horizontal keyMappings with modifiers to TableViewHorizontalArrowsTest
> 
>     * I will revert changes done to TableViewKeyInputTest

agreed on the overall approach, will add a couple of inline comments

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

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