Skip to content
This repository has been archived by the owner. It is now read-only.

8259237: Demo selection changes with left/right arrow key. No need to press space for selection. #99

Closed
wants to merge 6 commits into from

Conversation

pankaj-bansal
Copy link

@pankaj-bansal pankaj-bansal commented Jan 9, 2021

Please review a fix for jdk16

Issue: In SwingSet2, if the user navigates through the demos, the demo gets selected/starts on pressing left/right key only. There is no need to press the "space" key. Earlier, on pressing the left/right key, only demo icon used to get focused and user needed to press the "space" to actually select a demo.

Cause: The SwingSet2 has JToggleButtons added to a ButtonGroup. Each JToggleButton has an AbstractAction set on it, which loads the demo. Earlier, when the user pressed Left/Right button, only the selected button used to change. The Action set on JToggleButton used to perform only on pressing the "Space" button. Now, the Action is performed on navigating the JToggleButtons using Left/Right keys without the need to press the "space" key.

This issue is a side effect of fix done for https://bugs.openjdk.java.net/browse/JDK-8249548. The issues fixed in JDK-8249548 were not present in JRadioButton as there was code available to handle it in AquaButtonRadioUI and BasicRadioButtonUI. The fix done for JDK-8249548 moved duplicate code from AquaButtonRadioUI and BasicRadioButtonUI and moved it to BasicButtonUI, so this code is available for JToggleButton and JRadioButton for all L&Fs. This was wrong as there is a difference in behaviour for JRadioButtons added to ButtonGroup for AquaL&F and other L&Fs. In AquaL&F, the AbstractAction set on JRadioButton is not performed on button selection and user has to press "space". In other L&Fs, the AbstractAction is performed on selection of Button itself without the need to press "space".

Fix: The current change fixes the issue in present bug and keeps the fixes done in JDK-8249548. Following is the behaviour of JToggleButton and JRadioButton for different L&Fs before JDK-8249548 fix, after JDK-8249548 fix and after current fix.

Before fix for JDK-8249548
JToggleButton:
For all L&Fs, user can not navigate/select the buttons added to ButtonGroup properly as mentioned in the JDK-8249548.
JRadioButton:
For Synth L&F (Nimbus L&F), user can not navigate/select the buttons added to ButtonGroup.
For AquaL&F, user can select the buttons added to ButtonGroup by pressing left/right key and needs to press the "space" to perform the set Action.
For Other L&Fs, user can select/navigate the buttons and the set Action is also performed without pressing "space"

After fix for JDK-8249548
JToggleButton:
For all L&Fs, user can navigate/select the buttons added to ButtonGroups and the AbstractAction set is performed without the need to press "space".
JRadioButton:
For all L&Fs, user can navigate/select the buttons added to ButtonGroups and the AbstractAction set is performed without the need to press "space".

After current fix:
JToggleButton:
For all L&Fs, user can navigate/select the buttons added to ButtonGroups. User needs to press "space" to perform the set AbstractAction.
JRadioButton:
For AquaL&F, the behaviour before JDK-8249548 is restored, so user needs to press the "space" to perform the set Action.
For all other L&Fs (including Synth L&F), the behaviour is same. User can navigate/select the buttons added to ButtonGroups and set Action is performed without pressing "space"

I have run mach5 jobs with full jtreg/jck and all looks good. Links in JBS. The test TestButtonGroupFocusTraversal.java is modified such that it fails without current fix and passes after the fix. The fix also should be verified by running SwingSet2.


Progress

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

Issue

  • JDK-8259237: Demo selection changes with left/right arrow key. No need to press space for selection.

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk16 pull/99/head:pull/99
$ git checkout pull/99

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 9, 2021

👋 Welcome back pbansal! 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 openjdk bot added the rfr label Jan 9, 2021
@openjdk
Copy link

openjdk bot commented Jan 9, 2021

@pankaj-bansal The following labels will be automatically applied to this pull request:

  • 2d
  • awt
  • swing

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

@openjdk openjdk bot added 2d swing awt labels Jan 9, 2021
@mlbridge
Copy link

mlbridge bot commented Jan 9, 2021

Webrevs

Copy link
Contributor

@prsadhuk prsadhuk left a comment

I guess you did not run jtreg job with latest problemlist as it did not run
javax/swing/JRadioButton/ButtonGroupFocus/ButtonGroupFocusTest.java
javax/swing/JRadioButton/8075609/bug8075609.java
javax/swing/JRadioButton/8033699/bug8033699.java
Please see this test pass with your change as some of them used to fail after JDK-8226892 which was reworked by fix of JDK-8249548

final AquaButtonListener listener = (AquaButtonListener)b.getClientProperty(this);
b.putClientProperty(this, null);
if (listener != null) {
b.removeMouseListener(listener);
b.removeMouseListener(listener);
Copy link
Contributor

@prsadhuk prsadhuk Jan 11, 2021

Choose a reason for hiding this comment

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

Why are we removing same listener twice?

Copy link
Author

@pankaj-bansal pankaj-bansal Jan 11, 2021

Choose a reason for hiding this comment

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

I just rolled back the changes I did in this file in JDK-8249548. Looks like this was has been there for some time. See the file in commit made in this file before JDK-8249548 https://github.com/openjdk/jdk/blob/c18080fef714949221c8ddabc4e23d409575c3d3/src/java.desktop/macosx/classes/com/apple/laf/AquaButtonUI.java.
But yes, it should not be done and I will remove it and update the PR.

@azuev-java
Copy link
Member

azuev-java commented Jan 11, 2021

Quick question: why we special casing JRadioButton case? Is there any reason we should call the action on them?

@pankaj-bansal
Copy link
Author

pankaj-bansal commented Jan 11, 2021

Quick question: why we special casing JRadioButton case? Is there any reason we should call the action on them?

This was the behaviour which was there before JDK-8249548 for JRadioButton and I am just reverting to it. As explained in the PR description, the behaviour before JDK-8249548 was as below

Before fix for JDK-8249548
JToggleButton:
For all L&Fs, user can not navigate/select the buttons added to ButtonGroup properly as mentioned in the JDK-8249548.
JRadioButton:
For Synth L&F (Nimbus L&F), user can not navigate/select the buttons added to ButtonGroup.
For AquaL&F, user can select the buttons added to ButtonGroup by pressing left/right key and needs to press the "space" to perform the set Action.
For Other L&Fs, user can select/navigate the buttons and the set Action is also performed without pressing "space"

@openjdk openjdk bot removed 2d awt labels Jan 11, 2021
@pankaj-bansal
Copy link
Author

pankaj-bansal commented Jan 11, 2021

I have removed the duplicate code for AquaL&F. I think during the fix made for JDK-8226892, it was a mistake to not change the AquaL&F code. So, the behaviour of JRadioButton should be similar across L&Fs and the duplicate code in AquaL&F is not needed. A mach5 job with all jtreg/jck tests is being run and I will will update when it finishes.

Copy link
Member

@azuev-java azuev-java left a comment

Looks ok now.

@openjdk
Copy link

openjdk bot commented Jan 11, 2021

@pankaj-bansal 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:

8259237: Demo selection changes with left/right arrow key. No need to press space for selection.

Reviewed-by: psadhukhan, kizune, serb

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

  • a7e5da2: 8258384: AArch64: SVE verify_ptrue fails on some tests
  • 2cb271e: 8253996: Javac error on jdk16 build 18: invalid flag: -Xdoclint:-missing
  • d60a937: 8259028: ClassCastException when using custom filesystem with wrapper FileChannel impl
  • e05f36f: 8259043: More Zero architectures need linkage with libatomic
  • 020ec84: 8259429: Update reference to README.md
  • fb68395: 8259014: (so) ServerSocketChannel.bind(UnixDomainSocketAddress)/SocketChannel.bind(UnixDomainSocketAddress) will have unknown user and group owner (win)

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 label Jan 11, 2021
@pankaj-bansal
Copy link
Author

pankaj-bansal commented Jan 11, 2021

I guess you did not run jtreg job with latest problemlist as it did not run
javax/swing/JRadioButton/ButtonGroupFocus/ButtonGroupFocusTest.java
javax/swing/JRadioButton/8075609/bug8075609.java
javax/swing/JRadioButton/8033699/bug8033699.java
Please see this test pass with your change as some of them used to fail after JDK-8226892 which was reworked by fix of JDK-8249548

I have updated the PR with some commits and ran the full jtreg/jck tests including the tests you have mentioned. The fix is not breaking anything.

@@ -30,6 +30,7 @@
* @run main TestButtonGroupFocusTraversal
*/

Copy link
Contributor

@prsadhuk prsadhuk Jan 12, 2021

Choose a reason for hiding this comment

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

We need to append this bugid as product code is changed for which this testcase is modified

Copy link
Author

@pankaj-bansal pankaj-bansal Jan 12, 2021

Choose a reason for hiding this comment

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

Done, please have a look.

@mrserb
Copy link
Member

mrserb commented Jan 12, 2021

The spec of the update method seems outdated:
* Find the new toggle button that focus needs to be
* moved to in the group, select the button

BTW I am not sure why the Aqua is so specific and requires an additional press while the same radio buttons do not require that. Is it possible that some fix was implemented in the shared code so now all L&F skip the "space"?

@pankaj-bansal
Copy link
Author

pankaj-bansal commented Jan 12, 2021

The spec of the update method seems outdated:

  • Find the new toggle button that focus needs to be
  • moved to in the group, select the button

BTW I am not sure why the Aqua is so specific and requires an additional press while the same radio buttons do not require that. Is it possible that some fix was implemented in the shared code so now all L&F skip the "space"?

The AquaL&F also does not need the additional "space" press for JRadioButton. During the fix of JDK-8226892, code was added for other L&F, so that the additional "space" press is not needed. But same code was not added for AquaL&F. So, the additional key press was needed. Now, the additional press should not be needed for AquaL&F also for JRadioButton. So now, for all L&Fs, the additional key press is needed for JToggleButton and is not needed for JRadioButton.

I did not add radio button to spec as RadioButton are also ToggleButton only. But yes, I will highlight that. So I will update the spec as below. Hope that is ok.
* Find the new toggle/radio button that focus needs to be
* moved to in the group, select the button

@mrserb
Copy link
Member

mrserb commented Jan 12, 2021

I did not add radio button to spec as RadioButton are also ToggleButton only. But yes, I will highlight that. So I will update the spec as below. Hope that is ok.

  • Find the new toggle/radio button that focus needs to be
  • moved to in the group, select the button

Also please add a comment of why we need to use the setPressed/setArmed it seems unrelated to the selection of the button, and is not described anywhere.

@pankaj-bansal
Copy link
Author

pankaj-bansal commented Jan 12, 2021

I did not add radio button to spec as RadioButton are also ToggleButton only. But yes, I will highlight that. So I will update the spec as below. Hope that is ok.

  • Find the new toggle/radio button that focus needs to be
  • moved to in the group, select the button

Also please add a comment of why we need to use the setPressed/setArmed it seems unrelated to the selection of the button, and is not described anywhere.

Done, please have a look.

@@ -76,6 +79,20 @@ public void run() {
radioButton1 = new JRadioButton("1");
radioButton2 = new JRadioButton("2");
Copy link
Member

@mrserb mrserb Jan 12, 2021

Choose a reason for hiding this comment

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

Just to be safe please check the JCheckBox, it also extends the JToggleButton and we should not break it.

@mrserb
Copy link
Member

mrserb commented Jan 12, 2021

Just small summary info for the future: we have a JToggleButton class and two subclasses: JCheckBox and JRadioButton.

  1. It was decided(but not strictly specified) to generate an action when the selection is changed by the arrow keys for JRadioButton. It is just common sense.
  2. It was decided to not generate an action when the selection is changed by arrow keys for JCheckBox. Since the user should have the ability to move over different checkboxes without toggling them.
  3. The parent class JToggleButton is a root cause of the current bug under discussion. We could implement it in any way, we can generate an action when the user presses the arrow key, but the opposite just a safer solution now.

mrserb
mrserb approved these changes Jan 12, 2021
@pankaj-bansal
Copy link
Author

pankaj-bansal commented Jan 12, 2021

/integrate

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

openjdk bot commented Jan 12, 2021

@pankaj-bansal Since your change was applied there have been 6 commits pushed to the master branch:

  • a7e5da2: 8258384: AArch64: SVE verify_ptrue fails on some tests
  • 2cb271e: 8253996: Javac error on jdk16 build 18: invalid flag: -Xdoclint:-missing
  • d60a937: 8259028: ClassCastException when using custom filesystem with wrapper FileChannel impl
  • e05f36f: 8259043: More Zero architectures need linkage with libatomic
  • 020ec84: 8259429: Update reference to README.md
  • fb68395: 8259014: (so) ServerSocketChannel.bind(UnixDomainSocketAddress)/SocketChannel.bind(UnixDomainSocketAddress) will have unknown user and group owner (win)

Your commit was automatically rebased without conflicts.

Pushed as commit 28ff2de.

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

@pankaj-bansal pankaj-bansal deleted the JDK-8259237 branch Jan 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
integrated swing
4 participants