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

[Ajit Ghaisas]

On Sun, 1 Mar 2020 12:24:06 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> @kleopatra is right about the need to handle the case where the orientation of a node changes. As for the test, I think the idea of parameterizing it with LTR, RTL is good. I haven't reviewed it in detail, but added one minor suggestion for you to consider.
>
> After digging a bit, a couple of notes (not entirely certain if this here is the correct place for them?) 
> 
> **Changing existing tests**
> 
> Don't know what the culture in the fx context is, but: personally I prefer to not touch existing methods. Obvious reason is that I might break a test or test the wrong thing or tweak it in any way that makes it rather useless.
> 
> An example of testing the wrong thingy on changing a test method might be  test_rt18488_selectToLeft (guard against [JDK-8120174](https://bugs.openjdk.java.net/browse/JDK-8120174)). The report describes the misbehaviour as
> 
> 1. select last cell in a row
> 2. extend selection by keyboard backwards (that is to decreasing column indices), making cellMin selected
> 3. shrink selection by keyboard by one cell (that is increasing column index)
> 4. expected: cellMin unselected, actual (bug) cellMin still selected
> 
> the block added for the rtl variant does test something else for
> 
> 2. has no effect because it's already the upper boundary
> 3. nothing to shrink, instead it extends by one
> 4. nothing unselected
> 
> **Parameterized Tests**
> 
> repeating earlier comments (just to have it here for reading convenience): the goal of the parameterization is to make the test code (mostly) unaware of the parameters. In particular, if I feel the need for conditional test blocks - either in setup or in assert blocks - _on the parameters_, I often find out that something went wrong with the factoring. Not written in stone, though, could be me only :)
> 
> **Alternative**
> 
> My preference would be to not touch TableViewKeyInputTest at all, but to add a new test class exclusively for testing orientation-dependent horizontal navigation by keyboard. It should focus on what's actually changed, that is the _basic_ left/right/extend/shrink etc navigation and dynamic change of those. The class can be parameterized, the parameter being a triplet of orientation and forward/backward keys. 
> 
> The emphasis on _basic_ is intentional: I think that there is no need to cover all existing tests against bug reports nor more evolved navigation. Given all basic mappings are working as expected, everything built on top should work as well - might be wrong or there might be exceptions :)
> 
> For a bit of clarity - hopefully- of what I mean, I put togeter a raw poc example [TableViewHorizontalArrowsTest](https://github.com/kleopatra/jfx/blob/experiment-tableview-navigation-rtl-bug-8235480/modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewHorizontalArrowsTest.java))

Thanks @kleopatra for thinking through clearly about testing and providing a simpler test approach with PoC.
I agree with *NOT* modifying existing horizontal key navigation tests to test for NodeOrientation. Testing this separately in a new test does make sense.

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.

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.

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

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

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