<Swing Dev> Request for review, JDK-8009883, REGRESSION: test/closed/javax/swing/AbstractButton/4246045/bug4246045.java fails
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Fri May 23 10:34:32 UTC 2014
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