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

[Pete Brunet]

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.
>
>   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
>