8289547 : Update javax/swing/Popup/TaskbarPositionTest.java

[Renjithkannath]

Hi Reviewers,
Noticed this test case not verifying all the cases which is intended. Modified it for improving the coverage for 3 type of popups (menu, context menu and combobox).

Evaluating conditions:

1. Not enough space for showing popup downwards(default layout), it should show upwards
2. Window starts from negative position, Popup should show on visible area

For achieving this following areas are modified

• Updated isPopupOnScreen by adding Additional checks (like the position of combobox popup is always verified)
• Menu creation made as function and reused for all menu creation.
• Updated ComboPopupCheckListener class and modified its popupMenuWillBecomeInvisible function. Made it as generic and it is capable to evaluate any combo box's position if this class set as its listener.
• This test case is not intended for multi monitor setup so added a check for identifying monitor, else error out.
• Updated CTRL_MASK to CTRL_DOWN_MASK for removing depreciation warnings and removed some other warnings.

Please review this

Regards,
Renjith.

---

Progress

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

Issue

• JDK-8289547: Update javax/swing/Popup/TaskbarPositionTest.java

Reviewers

• Alexey Ivanov (@aivanov-jdk - Reviewer)
• Dmitry Markov (@dmarkov20 - Reviewer) ⚠️ Review applies to b6b521b9
• Harshitha Onkar (@honkar-jdk - Committer) ⚠️ Review applies to a46881b2

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13578

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13578.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back Renjithkannath! 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.

[openjdk]

@Renjithkannath The following label will be automatically applied to this pull request:

• client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 02: Full - Incremental (833dee4a)
• 01: Full - Incremental (a46881b2)
• 00: Full (b6b521b9)

[openjdk]

⚠️ @Renjithkannath the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout 8289547-v2
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

[openjdk]

@Renjithkannath 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:

8289547: Update javax/swing/Popup/TaskbarPositionTest.java

Reviewed-by: aivanov, dmarkov, honkar

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 571 new commits pushed to the master branch:

• 7cf6eec: 8307488: Incorrect weight of the first ObjectAllocationSample JFR event
• a5d8d59: 8308930: [JVMCI] TestUncaughtErrorInCompileMethod times out
• 70130d3: 8306119: Many components respond to a mouse event by requesting focus without supplying the MOUSE_EVENT cause
• 6360b49: 8308948: Remove unimplemented ThreadLocalAllocBuffer::reset
• e21f865: 8308915: RISC-V: Improve temporary vector register usage avoiding the use of v0
• 547a8b4: 8306560: Add TOOLING.jsh load file
• ca54f4e: 8306428: RunThese30M.java crashed with assert(early->flag() == current->flag() || early->flag() == mtNone)
• 5fdb22f: 8308876: JFR: Deserialization of EventTypeInfo uses incorrect attribute names
• bd113ee: 8308814: extend SetLocalXXX minimal support for virtual threads
• a923634: 8286470: Support searching for sections in class/package javadoc
• ... and 561 more: https://git.openjdk.org/jdk/compare/fdaabd6eecd86d1a8b1d1a4ed11cd03996d1db65...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@aivanov-jdk, @dmarkov20, @honkar-jdk) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[Renjithkannath]

/integrate

[openjdk]

@Renjithkannath
Your change (at version b6b521b) is now ready to be sponsored by a Committer.

[dmarkov20]

The new version of the test fails for me on macOS because CTRL + DOWN key combination is reserved by the operating system. It is necessary to disable "Application windows" system key shortcut to execute the test without failure.
An interesting thing is that the old version of the test works fine even if "Application windows" system key shortcut is enabled.
I would suggest documenting (probably at testing.md/wiki page) that "Application windows" key shortcut should be disabled prior testing.

[aivanov-jdk]

The new version of the test fails for me on macOS because CTRL + DOWN key combination is reserved by the operating system. It is necessary to disable "Application windows" system key shortcut to execute the test without failure.

Yes, it's known. It is recommended to disable all system shortcuts for running client tests, see the requirements on the wiki, which may be unfeasible for a system one uses day-to-day.

An interesting thing is that the old version of the test works fine even if "Application windows" system key shortcut is enabled.

It's because the old test didn't open that popup or didn't check its position, I can't remember for sure.

I would suggest documenting (probably at testing.md/wiki page) that "Application windows" key shortcut should be disabled prior testing.

It is already documented per the comment above.

Do you also suggest changing the key stroke to another combination?

[dmarkov20]

The new version of the test fails for me on macOS because CTRL + DOWN key combination is reserved by the operating system. It is necessary to disable "Application windows" system key shortcut to execute the test without failure.

Yes, it's known. It is recommended to disable all system shortcuts for running client tests, see the requirements on the wiki, which may be unfeasible for a system one uses day-to-day.

An interesting thing is that the old version of the test works fine even if "Application windows" system key shortcut is enabled.

It's because the old test didn't open that popup or didn't check its position, I can't remember for sure.

I would suggest documenting (probably at testing.md/wiki page) that "Application windows" key shortcut should be disabled prior testing.

It is already documented per the comment above.

Do you also suggest changing the key stroke to another combination?

Ah, I see. The previous version didn't cover all cases.
It is up to author either to change the key stroke to another one or stay it as is. I am fine with the current version

[honkar-jdk]

Do you also suggest changing the key stroke to another combination?

@aivanov-jdk @Renjithkannath I think the idea of changing the key combination would make it less confusing for someone testing it manually and to figure out that it fails because of Application Windows shortcut & then having to disable it.

Or another option is to add a note in the test itself, probably in jtreg @summary tag or the test frame about having to disable the shortcut in Mac so that the tester is aware.

[Renjithkannath]

@aivanov-jdk , you are right "the old test didn't open that popup and check its position" and similarly some other cases also missed and that was the reason for updating the test case.
Thank you @dmarkov20 bringing this point up and @honkar-jdk for your recommendation. Since system per-requirement is well documented look like we can keep it as it is. @aivanov-jdk please share your recommendation will update based on this.

[Renjithkannath]

Updated @summary of the test case, thank you @dmarkov20 and @honkar-jdk for your review and suggestion. Please let me know is that help.

[honkar-jdk]

LGTM

[aivanov-jdk]

I'm unsure if we're allowed using Unicode chars inside the tests. It shouldn't be a problem here because it's in a comment.

[Renjithkannath]

/integrate

[openjdk]

@Renjithkannath
Your change (at version 833dee4) is now ready to be sponsored by a Committer.

[aivanov-jdk]

/sponsor

[openjdk]

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

• 7cf6eec: 8307488: Incorrect weight of the first ObjectAllocationSample JFR event
• a5d8d59: 8308930: [JVMCI] TestUncaughtErrorInCompileMethod times out
• 70130d3: 8306119: Many components respond to a mouse event by requesting focus without supplying the MOUSE_EVENT cause
• 6360b49: 8308948: Remove unimplemented ThreadLocalAllocBuffer::reset
• e21f865: 8308915: RISC-V: Improve temporary vector register usage avoiding the use of v0
• 547a8b4: 8306560: Add TOOLING.jsh load file
• ca54f4e: 8306428: RunThese30M.java crashed with assert(early->flag() == current->flag() || early->flag() == mtNone)
• 5fdb22f: 8308876: JFR: Deserialization of EventTypeInfo uses incorrect attribute names
• bd113ee: 8308814: extend SetLocalXXX minimal support for virtual threads
• a923634: 8286470: Support searching for sections in class/package javadoc
• ... and 561 more: https://git.openjdk.org/jdk/compare/fdaabd6eecd86d1a8b1d1a4ed11cd03996d1db65...master

Your commit was automatically rebased without conflicts.

[openjdk]

@aivanov-jdk @Renjithkannath Pushed as commit d73fc70.

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