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

8048109: JToggleButton does not fire actionPerformed under certain conditions #600

Closed
wants to merge 8 commits into from

Conversation

trebari
Copy link
Member

@trebari trebari commented Oct 12, 2020

Please review the following fix for jdk16.

Issue : There is a JToggleButton that will post/take down a JPopupMenu when the button is selected. If the button is selected and the menu is not posted the action listener will post the menu. If the button is selected and the menu is displayed the action listener will take the menu down. The use case is:
1 - select button
2 - menu posted
3 - select button
4 - menu taken down

With MetalLookAndFeel the above use case works fine, but with WindowsLookAndFeel the second button selection does not fire the actionPerformed event, button needs to be selected third time for the menu to be taken down.

The issue is that the button must be selected twice after the menu is posted to have the actionPerformed event to fire when using the Windows look and feel.

Fix : MouseGrabber is not removed while uninstalling the listeners in the BasicPopupMenuUI.
By removing the mouseGrabber in the uninstallListeners() methods fixes this issue.

Added a test to test the same in all the LookAndFeels


Progress

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

Issue

  • JDK-8048109: JToggleButton does not fire actionPerformed under certain conditions

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/600/head:pull/600
$ git checkout pull/600

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 12, 2020

👋 Welcome back trebari! 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
Copy link

openjdk bot commented Oct 12, 2020

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

  • swing

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.

@openjdk openjdk bot added the swing label Oct 12, 2020
@openjdk openjdk bot added the rfr label Oct 13, 2020
@mlbridge
Copy link

mlbridge bot commented Oct 13, 2020

Webrevs

Copy link

@psadhukhan-personal psadhukhan-personal left a comment

Hi @psadhukhan-personal, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user psadhukhan-personal for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 12, 2020

@trebari This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@mrserb
Copy link
Member

mrserb commented Nov 30, 2020

@prsadhuk looks like your comment was added using the wrong account.

@prsadhuk
Copy link
Contributor

prsadhuk commented Nov 30, 2020

not wrong but different account but that doesnot make the comment invalid, I guess.

@mrserb
Copy link
Member

mrserb commented Nov 30, 2020

not wrong but different account but that doesnot make the comment invalid, I guess.

But nobody sees it, it is hidden.

@prsadhuk
Copy link
Contributor

prsadhuk commented Nov 30, 2020

Well, the comment was added on Oct 15 and it got hidden on Oct28 because the ac was not of github author/committer. I think the fix submitter should have seen the comment lying there for 2 weeks before it got hidden.

@trebari
Copy link
Member Author

trebari commented Dec 1, 2020

I have seen the comment, it is not visible in github but its there in mailing-list.

"It seems the effect is in WindowsLookAndFeel but you are fixing in Basic L&F. If the cause is in Basic L&F, then it
would have affected other L&F also like Metal,Nimbus, doesn't it? If MouseGrabber not removed is the problem, the I
guess MouseGrabber.uninstall() is not called. This is called in BasicLookAndFeel#uninitialize() and is overridden in
WindowsLookAndFeel. Metal does not override this so maybe that's why the issue is not seen in Metal. Probably the
right fix is to call this MouseGrabber.uninstall() in Windows:LookAndFeel.uninitialize() same way as is done in
BasicLookAndFeel.uninitialize()"

the current fix doesn't looks correct so working on finding the correct fix.

@openjdk openjdk bot removed the rfr label Dec 10, 2020
@trebari
Copy link
Member Author

trebari commented Dec 10, 2020

In case of the first attempt to bring down the popup menu the event was consumed by following code of BasicPopupMenuUI
boolean consumeEvent =
UIManager.getBoolean("PopupMenu.consumeEventOnClose");
// Consume the event so that normal processing stops.
if(consumeEvent && !(src instanceof MenuElement)) {
me.consume();
}
The PopupMenu.consumeEventOnClose is true for Windows, Motif, Nimbus, and GTK LAF and this issue is seen in all these LookAndFeels.
For Metal LAF this variable is never set to true so it uses of BasicLookAndFeel which is set to FALSE
This issue is not seen on MetalLAF.

So changed PopupMenu.consumeEventOnClose to FALSE for Windows, Motif, Nimbus, and GTK LAF and
it fixes the issue.

@openjdk openjdk bot added the rfr label Dec 10, 2020
@prsadhuk
Copy link
Contributor

prsadhuk commented Dec 11, 2020

Any idea why commenting popup.setInvoker(jtb); line in the test works even without the fix?

@trebari
Copy link
Member Author

trebari commented Dec 12, 2020

In case of commenting popup.setInvoker(jtb);
the following line of code in BasicPopupMenuUI is never called

boolean consumeEvent =UIManager.getBoolean("PopupMenu.consumeEventOnClose");
// Consume the event so that normal processing stops.
if(consumeEvent && !(src instanceof MenuElement)) {
me.consume();
}

So the event is not consumed here by me.consume() and the the issue is not seen.

@openjdk
Copy link

openjdk bot commented Dec 14, 2020

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

8048109: JToggleButton does not fire actionPerformed under certain conditions

Reviewed-by: serb, psadhukhan, vdyakov

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

  • 270014a: 8234131: Miscellaneous changes imported from jsr166 CVS 2021-01
  • 63e3bd7: 8246677: LinkedTransferQueue and SynchronousQueue synchronization updates
  • 5cfa8c9: 8246585: ForkJoin updates
  • 6472104: 6278172: TextComponent.getSelectedText() throws StringIndexOutOfBoundsException
  • a653928: 8259512: Update --release 16 symbol information for JDK 16 build 31
  • 7e6677b: 8259446: runtime/jni/checked/TestCheckedReleaseArrayElements.java fails with stderr not empty
  • 628c546: 8258796: [test] Apply HexFormat to tests for java.security
  • 876c7fb: 8259493: [test] Use HexFormat instead of adhoc hex utilities in network code and locale SoftKeys
  • 090bd3a: 8259397: ThreadsSMRSupport::print_info_on() should use try_lock_without_rank_check()
  • 10a6b0d: 8234773: Fix ThreadsSMRSupport::_bootstrap_list
  • ... and 1457 more: https://git.openjdk.java.net/jdk/compare/04775f11fe637029b6040b494bb495939c6d3fcf...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 openjdk bot added the ready label Dec 14, 2020
@trebari
Copy link
Member Author

trebari commented Dec 16, 2020

@mrserb please take a look.

@@ -803,7 +803,7 @@ public Object createValue(UIDefaults table) {
"PasswordField.font", new FontLazyValue(Region.PASSWORD_FIELD),


"PopupMenu.consumeEventOnClose", Boolean.TRUE,
"PopupMenu.consumeEventOnClose", Boolean.FALSE,
Copy link
Member

@mrserb mrserb Dec 18, 2020

Choose a reason for hiding this comment

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

This property was added to support some kind of "native" behavior.
The code from the BasicPopupMenuUI.java:

                    // Ask UIManager about should we consume event that closes
                    // popup. This made to match native apps behaviour.
                    boolean consumeEvent =
                        UIManager.getBoolean("PopupMenu.consumeEventOnClose");
                    // Consume the event so that normal processing stops.
                    if(consumeEvent && !(src instanceof MenuElement)) {
                        me.consume();
                    }

So after this fix, the mouse event that causes the popup to close will be not be dispatched to the next component?

Copy link
Member Author

@trebari trebari Dec 18, 2020

Choose a reason for hiding this comment

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

Before the fix the mouse event that cause the popup to close was consumed here as the "PopupMenu.consumeEventOnClose" was true.

After the fix the mouse event that cause the popup to close will not be consumed here as "PopupMenu.consumeEventOnClose" is set to false in the fix for Windows, GTK, Nimbus and Motif LAF.

Copy link
Member

@mrserb mrserb Dec 18, 2020

Choose a reason for hiding this comment

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

And does it match the native behavior? I mean different values of "consumeEventOnClose" weren't a bug. It was intentionally set to the appropriate value.

Copy link
Member Author

@trebari trebari Dec 21, 2020

Choose a reason for hiding this comment

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

yeah , i have checked in windows and ubuntu native apps and it is matching native behaviour.
The issue is seen in Motif, Nimbus, GTK and windows which sets consumeEventOnClose to true.
It is not seen in Metal And Aqua which doesn't set this variable and the value from BasicLookAndFeel.java is used which is false.

Copy link
Member

@mrserb mrserb Dec 23, 2020

Choose a reason for hiding this comment

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

Can you recheck that using the next usecase:

  • Create menu in the frame
  • Add jbutton to the frame exactly in the place where the menu popup is opened
  • Open menu and click some menu item, will the jbutton be clicked?

Copy link
Member Author

@trebari trebari Jan 6, 2021

Choose a reason for hiding this comment

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

Open menu and click some menu item, will the jbutton be clicked?
No, the JButton is not clicked in this use case.

mrserb
mrserb approved these changes Jan 10, 2021
Copy link
Member

@mrserb mrserb left a comment

OK, let's see how it will work.

@trebari
Copy link
Member Author

trebari commented Jan 10, 2021

/integrate

@openjdk openjdk bot closed this Jan 10, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jan 10, 2021
@openjdk
Copy link

openjdk bot commented Jan 10, 2021

@trebari Since your change was applied there have been 1468 commits pushed to the master branch:

  • 81db63e: 8259517: Incorrect test path in test cases
  • 270014a: 8234131: Miscellaneous changes imported from jsr166 CVS 2021-01
  • 63e3bd7: 8246677: LinkedTransferQueue and SynchronousQueue synchronization updates
  • 5cfa8c9: 8246585: ForkJoin updates
  • 6472104: 6278172: TextComponent.getSelectedText() throws StringIndexOutOfBoundsException
  • a653928: 8259512: Update --release 16 symbol information for JDK 16 build 31
  • 7e6677b: 8259446: runtime/jni/checked/TestCheckedReleaseArrayElements.java fails with stderr not empty
  • 628c546: 8258796: [test] Apply HexFormat to tests for java.security
  • 876c7fb: 8259493: [test] Use HexFormat instead of adhoc hex utilities in network code and locale SoftKeys
  • 090bd3a: 8259397: ThreadsSMRSupport::print_info_on() should use try_lock_without_rank_check()
  • ... and 1458 more: https://git.openjdk.java.net/jdk/compare/04775f11fe637029b6040b494bb495939c6d3fcf...master

Your commit was automatically rebased without conflicts.

Pushed as commit 65ca5c6.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated swing
5 participants