On Sat, 15 Feb 2020 15:01:47 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> The pull request has been updated with 1 additional commit. > > @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)) ------------- PR: https://git.openjdk.java.net/jfx/pull/114
