<Swing Dev> [9] Review request for 8033699: Incorrect radio button behavior
Vivi An
vivi.an at oracle.com
Wed Oct 8 05:45:20 UTC 2014
Hello Alexander,
Latest patched is ready for review, test case is updated too:
http://cr.openjdk.java.net/~van/8033699/webrev.07/
Thanks
Vivi
On 10/2/2014 2:46 PM, Vivi An wrote:
> 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/20141007/abaa32bb/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/20141007/abaa32bb/attachment.png>
More information about the swing-dev
mailing list