Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation #114

Closed
wants to merge 8 commits into from

Conversation

@aghaisas
Copy link
Collaborator

aghaisas commented Feb 14, 2020

Bug : https://bugs.openjdk.java.net/browse/JDK-8235480

Fix : Added the missed out RTL checks to the key mappings in TableViewBehaviorBase class.

Testing : Added a test to take NodeOrientation as a parameter and test horizontal key navigation for the TableView.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation

Reviewers

  • Kevin Rushforth (kcr - Reviewer)
  • Jeanette Winzenburg (fastegal - Author)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/114/head:pull/114
$ git checkout pull/114

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 14, 2020

👋 Welcome back aghaisas! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@aghaisas aghaisas changed the title [WIP] 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation Feb 14, 2020
@openjdk openjdk bot added the rfr label Feb 14, 2020
@mlbridge
Copy link

mlbridge bot commented Feb 14, 2020

Copy link
Member

kevinrushforth left a comment

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

@kevinrushforth
Copy link
Member

kevinrushforth commented Feb 21, 2020

/reviewers 2

@openjdk
Copy link

openjdk bot commented Feb 21, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).


if (orientation == NodeOrientation.LEFT_TO_RIGHT) {
keyboard.doRightArrowPress(KeyModifier.getShortcutKey());
keyboard.doRightArrowPress(KeyModifier.getShortcutKey());
} else if (orientation == NodeOrientation.RIGHT_TO_LEFT) {
keyboard.doLeftArrowPress(KeyModifier.getShortcutKey());
keyboard.doLeftArrowPress(KeyModifier.getShortcutKey());
}
Comment on lines 1134 to 1141

This comment has been minimized.

@kleopatra

kleopatra Feb 25, 2020 Collaborator

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);
    }
}

This comment has been minimized.

@kleopatra

kleopatra Feb 25, 2020 Collaborator

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.

This comment has been minimized.

@aghaisas

aghaisas Feb 28, 2020 Author Collaborator

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.

This comment has been minimized.

@aghaisas

aghaisas Feb 28, 2020 Author Collaborator

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.

This comment has been minimized.

@kleopatra

kleopatra Feb 28, 2020 Collaborator

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

This comment has been minimized.

@kleopatra

kleopatra Feb 28, 2020 Collaborator

and yes, agree with using parameterized tests at all ... just have this nagging feeling that it's not quite done to the end :)

@kleopatra
Copy link
Collaborator

kleopatra commented Mar 1, 2020

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

  1. has no effect because it's already the upper boundary
  2. nothing to shrink, instead it extends by one
  3. 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)

@openjdk openjdk bot removed the rfr label Mar 6, 2020
@openjdk openjdk bot added the rfr label Mar 6, 2020
@aghaisas
Copy link
Collaborator Author

aghaisas commented Mar 6, 2020

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
Copy link
Member

kevinrushforth left a comment

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.

import test.com.sun.javafx.scene.control.infrastructure.StageLoader;

/**
* Test basic horizontal navigation mappings for TableView. It's parametrized on NodeOrientation

This comment has been minimized.

@kevinrushforth

kevinrushforth Mar 6, 2020 Member

Typo: should be parameterized

@kleopatra
Copy link
Collaborator

kleopatra commented Mar 12, 2020

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

Copy link
Collaborator

kleopatra left a comment

sry for the spelling errors you inherited :)

@openjdk openjdk bot removed the rfr label Mar 13, 2020
@openjdk openjdk bot added the rfr label Mar 13, 2020
@kleopatra
Copy link
Collaborator

kleopatra commented Mar 17, 2020

@aghaisas can't see your last response here (looks like I'm fighting the system here - and loosing again ;) So copying from the mailing list:

I have the updated the method documentation (also changed the method name to toggleNodeOrientation() to clearly reflect
what it does)

good move!

There would always be different ways to test the same thing. I think, current test is in good shape and suffices the
purpose of unit testing this fix.

yeah, I agree - with a minor matter: the dynamic change testing is for a single key combination only. Personally, I prefer test completeness to really grab all changes (here all key combinations): future maintainers sometimes have ideas (f.i. inlining the isRTL, and accidentally not doing it on the simple keys) which they should be protected from doing - up to you, though.

Assuming that Behaviours will move into public scope, shouldn't public/protected api documented when added (as isRtL())? Not sure about fx team internal conventions.

Anyway, all side-thought, so feel free to commit

Copy link
Member

kevinrushforth left a comment

Looks good to me.

This needs one more reviewer. @kleopatra would you be willing?

Copy link
Collaborator

kleopatra left a comment

yeah sure .. thought I did already approve, looks like I didn't find the correct way - trying again

@openjdk
Copy link

openjdk bot commented Mar 23, 2020

@aghaisas This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

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

Reviewed-by: kcr, fastegal
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 28 commits pushed to the master branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 2aa821855e031a2fa193c2de1515c627dbc78ef3.

➡️ To integrate this PR with the above commit message, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Mar 23, 2020
@aghaisas
Copy link
Collaborator Author

aghaisas commented Mar 24, 2020

/integrate

@openjdk openjdk bot closed this Mar 24, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 24, 2020
@openjdk
Copy link

openjdk bot commented Mar 24, 2020

@aghaisas The following commits have been pushed to master since your change was applied:

  • e9c6119: 8240692: Cleanup of the javafx property objects
  • 90289a2: 8239107: Update libjpeg to version 9d
  • b81cf32: 8236259: MemoryLeak in ProgressIndicator
  • 9ea7f96: 8240832: Remove unused applecoreaudio.md third-party legal file
  • 0fc1420: Merge
  • f8c235b: 8240631: Create release notes for JavaFX 14
  • 50e15fc: 8240466: AppJSCallback* apps launched by ModuleLauncherTest intermittently hang
  • e3026b9: 8240688: Remove the JavaBeanXxxPropertyBuilders constructors
  • cfa1193: 8236685: [macOs] Remove obsolete file dialog subclasses
  • f25e8cf: 8212034: Potential memory leaks in jpegLoader.c in error case
  • b2ac76a: 8240451: JavaFX javadoc build fails with JDK 14
  • cf0bba6: 8240211: Stack overflow on Windows 32-bit can lead to crash
  • 337ed72: 8237926: Potential memory leak of model data in javafx.scene.control.ListView
  • 960f039: 8208761: Update constant collections to use the new immutable collections
  • 10c9528: 8240265: iOS: Unnecessary logging on pinch gestures
  • 4c132cd: 8237889: Update libxml2 to version 2.9.10
  • 20328b3: 8240218: IOS Webkit implementation broken
  • 4c82af8: 8236832: [macos 10.15] JavaFX Application hangs on video play on Cata…
  • 9cd6f79: 8196586: Remove use of deprecated finalize methods from javafx property objects
  • ef2f9ce: 8238755: allow to create static lib for javafx.media on linux
  • c3ee1a3: 8239822: Intermittent unit test failures in RegionCSSTest
  • 3150562: 8235772: Remove use of deprecated finalize method from PiscesRenderer and AbstractSurface
  • 4eaff0d: 8239109: Update SQLite to version 3.31.1
  • d8e7f85: 8239454: LLIntData : invalid opcode returned for 16 and 32 bit wide instructions
  • 48ddd80: Merge
  • 35a01ca: 8228867: Fix mistakes in FX API docs
  • b5e65ec: 8238434: Ensemble: Update version of Lucene to 7.7.2

Your commit was automatically rebased without conflicts.

Pushed as commit 2aa8218.

@aghaisas aghaisas deleted the aghaisas:bug_fix_branch branch Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.