<Swing Dev> Request for review, JDK-8009883, REGRESSION: test/closed/javax/swing/AbstractButton/4246045/bug4246045.java fails
Pete Brunet
peter.brunet at oracle.com
Fri May 23 15:14:24 UTC 2014
Thanks Alexandr.
So far Alexandr is the only reviewer of this. I'd like one more.
Thanks, Pete
On 5/23/14 5:34 AM, Alexander Scherbatiy wrote:
> On 5/22/2014 9:59 PM, Pete Brunet wrote:
>> I'd like one more reviewer of this fix.
>>
>> Also I removed the @Deprecated and will deal with this in a following
>> JBS issue.
>>
>> http://cr.openjdk.java.net/~ptbrunet/JDK-8009883/webrev.04/
>
> The fix looks good for me.
>
> Thanks,
> Alexandr.
>
>>
>> Pete
>>
>> On 5/21/14 10:08 AM, Pete Brunet wrote:
>>> 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