<Swing Dev> [9] Review request for 8033699: Incorrect radio button behavior
Vivi An
vivi.an at oracle.com
Thu Oct 2 21:46:10 UTC 2014
Thanks Alexander. Will look into it.
Regards,
~ Vivi
On 10/2/2014 12:20 AM, Alexander Zvegintsev wrote:
> Sorry, I wasn't clear,
> provided code with the "MIDDLE" button is just an example what user
> can write (pretty odd one btw)
> it is not in your tests.
>
> By lost functionality I mean that after this fix this user can't
> navigate to the "MIDDLE" button via keyboard
> (or to the btnEnd after ButtonGroup reordering).
>
> --
> Thanks,
> Alexander.
> 02.10.2014 0:40, Vivi An wrote:
>> Thanks Alexander.
>>
>> I have some questions, please see comments inline.
>>
>> Regards,
>>
>> ~ Vivi
>>
>> On 10/1/2014 5:47 AM, Alexander Zvegintsev wrote:
>>> Hello Vivi,
>>>
>>> the fix looks good to me in general, but we lost some functionality,
>> By functionality, you mean the functionality of the test, am I right?
>>
>>> here is some modified code from the previous version of the test:
>>>
>>> ButtonGroup btnGrp = new ButtonGroup();
>>> btnGrp.add(radioBtn1);
>>> btnGrp.add(radioBtn2);
>>> btnGrp.add(radioBtn3);
>>> radioBtn1.setSelected(true);
>>>
>>> mainFrame.getContentPane().add(btnStart);
>>> mainFrame.getContentPane().add(radioBtn1);
>>> mainFrame.getContentPane().add(radioBtn2);
>>> mainFrame.getContentPane().add(new JButton("MIDDLE"));
>>> mainFrame.getContentPane().add(radioBtn3);
>>> mainFrame.getContentPane().add(btnEnd);
>> Just double checked the test in previous version and current version,
>> did not find a button "MIDDLE" like previous code snippet, or I
>> misunderstood, this is pseudo code?
>> Compare to the old test code, new changes made are:
>> 1) Radio button group( radioBtn1, radioBtn2 and radioBtn1 ) added to
>> a panel ("box" in below code snippet) and then to main frame, border
>> also added too to separate from new added non grouped single radio button
>> 2) A single radio button (Not Grouped) is added to cover the bug
>> found in last round
>> 3) Tests 1- 6 are adjusted for preceding change to cover areas would
>> like to test
>>
>> Below is snippet from latest version, the "box" is the panel which
>> contains radio button group, a screenshot added too
>> 110 mainFrame.getContentPane().add(btnStart);
>> 111 mainFrame.getContentPane().add(box);
>> 112 mainFrame.getContentPane().add(radioBtnSingle);
>> 113 mainFrame.getContentPane().add(btnEnd);
>>
>>
>>
>>>
>>> After this fix we are not able to select "MIDDLE" button with a
>>> keyboard anymore.
>>> If we change ButtonGroup order to
>>>
>>> btnGrp.add(radioBtn3);
>>> btnGrp.add(radioBtn2);
>>>
>>> We get into a loop and we are unable to select btnEnd and all
>>> following components(Shift+Tab can help in that case).
>>> I think that these cases deserves some attention.
>>>
>>> --
>>> Thanks,
>>> Alexander.
>>>
>>> On 10/01/2014 01:15 AM, Vivi An wrote:
>>>> Hello Alexander,
>>>>
>>>> Thanks for the review.
>>>>
>>>> New patch is available under
>>>> http://cr.openjdk.java.net/~van/8033699/webrev.06/
>>>>
>>>> Regards,
>>>>
>>>> Vivi
>>>>
>>>> On 9/19/2014 3:22 AM, Alexander Zvegintsev wrote:
>>>>> Hello Vivi,
>>>>>
>>>>> Since you are touching this code, could you please add missing
>>>>> @Override annotations?
>>>>>
>>>> Added
>>>>> void jumpToNextComponent(boolean next) {
>>>>> if (!getButtonGroupInfo())
>>>>> return;
>>>>>
>>>>> This leads to an issue: we can't switch to a next component with a
>>>>> TAB key if JRadioButton
>>>>> is not in a ButtonGroup. (It may be a reason to write another test.)
>>>> Fixed
>>>>>
>>>>> private boolean isValidRadioButtonObj(Object obj) {
>>>>> return ((obj != null) && (obj instanceof JRadioButton) &&
>>>>> ((JRadioButton) obj).isVisible() &&
>>>>> ((JRadioButton) obj).isEnabled());
>>>>> }
>>>> Fixed
>>>>>
>>>>> There is no need to do a null check before instanceof [1]
>>>>>
>>>>> * @param evt, the event object.
>>>>> * @param next, indicate if it's next one
>>>>> */
>>>>> private void selectRadioButton(ActionEvent event, boolean next) {
>>>>>
>>>>> typo: evt should be event
>>>>>
>>>> Fixed
>>>>>
>>>>> I run the test and it fails at least on Linux. It happens due to a
>>>>> big auto delay:
>>>>> Robot holds key for too long so system sends TAB key multiple time.
>>>>> So I think auto delay should be about 100 ms.
>>>>>
>>>>> robot.setAutoDelay(300);
>>>>>
>>>>> It is enough to call it once after Robot creation, this will
>>>>> automatically add a 300 ms delay
>>>>> between Robot generated events (such as keyPress, mouseMove, etc).
>>>>> If you really want a delay between test runs please use
>>>>> Thread.sleep().
>>>>>
>>>>> I see many calls to hitKey() and then to realSync(), so we can
>>>>> move realSync() call to the end of hitKey().
>>>>> checkResult() may be combined with runTest(), hence we call one
>>>>> function instead of two.
>>>>>
>>>>> int sum = 0;
>>>>> for (int i = 0; i < testResults.length; i++) {
>>>>> if (testResults[i] == false)
>>>>> sum++;
>>>>> }
>>>>>
>>>>> if (sum != 0){
>>>>> System.out.println("Total 5 tests, failed test(s): " +
>>>>> sum);
>>>>> throw new RuntimeException("Test failed.");
>>>>> }
>>>>>
>>>>>
>>>>> I don't think that this logic is applicable here, each test
>>>>> depends on previous: Test2 failure means that
>>>>> focus is on a wrong component, so all following tests will fail.
>>>>> BTW, this code is unreachable in case of test failure, since
>>>>> checkResult() already throws an exception.
>>>>>
>>>>> http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.20.2
>>>>>
>>>> Test is updated, non-grouped radio button test is added, test
>>>> passed on Oracle Linux 5 u6 32 bits and Windows 7.
>>>>>
>>>>> --
>>>>> Thanks,
>>>>> Alexander.
>>>>>
>>>>> On 09/18/2014 09:23 PM, Vivi An wrote:
>>>>>> Thank you Alexandr.
>>>>>>
>>>>>> Need one more review for this. Alexander, could you please take a
>>>>>> look at below fix?
>>>>>> http://cr.openjdk.java.net/~van/8033699/webrev.05/
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Vivi
>>>>>>
>>>>>> On 9/18/2014 12:55 AM, Alexander Scherbatiy wrote:
>>>>>>>
>>>>>>> The fix looks good to me.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Alexandr.
>>>>>>>
>>>>>>> On 9/18/2014 3:09 AM, Vivi An wrote:
>>>>>>>> Hello Alex,
>>>>>>>>
>>>>>>>> On 9/17/2014 12:44 AM, Alexander Scherbatiy wrote:
>>>>>>>>> On 9/17/2014 9:17 AM, Vivi An wrote:
>>>>>>>>>> You are right. Fixed now.
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~van/8033699/webrev.04/
>>>>>>>>>
>>>>>>>>> 528 if (e.isShiftDown()) {
>>>>>>>>> 529 if (e.getKeyCode() == KeyEvent.VK_TAB) {
>>>>>>>>> 537 // ...
>>>>>>>>> btnGroupInfo.jumpToNextComponent(false);
>>>>>>>>> 539 }
>>>>>>>>> 540 } else if (e.getKeyCode() == KeyEvent.VK_TAB) {
>>>>>>>>> 548 // ...
>>>>>>>>> btnGroupInfo.jumpToNextComponent(true);
>>>>>>>>> 550 }
>>>>>>>>>
>>>>>>>>> The code in the if/else branches is almost the same except
>>>>>>>>> the true/false argument.
>>>>>>>>> I would suggest to unify them in the following way:
>>>>>>>>>
>>>>>>>>> if (e.getKeyCode() == KeyEvent.VK_TAB) {
>>>>>>>>> // ... jumpToNextComponent(!e.isShiftDown())
>>>>>>>>> }
>>>>>>>>>
>>>>>>>> Fixed.
>>>>>>>>>
>>>>>>>>> 510 if (next)
>>>>>>>>> 511 KeyboardFocusManager.
>>>>>>>>> 512
>>>>>>>>> getCurrentKeyboardFocusManager().focusNextComponent((JComponent)lastBtn);
>>>>>>>>> 513 else
>>>>>>>>> 514 KeyboardFocusManager.
>>>>>>>>> 515
>>>>>>>>> getCurrentKeyboardFocusManager().focusPreviousComponent((JComponent)firstBtn);
>>>>>>>>>
>>>>>>>>> This code can be also a little bit optimized:
>>>>>>>>>
>>>>>>>>> KeyboardFocusManager.getCurrentKeyboardFocusManager().
>>>>>>>>> focusPreviousComponent((JComponent) (next ? lastBtn :
>>>>>>>>> firstBtn));
>>>>>>>>>
>>>>>>>> It's different function call, one for focusNextComponent and
>>>>>>>> the other for focusPreviousComponent, not able to optimize in
>>>>>>>> suggested way.
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~van/8033699/webrev.05/
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>>
>>>>>>>> Vivi
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Alexandr.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thank you
>>>>>>>>>>
>>>>>>>>>> ~ Vivi
>>>>>>>>>>
>>>>>>>>>> On 9/16/2014 12:39 AM, Alexander Scherbatiy wrote:
>>>>>>>>>>>
>>>>>>>>>>> I have missed one more case:
>>>>>>>>>>> 527 public void keyPressed(KeyEvent e) {
>>>>>>>>>>> 528 if (e.getKeyCode() == KeyEvent.VK_TAB) {
>>>>>>>>>>> // jumpToNextComponent(true)
>>>>>>>>>>> 538 }
>>>>>>>>>>> 539
>>>>>>>>>>> 540 if (e.getKeyCode() == KeyEvent.VK_TAB &&
>>>>>>>>>>> e.isShiftDown()) {
>>>>>>>>>>> //jumpToNextComponent(false)
>>>>>>>>>>> 550 }
>>>>>>>>>>> 551 }
>>>>>>>>>>>
>>>>>>>>>>> It seems that if e.isShiftDown() is true then both
>>>>>>>>>>> jumpToNextComponent(true) and jumpToNextComponent(false)
>>>>>>>>>>> methods can be called.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Alexandr.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 9/16/2014 7:38 AM, Vivi An wrote:
>>>>>>>>>>>> Fixed. New Webrev:
>>>>>>>>>>>> http://cr.openjdk.java.net/~van/8033699/webrev.03/
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks Alex.
>>>>>>>>>>>>
>>>>>>>>>>>> Regards,
>>>>>>>>>>>>
>>>>>>>>>>>> ~ Vivi
>>>>>>>>>>>>
>>>>>>>>>>>> On 9/15/2014 2:59 AM, Alexander Scherbatiy wrote:
>>>>>>>>>>>>> On 9/12/2014 8:56 PM, Vivi An wrote:
>>>>>>>>>>>>>> Thanks Alexander.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> All items addressed except last one, please see comments
>>>>>>>>>>>>>> inline.
>>>>>>>>>>>>>> New Webrev:
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~van/8033699/webrev.02/
>>>>>>>>>>>>>
>>>>>>>>>>>>> 163 if ( keyListener != null ) {
>>>>>>>>>>>>>
>>>>>>>>>>>>> Could you remove spaces in the brackets? After code
>>>>>>>>>>>>> formatting it should be "if (keyListener != null) {"
>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 543 if (next && btnGroupInfo.lastBtn != null)
>>>>>>>>>>>>>>> 544 KeyboardFocusManager.
>>>>>>>>>>>>>>> 545
>>>>>>>>>>>>>>> getCurrentKeyboardFocusManager().focusNextComponent((JComponent)btnGroupInfo.lastBtn);
>>>>>>>>>>>>>>> 546 else if (btnGroupInfo.firstBtn != null)
>>>>>>>>>>>>>>> 547 KeyboardFocusManager.
>>>>>>>>>>>>>>> 548
>>>>>>>>>>>>>>> getCurrentKeyboardFocusManager().focusPreviousComponent((JComponent)btnGroupInfo.firstBtn);
>>>>>>>>>>>>>>> 549 }
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Should the first button be focused If next is true and
>>>>>>>>>>>>>>> last button is null?
>>>>>>>>>>>>>> Don't think last button could be null when this function
>>>>>>>>>>>>>> is triggered, last and first will always be set in case
>>>>>>>>>>>>>> there is at least one valid (enabled and visible) radio
>>>>>>>>>>>>>> button in the group.
>>>>>>>>>>>>>
>>>>>>>>>>>>> It seems that lastBtn != null and firstBtn != null
>>>>>>>>>>>>> checks are not necessary in this case.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> ~ Vivi
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 9/8/2014 5:43 AM, Alexander Scherbatiy wrote:
>>>>>>>>>>>>>>>>> On 9/4/2014 10:56 PM, Vivi An wrote:
>>>>>>>>>>>>>>>>>> Hello,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Please review fix for JDK-8033699. Changes made:
>>>>>>>>>>>>>>>>>> 1) When tab or shift-tab key pressed, focus moved to
>>>>>>>>>>>>>>>>>> next/previous
>>>>>>>>>>>>>>>>>> component outside of the radio button group
>>>>>>>>>>>>>>>>>> 2) Using up/down or left/right arrow key to change
>>>>>>>>>>>>>>>>>> selection inside
>>>>>>>>>>>>>>>>>> radio button group
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Bug:https://bugs.openjdk.java.net/browse/JDK-8033699
>>>>>>>>>>>>>>>>>> Webrev:http://cr.openjdk.java.net/~van/8033699/webrev.00/
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> 56 private KeyListener keyListener = null;
>>>>>>>>>>>>>>>>> 57 private KeyHandler handler = null;
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> It seems that these both fields point to the same
>>>>>>>>>>>>>>>>> object. Is it possible to get rid of one of them?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> 152 if ( keyListener != null ) {
>>>>>>>>>>>>>>>>> 153 button.removeKeyListener( keyListener );
>>>>>>>>>>>>>>>>> 154 }
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Some UIs also set the listener to null in the
>>>>>>>>>>>>>>>>> uninstallUI()/uninstallListeners() methods (like
>>>>>>>>>>>>>>>>> BasicComboBoxUI or BasicColorChooserUI).
>>>>>>>>>>>>>>>>> Should the keyListener also be set to null to free the
>>>>>>>>>>>>>>>>> KeyHandler object?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> 369 if (!(eventSrc instanceof JRadioButton &&
>>>>>>>>>>>>>>>>> 370 ((JRadioButton) eventSrc).isVisible()
>>>>>>>>>>>>>>>>> && ((JRadioButton) eventSrc).isEnabled()))
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> This check is used several times. It can be moved to a
>>>>>>>>>>>>>>>>> separate method.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> 373 // Get the button model from the source.
>>>>>>>>>>>>>>>>> 374 ButtonModel model = ((AbstractButton)
>>>>>>>>>>>>>>>>> eventSrc).getModel();
>>>>>>>>>>>>>>>>> 375 if (!(model instanceof DefaultButtonModel))
>>>>>>>>>>>>>>>>> 376 return;
>>>>>>>>>>>>>>>>> 377
>>>>>>>>>>>>>>>>> 378 // If the button model is
>>>>>>>>>>>>>>>>> DefaultButtonModel, and use it, otherwise return.
>>>>>>>>>>>>>>>>> 379 DefaultButtonModel bm =
>>>>>>>>>>>>>>>>> (DefaultButtonModel) model;
>>>>>>>>>>>>>>>>> 380 if (bm == null)
>>>>>>>>>>>>>>>>> 381 return;
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> The second check is not necessary because (model
>>>>>>>>>>>>>>>>> instanceof DefaultButtonModel) returns false for null
>>>>>>>>>>>>>>>>> model.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> 404 AbstractButton curElement = null;
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> The curElement variable declaration could be moved
>>>>>>>>>>>>>>>>> into the 'while' block.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> 408 if (curElement instanceof
>>>>>>>>>>>>>>>>> JRadioButton &&
>>>>>>>>>>>>>>>>> 409 ((JRadioButton) curElement).isVisible() &&
>>>>>>>>>>>>>>>>> 410 ((JRadioButton) curElement).isEnabled()){
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> It is possible to use 'continue' here to not put the
>>>>>>>>>>>>>>>>> other code inside the 'if' block.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> 418 else if (!srcFound){
>>>>>>>>>>>>>>>>> 422 } else if (srcFound && nextBtn ==
>>>>>>>>>>>>>>>>> null){
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> It is not necessary to check the srcFound in the
>>>>>>>>>>>>>>>>> second 'if' because it should already have true value.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> 444 if (newSelectedBtn != null){
>>>>>>>>>>>>>>>>> 445 newSelectedBtn.requestFocusInWindow();
>>>>>>>>>>>>>>>>> 446 newSelectedBtn.setSelected(true);
>>>>>>>>>>>>>>>>> 447 }
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Is it possible here that newSelectedBtn == eventSrc?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> 522 private void
>>>>>>>>>>>>>>>>> jumpToNextComponent(JRadioButton btn, boolean next){
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> The code that retrieves elements from a button group
>>>>>>>>>>>>>>>>> is also used in the selectRadioButton()
>>>>>>>>>>>>>>>>> and can be moved to a separate method.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> JPRT build succeeded, JCK tests for JRadiobutton and
>>>>>>>>>>>>>>>>>> JCheckBox all passed.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Thank you
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Vivi
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20141002/e6177f8d/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: image/png
Size: 32466 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20141002/e6177f8d/attachment.png>
More information about the swing-dev
mailing list