8347836: Disabled PopupMenu shows shortcuts on Mac #23402

DamonGuy · 2025-01-31T19:54:29Z

The test instructions say that disabled PopupMenus should not have shortcuts shown, but on MacOS, these shortcuts still appear. When checking native MacOS15 behavior, disabled PopupMenus still have shortcuts shown. Since the test doesn't modify the popup's shortcuts other than adding the shortcut for A, it makes sense that the result matches native behavior. So, I modified the test instructions instead to exclude MacOS from this step.

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-8347836: Disabled PopupMenu shows shortcuts on Mac (Bug - P4)

Reviewers

Abhishek Kumar (@kumarabhi006 - Reviewer) Review applies to 1cc6c40e
Alexander Zvegintsev (@azvegint - Reviewer)
Alisen Chung (@alisenchung - Committer)
Alexander Zuev (@azuev-java - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23402

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-01-31T19:55:33Z

👋 Welcome back dnguyen! 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 · 2025-01-31T19:56:26Z

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

8347836: Disabled PopupMenu shows shortcuts on Mac

Reviewed-by: azvegint, achung, kizune, abhiscxk

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

b9b62a0: 8346792: serviceability/jvmti/vthread/GetThreadState/GetThreadState.java testObjectWaitMillis failed
6b994cd: 8333697: C2: Hit MemLimit in PhaseCFG::global_code_motion
2ff8440: 8349344: Clarify documentation of Arena.ofConfined
19399d2: 8348572: C2 compilation asserts due to unexpected irreducible loop
6146588: 8348190: Framework for tracing makefile inclusion and parsing
66a3898: 8348659: AArch64: IR rule failure with compiler/loopopts/superword/TestSplitPacks.java
40603a5: 8349214: Improve size optimization flags for MSVC builds
0926949: 8347629: Test FailOverDirectExecutionControlTest.java fails with -Xcomp
a51e669: 8349200: [JMH] time.format.ZonedDateTimeFormatterBenchmark fails
9d23de5: 8184352: Remove Sun provider information from KeyPairGenerator javadoc
... and 301 more: https://git.openjdk.org/jdk/compare/379d05bcc130446086786ecf6ca5a6b8e977386c...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.

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

openjdk · 2025-01-31T19:57:08Z

@DamonGuy 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 · 2025-01-31T20:00:28Z

Webrevs

kumarabhi006

You may remove .rows((int) INSTRUCTIONS.lines().count() + 2), not mandatory though.

kumarabhi006 · 2025-02-03T07:00:58Z

test/jdk/java/awt/PopupMenu/PopupMenuVisuals.java

@@ -47,7 +47,7 @@ public class PopupMenuVisuals {
         If following conditions are met:
          - Menu is disabled
          - Menu has caption 'Popup menu' (only applicable for linux)
-          - Menu items don't show shortcuts
+          - Menu items don't show shortcuts (except on MacOS)


Is it good to restrict the test for "Windows and Linux" only ?

It still tests for PopupMenu visuals on MacOS I suppose. We can still see if a disabled PopupMenu appears correctly. It's also a manual test. Not sure if restricting the test is actually required unless there's a bigger reason I'm missing.

test/jdk/java/awt/PopupMenu/PopupMenuVisuals.java

kumarabhi006

Test changes looks good to me.

azvegint · 2025-02-03T18:25:47Z

test/jdk/java/awt/PopupMenu/PopupMenuVisuals.java

@@ -47,7 +47,7 @@ public class PopupMenuVisuals {
         If following conditions are met:
          - Menu is disabled
          - Menu has caption 'Popup menu' (only applicable for linux)


I believe we can only print instructions that are specific to the current platform.

e.g.:

--- a/test/jdk/java/awt/PopupMenu/PopupMenuVisuals.java +++ b/test/jdk/java/awt/PopupMenu/PopupMenuVisuals.java @@ -20,14 +20,17 @@ * or visit www.oracle.com if you need additional information or have any * questions. */ + /* * @test * @bug 6180413 6184485 6267144 * @summary test for popup menu visual bugs in XAWT - * @library /java/awt/regtesthelpers - * @build PassFailJFrame + * @library /java/awt/regtesthelpers /test/lib + * @build PassFailJFrame jdk.test.lib.Platform * @run main/manual PopupMenuVisuals -*/ + */ + +import jdk.test.lib.Platform; import java.awt.Button; import java.awt.CheckboxMenuItem; @@ -45,11 +48,13 @@ public class PopupMenuVisuals { This test should show a button 'Popup'. Click on the button. A popup menu should be shown. If following conditions are met: - - Menu is disabled - - Menu has caption 'Popup menu' (only applicable for linux) - - Menu items don't show shortcuts (except on MacOS) + - Menu is disabled %s%s - Click Pass else click Fail."""; + Click Pass else click Fail.""" + .formatted( + Platform.isLinux() ? "\n - Menu has caption 'Popup menu'" : "", + !Platform.isOSX() ? "\n - Menu items don't show shortcuts" : "" + ); static PopupMenu pm; static Frame frame;

I think it will improve a test user experience.

Neat! Learned something new. Will definitely use this where applicable in the future. Added.

alisenchung · 2025-02-04T00:24:38Z

test/jdk/java/awt/PopupMenu/PopupMenuVisuals.java

@@ -79,9 +83,9 @@ private static Frame createTestUI() {
        CheckboxMenuItem mi3 = new CheckboxMenuItem("Item 3");
        Menu sm = new Menu("Submenu");

-        //Get things going.  Request focus, set size, et cetera
+        // Get things going.  Request focus, set size, et cetera


can just remove this comment since it's an artifact from Applet testing

DamonGuy · 2025-02-05T18:42:18Z

/integrate

openjdk · 2025-02-05T18:43:21Z

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

82bc0a7: 8344316: security/auth/callback/TextCallbackHandler/Password.java make runnable with JTReg and add the UI
2f2f7cf: 8349084: Update vectors used in several PQC benchmarks
b9b62a0: 8346792: serviceability/jvmti/vthread/GetThreadState/GetThreadState.java testObjectWaitMillis failed
6b994cd: 8333697: C2: Hit MemLimit in PhaseCFG::global_code_motion
2ff8440: 8349344: Clarify documentation of Arena.ofConfined
19399d2: 8348572: C2 compilation asserts due to unexpected irreducible loop
6146588: 8348190: Framework for tracing makefile inclusion and parsing
66a3898: 8348659: AArch64: IR rule failure with compiler/loopopts/superword/TestSplitPacks.java
40603a5: 8349214: Improve size optimization flags for MSVC builds
0926949: 8347629: Test FailOverDirectExecutionControlTest.java fails with -Xcomp
... and 303 more: https://git.openjdk.org/jdk/compare/379d05bcc130446086786ecf6ca5a6b8e977386c...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-02-05T18:43:31Z

@DamonGuy Pushed as commit 379c3f9.

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

DamonGuy added 4 commits November 23, 2024 20:39

Initial commit for backout

c51406a

Merge branch 'master' of github.com:DamonGuy/jdk

a98fb87

Merge branch 'master' of github.com:DamonGuy/jdk

947f73d

Initial commit

145cf9f

openjdk bot added the rfr Pull request is ready for review label Jan 31, 2025

openjdk bot added the client [email protected] label Jan 31, 2025

kumarabhi006 reviewed Feb 3, 2025

View reviewed changes

Review comments

1cc6c40

kumarabhi006 reviewed Feb 3, 2025

View reviewed changes

test/jdk/java/awt/PopupMenu/PopupMenuVisuals.java Outdated Show resolved Hide resolved

kumarabhi006 approved these changes Feb 3, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 3, 2025

azvegint reviewed Feb 3, 2025

View reviewed changes

DamonGuy added 2 commits February 3, 2025 10:57

Add platform conditionals to test instructions

b41b27b

Separate comment blocks

fb78b9e

openjdk bot removed the ready Pull request is ready to be integrated label Feb 3, 2025

azvegint approved these changes Feb 3, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 3, 2025

alisenchung approved these changes Feb 4, 2025

View reviewed changes

azuev-java approved these changes Feb 5, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 5, 2025

openjdk bot closed this Feb 5, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8347836: Disabled PopupMenu shows shortcuts on Mac #23402

8347836: Disabled PopupMenu shows shortcuts on Mac #23402

DamonGuy commented Jan 31, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 31, 2025

openjdk bot commented Jan 31, 2025 •

edited

Loading

openjdk bot commented Jan 31, 2025

mlbridge bot commented Jan 31, 2025 •

edited

Loading

kumarabhi006 left a comment

kumarabhi006 Feb 3, 2025

DamonGuy Feb 3, 2025

kumarabhi006 left a comment

azvegint Feb 3, 2025 •

edited

Loading

DamonGuy Feb 3, 2025

alisenchung Feb 4, 2025

DamonGuy commented Feb 5, 2025

openjdk bot commented Feb 5, 2025

openjdk bot commented Feb 5, 2025

8347836: Disabled PopupMenu shows shortcuts on Mac #23402

8347836: Disabled PopupMenu shows shortcuts on Mac #23402

Conversation

DamonGuy commented Jan 31, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 31, 2025

openjdk bot commented Jan 31, 2025 • edited Loading

openjdk bot commented Jan 31, 2025

mlbridge bot commented Jan 31, 2025 • edited Loading

Webrevs

kumarabhi006 left a comment

Choose a reason for hiding this comment

kumarabhi006 Feb 3, 2025

Choose a reason for hiding this comment

DamonGuy Feb 3, 2025

Choose a reason for hiding this comment

kumarabhi006 left a comment

Choose a reason for hiding this comment

azvegint Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

DamonGuy Feb 3, 2025

Choose a reason for hiding this comment

alisenchung Feb 4, 2025

Choose a reason for hiding this comment

DamonGuy commented Feb 5, 2025

openjdk bot commented Feb 5, 2025

openjdk bot commented Feb 5, 2025

DamonGuy commented Jan 31, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jan 31, 2025 •

edited

Loading

mlbridge bot commented Jan 31, 2025 •

edited

Loading

azvegint Feb 3, 2025 •

edited

Loading