Re: [Rev 02] RFR: 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation
On Sat, 15 Feb 2020 15:01:47 GMT, Kevin Rushforth 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
Re: [Rev 02] RFR: 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation
On Fri, 28 Feb 2020 12:14:24 GMT, Ajit Ghaisas wrote: >> 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. > > Regarding your suggestion of having forward/backward semantic methods and > calling them in tests - > There are two types of tests - > 1. Key action remains same - asserts differ based on NodeOrientation > 2. Key action differs based on NodeOrientation - but, asserts remain same > > Example of 1 is - test_rt18488_selectToLeft > Example of 2 is - test_rt18591_cell_1 > > Your suggestion can be applied only to the tests of type-2. I will try to > update tests of this type. ahh .. okay, missed that difference, thanks :) Hmm, still feels smelly to have to use the parameter for conditional branches (either on the assumption or on the navigation). Will check later - PR: https://git.openjdk.java.net/jfx/pull/114
Re: [Rev 02] RFR: 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation
On Fri, 28 Feb 2020 17:22:39 GMT, Jeanette Winzenburg wrote: >> Regarding your suggestion of having forward/backward semantic methods and >> calling them in tests - >> There are two types of tests - >> 1. Key action remains same - asserts differ based on NodeOrientation >> 2. Key action differs based on NodeOrientation - but, asserts remain same >> >> Example of 1 is - test_rt18488_selectToLeft >> Example of 2 is - test_rt18591_cell_1 >> >> Your suggestion can be applied only to the tests of type-2. I will try to >> update tests of this type. > > ahh .. okay, missed that difference, thanks :) > > Hmm, still feels smelly to have to use the parameter for conditional branches > (either on the assumption or on the navigation). Will check later and yes, agree with using parameterized tests at all ... just have this nagging feeling that it's not quite done to the end :) - PR: https://git.openjdk.java.net/jfx/pull/114
Re: [Rev 02] RFR: 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation
On Fri, 28 Feb 2020 12:00:23 GMT, Ajit Ghaisas wrote: >> 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. Regarding your suggestion of having forward/backward semantic methods and calling them in tests - There are two types of tests - 1. Key action remains same - asserts differ based on NodeOrientation 2. Key action differs based on NodeOrientation - but, asserts remain same Example of 1 is - test_rt18488_selectToLeft Example of 2 is - test_rt18591_cell_1 Your suggestion can be applied only to the tests of type-2. I will try to update tests of this type. - PR: https://git.openjdk.java.net/jfx/pull/114
Re: [Rev 02] RFR: 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation
On Tue, 25 Feb 2020 15:10:50 GMT, Jeanette Winzenburg
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
Re: [Rev 02] RFR: 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation
On Mon, 24 Feb 2020 17:15:02 GMT, Ajit Ghaisas 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
Re: [Rev 02] RFR: 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation
On Tue, 25 Feb 2020 15:04:05 GMT, Jeanette Winzenburg
wrote:
>> 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);
> }
> }
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.
-
PR: https://git.openjdk.java.net/jfx/pull/114
Re: [Rev 02] RFR: 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation
On Sat, 15 Feb 2020 14:59:30 GMT, Kevin Rushforth wrote: >> The pull request has been updated with 1 additional commit. > > modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewKeyInputTest.java > line 102: > >> 101: >> 102: private NodeOrientation orientation; >> 103: > > I might recommend also adding a `private boolean ltr` instance variable to > make the check less verbose, but that's up to you. In such a test with large number of lines, I prefer the checks to be verbose. - PR: https://git.openjdk.java.net/jfx/pull/114
Re: [Rev 02] RFR: 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation
On Fri, 14 Feb 2020 11:18:12 GMT, Jeanette Winzenburg wrote: >> The pull request has been updated with 1 additional commit. > > modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TableViewBehaviorBase.java > line 139: > >> 138: boolean rtl = isRTL(); >> 139: >> 140: KeyMapping enterKeyActivateMapping, escapeKeyCancelEditMapping; > > node orientation can change during the lifetime of a node ... Addressed this. Please review latest changes. - PR: https://git.openjdk.java.net/jfx/pull/114
Re: [Rev 02] RFR: 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation
> 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. - Added commits: - bf67b084: Remove debug statements from newly added test Changes: - all: https://git.openjdk.java.net/jfx/pull/114/files - new: https://git.openjdk.java.net/jfx/pull/114/files/cbbb884f..bf67b084 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/114/webrev.02 - incr: https://webrevs.openjdk.java.net/jfx/114/webrev.01-02 Stats: 10 lines in 1 file changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jfx/pull/114.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/114/head:pull/114 PR: https://git.openjdk.java.net/jfx/pull/114
