<Swing Dev> Request for review, JDK-8009883, REGRESSION: test/closed/javax/swing/AbstractButton/4246045/bug4246045.java fails

[Alexander Scherbatiy]

On 5/21/2014 2:56 PM, Alexander Scherbatiy wrote:
> On 5/20/2014 1:10 AM, Pete Brunet wrote:
>> On 5/16/14 11:04 AM, Pete Brunet wrote:
>>> On 5/16/14 10:45 AM, Alexander Scherbatiy wrote:
>>>> On 5/16/2014 7:15 PM, Pete Brunet wrote:
>>>>> On 5/16/14 6:45 AM, Alexander Scherbatiy wrote:
>>>>>>     Hi Peter,
>>>>>>
>>>>>>     Is there any difference between AccessibleAWTFocusHandler and
>>>>>> AccessibleFocusHandler classes?
>>>>> Hi Alexandr, The former is the focus handler used by the inner class
>>>>> accessibility support for an AWT Component and the latter is the 
>>>>> focus
>>>>> handler used by the inner class accessibility support for a 
>>>>> JComponent.
>>>>>
>>>>> The code is the same.
>>>>     Is it possible to remove the AccessibleFocusHandler class at all
>>>> and initialize the accessibleAWTFocusHandler
>>>>     only in the AccessibleAWTFocusHandler.addPropertyChangeListener()
>>>> method?
>>> This looks like it will work and I would like that solution a lot
>>> better.  I'll give it a try and if that works out I'll provide a new
>>> webrev.  Thanks Alexandr.
>> Hi Alexandr and AWT/Swing teams,
>>
>> Please review the new webrev at
>> http://cr.openjdk.java.net/~ptbrunet/JDK-8009883/webrev.01/
>
>    The fix looks good for me.

      Sorry. I just realize that the AccessibleFocusHandler  class has 
protected access and can't be removed because of the compatibility reasons.
http://docs.oracle.com/javase/7/docs/api/javax/swing/JComponent.AccessibleJComponent.AccessibleFocusHandler.html

     All others looks good for me. Just leave the 
AccessibleFocusHandler  class from AccessibleJComponent as is.

   Thanks,
   Alexandr.

>
>   Thanks,
>   Alexandr.
>
>>
>> Pete
>>>>    Thanks,
>>>>    Alexandr.
>>>>
>>>>> The former code is this:
>>>>>
>>>>>           protected class AccessibleAWTFocusHandler implements
>>>>> FocusListener {
>>>>>               public void focusGained(FocusEvent event) {
>>>>>                   if (accessibleContext != null) {
>>>>> accessibleContext.firePropertyChange(
>>>>> AccessibleContext.ACCESSIBLE_STATE_PROPERTY,
>>>>> null,
>>>>> AccessibleState.FOCUSED);
>>>>>                   }
>>>>>               }
>>>>>               public void focusLost(FocusEvent event) {
>>>>>                   if (accessibleContext != null) {
>>>>> accessibleContext.firePropertyChange(
>>>>> AccessibleContext.ACCESSIBLE_STATE_PROPERTY,
>>>>> AccessibleState.FOCUSED, null);
>>>>>                   }
>>>>>               }
>>>>>           }
>>>>>
>>>>> and the latter code is this
>>>>>
>>>>>           protected class AccessibleFocusHandler implements
>>>>> FocusListener {
>>>>>              public void focusGained(FocusEvent event) {
>>>>>                  if (accessibleContext != null) {
>>>>> accessibleContext.firePropertyChange(
>>>>> AccessibleContext.ACCESSIBLE_STATE_PROPERTY,
>>>>>                           null, AccessibleState.FOCUSED);
>>>>>                   }
>>>>>               }
>>>>>               public void focusLost(FocusEvent event) {
>>>>>                   if (accessibleContext != null) {
>>>>> accessibleContext.firePropertyChange(
>>>>> AccessibleContext.ACCESSIBLE_STATE_PROPERTY,
>>>>>                           AccessibleState.FOCUSED, null);
>>>>>                   }
>>>>>               }
>>>>>           }
>>>>>>     Thanks,
>>>>>>     Alexandr.
>>>>>>
>>>>>> On 5/7/2014 5:45 AM, Pete Brunet wrote:
>>>>>>> Hi Swing and AWT teams, (and Artem since you were involved in the
>>>>>>> 7179482 fix),
>>>>>>>
>>>>>>> I'd appreciate your review for a fix to a regression in
>>>>>>> java.awt.Component and javax.swing.JComponent.
>>>>>>>
>>>>>>> Original bug: https://bugs.openjdk.java.net/browse/JDK-7179482
>>>>>>> Orignal CCC: 7179482
>>>>>>> Regression Bug: https://bugs.openjdk.java.net/browse/JDK-8009883
>>>>>>> Regression Fix Webrev:
>>>>>>> http://cr.openjdk.java.net/~ptbrunet/JDK-8009883/webrev.00/
>>>>>>>
>>>>>>> Problem Description:
>>>>>>> jdk/test/closed/javax/swing/AbstractButton/4246045/bug4246045.java
>>>>>>> tests
>>>>>>> to make sure the focus state changes whenever a JButton or
>>>>>>> JToggleButton
>>>>>>> property change listener is called.  The problem in this case is 
>>>>>>> that
>>>>>>> for each change of focus the listener is called twice.  The 
>>>>>>> first time
>>>>>>> there is no problem because for example the component is not
>>>>>>> focused and
>>>>>>> then it is, but the second time the listener is called the
>>>>>>> component is
>>>>>>> already focused and the new state is also focused so the test case
>>>>>>> detects that the focus state didn't change.
>>>>>>>
>>>>>>> Root cause:
>>>>>>> There are two focus gained and two focus lost events fired when
>>>>>>> JComponents gain/loose focus.  Part of the fix for JDK-7179482 
>>>>>>> was to
>>>>>>> deprecate the 
>>>>>>> JComponent.AccessibleJComponent.accessibleFocusHandler
>>>>>>> field noting that
>>>>>>> Component.AccessibleAWTComponent.accessibleAWTFocusHandler 
>>>>>>> should be
>>>>>>> used instead.  The problem is that this was not done when 
>>>>>>> 7179482 was
>>>>>>> fixed.  The end result is that two focus listeners are added when
>>>>>>> JComponent.AccessibleJComponent.addPropertyChangeListener is 
>>>>>>> called.
>>>>>>> The first listener is added in that property change method and the
>>>>>>> second one is added when the superclass method is called, i.e.
>>>>>>> Component.AccessibleAWTComponent.addPropertyChangeListener. The two
>>>>>>> listeners cause two focus events with the second one not causing a
>>>>>>> state
>>>>>>> change.
>>>>>>>
>>>>>>> Proposed fix:
>>>>>>> The proposed fix is for
>>>>>>> JComponent.AccessibleJComponent.addPropertyChangeListener to set 
>>>>>>> its
>>>>>>> listener into
>>>>>>> Component.AccessibleAWTComponent.accessibleAWTFocusHandler
>>>>>>> so when the superclass method,
>>>>>>> Component.AccessibleAWTComponent.addPropertyChangeListener is
>>>>>>> called the
>>>>>>> listener at that level will already be set.
>>>>>>>
>>>>>>> A side effect of the fix is that logic was needed to be added in
>>>>>>> Component.AccessibleAWTComponent.addPropertyChangeListener to deal
>>>>>>> with
>>>>>>> the fact that the
>>>>>>> Component.AccessibleAWTComponent.accessibleAWTFocusHandler field
>>>>>>> can be
>>>>>>> set either from within the object's subclass method
>>>>>>> JComponent.AccessibleJComponent.addPropertyChangeListener or via an
>>>>>>> external call to
>>>>>>> Component.AccessAWTComponent.addPropertyChangeListener
>>>>>>> for cases other than when there is a JComponent subclass of 
>>>>>>> Component.
>>>>>>> In the former case the listener is already set by the subclass so a
>>>>>>> focus listener isn't added.  In the latter case the listener is not
>>>>>>> yet
>>>>>>> added so a focus listener is new'd and added.
>>>>>>>
>>>>>>> There are similar considerations when removing a listener; this 
>>>>>>> can be
>>>>>>> seen in the webrev.
>>>>>>>
>>>>>>> There may be a better way to fix this but the proposed fix does
>>>>>>> resolve
>>>>>>> the issue that the test case exposed.
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Pete
>