<Swing Dev> Request for review, JDK-8009883, REGRESSION: test/closed/javax/swing/AbstractButton/4246045/bug4246045.java fails
Pete Brunet
peter.brunet at oracle.com
Wed May 21 15:08:38 UTC 2014
On 5/21/14 7:04 AM, Alexander Scherbatiy wrote:
> 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 very much for catching that Alexandr! The new webrev is here:
http://cr.openjdk.java.net/~ptbrunet/JDK-8009883/webrev.03/
Note that I added a @Deprecated tag (and javadoc tag). Hopefully that's
the correct thing to do.
I'd like to list one more reviewer on the patch so I'd appreciate one
more set of eyes on this.
Pete
>
> 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
>>
>
More information about the swing-dev
mailing list