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 >
