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
