<Swing Dev> Request for review, JDK-8009883, REGRESSION: test/closed/javax/swing/AbstractButton/4246045/bug4246045.java fails
Pete Brunet
peter.brunet at oracle.com
Thu May 22 17:59:37 UTC 2014
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/
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