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

[Alexander Scherbatiy]

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.

   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