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

8314906: [testbug] Create behavior tests for text input controls #1221

Conversation

andy-goryachev-oracle
Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle commented Aug 23, 2023

Creating the first batch of tests and testing framework that enables writing behavior tests for javafx controls, focusing on key bindings. The idea is to make writing such tests a simple process.

This PR deals with the descendants of TextInputControl (TextField, PasswordField, TextArea). Most of the tests are headless, but in some cases (TextArea) a headful test is required because the behavior needs rendered text to function (example: page up / page down / line start / line end and the like).

The tests exercise the key bindings registered by the Skin (or, rather the associated Behavior) at least once, and sometimes more than once.

Some mappings cannot be tested due to Robot not supporting keypad events (created JDK-8316307).

In addition, the key bindings are documented in /doc-files/behavior markdown documents:

https://github.com/andy-goryachev-oracle/jfx/blob/8314906.behavior.test/doc-files/behavior/PasswordField.md
https://github.com/andy-goryachev-oracle/jfx/blob/8314906.behavior.test/doc-files/behavior/TextArea.md
https://github.com/andy-goryachev-oracle/jfx/blob/8314906.behavior.test/doc-files/behavior/TextField.md
https://github.com/andy-goryachev-oracle/jfx/blob/8314906.behavior.test/doc-files/behavior/TextInputControl.md


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

  • JDK-8314906: [testbug] Create behavior tests for text input controls (Bug - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1221/head:pull/1221
$ git checkout pull/1221

Update a local copy of the PR:
$ git checkout pull/1221
$ git pull https://git.openjdk.org/jfx.git pull/1221/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1221

View PR using the GUI difftool:
$ git pr show -t 1221

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1221.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 23, 2023

👋 Welcome back angorya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

"8", KeyCode.DIGIT8,
"9", KeyCode.DIGIT9,
".", KeyCode.PERIOD,
",", KeyCode.COMMA
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On French layouts the DIGIT codes don't actually produce digits (you have to hold down Shift) and there's no KeyCode.PERIOD (also a shifted character). You don't seem to be using those characters but you might want to remove them from this table so no one is tempted to use them in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting.

It is my understanding though that for the purposes of this test it should work as expected. I've added TextAreaBehaviorRobotTest.testTyping() - could you try running it with your setup please?

    @Test
    public void testTyping() throws Exception {
        execute(
            //addKeyListener(),
            "0123456789.,abracadabra",
            checkText("0123456789.,abracadabra", 23)
        );
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran the TextAreaBehaviorRobotTest on macOS 13.5.2. Test passed with my keyboard set to U.S. English and failed when set to French.

TextAreaBehaviorRobotTest > testTyping() FAILED
    org.opentest4j.AssertionFailedError: in step 2 ==> expected: <0123456789.,abracadabra> but was: <à&é"'(§è!ç,abracadabra>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merci, Monsieur Fox!

We seem to have quite a few tickets in this area (who created the first one in the list?):

  • JDK-8278924 [Linux] Robot key test can fail if multiple keyboard layouts are installed
  • JDK-8022079 [macosx] swing shortcuts are counterintuitive with non-english keyboard layouts
  • JDK-8087915 Mac: accelerator doesn't take into account azerty keyboard layout
  • JDK-4760902 java.awt.Robot can't generate % character on FRENCH keyboard

Not only we get key events for wrong characters, but also shortcut(A) generates character = q, text = , code = UNDEFINED, metaDown, shortcutDown] which closes the test app :-(

I have no idea how to fix this test... We could try to introduce a translation layer to emit the right sequences (for each keyboard layout), or, alternatively, we could skip the tests altogether for non-US keyboard, except ... there seems to be no easy way to determine the keyboard layout? (There are some suggestions involving InputContext or JNI on stackoverflow). We could also try to skip the test if Locale.getDefault is other than Locale.US, not sure if that would solve the issue.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem you're running into here isn't due to any bug in JavaFX. The way KeyCodes identify keys works well for defining accelerators but not for using a Robot to generate a specific printable character. As an extreme example, if you're using a Greek keyboard there will be a key that has KeyCode.Z assigned to it so you can use Shortcut+Z to invoke Undo. But that key will generate a Greek letter when pressed by a human or a Robot.

You can probably assume that anyone running these tests is using a Latin/Roman keyboard that can generate the lower-case letters a-z so one approach is to just stick to those characters in the typing tests. The other is to try to identify the layout using a Robot. For example, send KeyCode.DIGIT1 and see if that generates a '1'. If not you're probably on a French layout.

Punctation and symbols are hopeless, there's just too much variation in the keyboard layouts.

Not only we get key events for wrong characters, but also shortcut(A) generates character = q, text = , code = UNDEFINED, metaDown, shortcutDown] which closes the test app :-(

Could you provide more details (platform, keyboard, keyboard layout, etc.)? On the Mac PR #425 cleaned all this up in jfx21.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am running this on macOS 13.5.1 with several keyboard layouts enabled.

A test that does keyPress(KeyCode.A) followed by a keyRelease(KeyCode.A) generates the following sequence of events:

With the US layout:

KeyEvent [source = TextArea@7da926e[styleClass=text-input text-area], target = TextArea@7da926e[styleClass=text-input text-area], eventType = KEY_PRESSED, consumed = false, character = , text = a, code = A]
KeyEvent [source = TextArea@7da926e[styleClass=text-input text-area], target = TextArea@7da926e[styleClass=text-input text-area], eventType = KEY_TYPED, consumed = false, character = a, text = , code = UNDEFINED]
KeyEvent [source = TextArea@7da926e[styleClass=text-input text-area], target = TextArea@7da926e[styleClass=text-input text-area], eventType = KEY_RELEASED, consumed = false, character = , text = a, code = A]

With the French layout:

KeyEvent [source = TextArea@75ffb50a[styleClass=text-input text-area], target = TextArea@75ffb50a[styleClass=text-input text-area], eventType = KEY_PRESSED, consumed = false, character = , text = q, code = A]
KeyEvent [source = TextArea@75ffb50a[styleClass=text-input text-area], target = TextArea@75ffb50a[styleClass=text-input text-area], eventType = KEY_TYPED, consumed = false, character = q, text = , code = UNDEFINED]
KeyEvent [source = TextArea@75ffb50a[styleClass=text-input text-area], target = TextArea@75ffb50a[styleClass=text-input text-area], eventType = KEY_RELEASED, consumed = false, character = , text = q, code = A]

And with a cyrillic layout it generates whatever cyrillic letter is mapped to the key. So, basically, KeyCode is more like a scan code, which means there is no reliable way to test key bindings using Robot, unless we limit ourselves (and the test) to the US layout. Or is there another way?

Screenshot 2023-09-18 at 13 19 51

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the behavior we saw on the Mac before PR #425 was integrated. As of JavaFX 21 keyPress(KeyCode.A) should generate an 'a' and never a 'q' on all Latin layouts and all platforms (on a Cyrillic layout you'll still get a Cyrillic character).

I can't reproduce this with the test app (tests/manual/events/KeyboardTest.java). Is there a test in this PR I should be trying?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could try this code (using this PR's code):

public class KeyboardLayoutTest extends TextInputBehaviorRobotTest<TextArea> {

    public KeyboardLayoutTest() {
        super(new TextArea());
    }

    @BeforeEach
    @Override
    public void beforeEach() {
        super.beforeEach();
        control.setWrapText(true);
    }

    @Test
    public void testTyping() throws Exception {
        KeyCode[] codes = {
            KeyCode.A
        };

        execute(
            exe(() -> {
                control.addEventFilter(KeyEvent.ANY, (ev) -> {
                    System.out.println(ev);
                    ev.consume();
                });
            })
        );

        for(KeyCode cd: codes) {
            Util.runAndWait(() -> {
                robot.keyPress(cd);
                robot.keyRelease(cd);
            });
        }
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried your test code and couldn't reproduce the problem. Both U.S. English and French layouts gave me the expected result (an 'a' character). I also tried a Russian keyboard just to verify that it would generate a Cyrillic character which it did. I'm running the tests against JavaFX built using the sources in your pull request.

French

KeyEvent [source = TextArea@7237703[styleClass=text-input text-area], target = TextArea@7237703[styleClass=text-input text-area], eventType = KEY_PRESSED, consumed = false, character = ?, text = a, code = A]
KeyEvent [source = TextArea@7237703[styleClass=text-input text-area], target = TextArea@7237703[styleClass=text-input text-area], eventType = KEY_TYPED, consumed = false, character = a, text = , code = UNDEFINED]
KeyEvent [source = TextArea@7237703[styleClass=text-input text-area], target = TextArea@7237703[styleClass=text-input text-area], eventType = KEY_RELEASED, consumed = false, character = ?, text = a, code = A]

U.S. English

KeyEvent [source = TextArea@73459ba5[styleClass=text-input text-area], target = TextArea@73459ba5[styleClass=text-input text-area], eventType = KEY_PRESSED, consumed = false, character = ?, text = a, code = A]
KeyEvent [source = TextArea@73459ba5[styleClass=text-input text-area], target = TextArea@73459ba5[styleClass=text-input text-area], eventType = KEY_TYPED, consumed = false, character = a, text = , code = UNDEFINED]
KeyEvent [source = TextArea@73459ba5[styleClass=text-input text-area], target = TextArea@73459ba5[styleClass=text-input text-area], eventType = KEY_RELEASED, consumed = false, character = ?, text = a, code = A]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your help! I can confirm that the latest code does indeed generate the right key event.

I suspect there was something in my setup that got corrupted - I had to install the latest eclipse, found out it's pretty much unusable due to eclipse-jdt/eclipse.jdt.debug#309 , had to revert back to 2023-06 just to discover that my macOS happily suggested that 'your application is damaged and can't be opened' and do I want to move it to Trash? So after some xattr'ing, restoring proxy settings, etc., etc. I am back online and the issue is gone.

I did remove the offending characters and modified the tests, so thank you once again - your help was invaluable.

private final EventTarget target;
private final Scene scene;
private static HashMap<Character,KeyCode> keyCodes;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor : space after ,


private static HashMap<Character, KeyCode> createKeyCodes(Object ... pairs) {
HashMap<Character, KeyCode> m = new HashMap<>();
for(int i=0; i<pairs.length; ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor : space before and after = and <

* closeStage();
* }
* <pre>
* @param control the control being tested
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove @param line

@Test
@Override
public void testCopy() {
// copy is disabled
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An empty test case simply gets reported as passed.
Do you want to assert that the "copy is disabled" somehow?
There are a few other test cases in this file which are of similar nature.
If they need to be worked upon in future, we can mark them as TODO.
If they are just for information, we can keep the entire test cases commented so that they do not execute and report as passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, thanks!

@Test
@Override
public void testConsumeEnter() {
// does not consume ENTER
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment about empty test cases in this file.

Copy link
Collaborator

@aghaisas aghaisas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look good to me!

@andy-goryachev-oracle
Copy link
Contributor Author

@karthikpandelu could you also take a look at this please?

Copy link
Member

@karthikpandelu karthikpandelu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests look good to me.
Left few minor comments inline.

@@ -0,0 +1,74 @@
# PasswordField Behavior

## Key Binginds
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: typo "Binginds"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first rule of submitting scientific publications for a review: make an obvious spelling error in the title.
:-)

thanks!

@@ -0,0 +1,88 @@
# TextArea Behavior

## Key Binginds
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: typo "Binginds"

@@ -0,0 +1,74 @@
# TextField Behavior

## Key Binginds
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: typo "Binginds"

@@ -0,0 +1,73 @@
# TextInputControl Behavior

## Key Binginds
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: typo "Binginds"


Notes:

1. Base class mappings modified by the TextField class are highlighted in bold.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the key mappings are highlighted in bold in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

none have been modified, so we are ok.

* initStage(new ACTUAL_CONTROL());
* }
* <pre>
* @param control the control being tested
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameter can be renamed to control or comment can be updated.

Copy link
Member

@karthikpandelu karthikpandelu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@openjdk
Copy link

openjdk bot commented Oct 13, 2023

@andy-goryachev-oracle This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8314906: [testbug] Create behavior tests for text input controls

Reviewed-by: aghaisas, kpk

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 5 new commits pushed to the master branch:

  • 4604e87: 8318059: Typo is javafx.scene.Node.usesMirroring comment
  • e621d4b: 8314484: Update Gradle to 8.4
  • 73e690f: 8314486: JavaFX build uses deprecated features that will be removed in gradle 8
  • ec9a11b: 8316590: Rendering artifact after JDK-8311983
  • 2ec3343: 8313628: Column drag header, overlay and line are not correctly aligned

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

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

@openjdk openjdk bot added the ready Ready to be integrated label Oct 13, 2023
@andy-goryachev-oracle
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Oct 13, 2023

Going to push as commit f76437d.
Since your change was applied there have been 5 commits pushed to the master branch:

  • 4604e87: 8318059: Typo is javafx.scene.Node.usesMirroring comment
  • e621d4b: 8314484: Update Gradle to 8.4
  • 73e690f: 8314486: JavaFX build uses deprecated features that will be removed in gradle 8
  • ec9a11b: 8316590: Rendering artifact after JDK-8311983
  • 2ec3343: 8313628: Column drag header, overlay and line are not correctly aligned

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Oct 13, 2023
@openjdk openjdk bot closed this Oct 13, 2023
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review labels Oct 13, 2023
@openjdk
Copy link

openjdk bot commented Oct 13, 2023

@andy-goryachev-oracle Pushed as commit f76437d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@andy-goryachev-oracle andy-goryachev-oracle deleted the 8314906.behavior.test branch October 13, 2023 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
5 participants