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

[Pete Brunet]

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/

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