Hi Sergey, I already pushed the fix but ... On 4/20/16 10:20 AM, Sergey Bylokhov wrote: > Hi, Pete. > Just a small notes: > - It seems that AXTextChangeNotifier() should be created only if "c > is Accessible". That's true. I added a comment to 8154507/8 and will fix that with one of those patches. > - I am not sure, but when we clear these listeners(which were added > to AccessibilityEventMonitor), or it is not necessary? I didn't see a removal mechanism in the current code so I didn't do that. If VO is not active I assume the allocations won't happen. If someone start/stops VO I agree there would be a unused code in memory, but I suspect there is no means to unallocate/disable all the a11y support. I will check at Apple to see if anyone knows. Other than for testers this would be an unusual scenario. > - Is it possible that "AccessibleContext.ACCESSIBLE_TEXT_PROPERTY" > will be fired by JProgressBar or JSlider, if yes then we will get > double notification(one from AXTextChangeNotifier and one from > AXProgressChangeNotifier). I'll need to look into this more when I work on 8154507/8. I added your comments to those bugs.
Pete > > On 19.04.16 22:16, Phil Race wrote: >> +1 >> >> -phil. >> >> On 04/19/2016 12:05 PM, Pete Brunet wrote: >>> New patch: http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/webrev.03/ >>> >>> Changes: >>> - removed _AccessibleState >>> - removed wildcard imports. >>> >>> Is this ready to push? >>> >>> Pete >>> >>> On 4/19/16 12:26 PM, Phil Race wrote: >>>> oh one nit picky thing - can we get rid of the wild card imports ? >>>> >>>> -phil. >>>> >>>> On 04/19/2016 10:21 AM, Phil Race wrote: >>>>> Which seems to imply the _AccessibleState subclass has been obsolete >>>>> since 1.5 >>>>> Although when these tools were unbundled it perhaps could not assume >>>>> 1.5. >>>>> >>>>> The rest of the changes look fine and it is a lot simpler than the >>>>> v0 .. >>>>> >>>>> -phil. >>>>> >>>>> On 04/19/2016 08:54 AM, Pete Brunet wrote: >>>>>> Thanks for noticing that Alexandr. I see this state was added in >>>>>> 1.5 and apparently the code from which I borrowed this from was >>>>>> never updated. I will remove that and open a bug to remove that >>>>>> from the original. -Pete >>>>>> >>>>>> On 4/19/16 7:39 AM, Alexander Scherbatiy wrote: >>>>>>> On 19/04/16 05:50, Pete Brunet wrote: >>>>>>>> Please review the following patch. >>>>>>>> >>>>>>>> Bug:https://bugs.openjdk.java.net/browse/JDK-8076554 >>>>>>>> Patch:http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/webrev.02/ >>>>>>>> >>>>>>>> The problem is that the code is currently hardcoded to only >>>>>>>> support text >>>>>>>> accessibility for JTextComponent using the DocumentListener, >>>>>>>> CaretListener interfaces. This eliminates the ability to provide >>>>>>>> accessibility for a custom text components. The resolution is >>>>>>>> to remove >>>>>>>> the JTextComponent gate and use the PropertyChangeListener >>>>>>>> listener >>>>>>>> interface with its ACCESSIBLE_CARET_PROPERTY and >>>>>>>> ACCESSIBLE_TEXT_PROPERTY properties. >>>>>>> The AccessibleState class already has the MANAGES_DESCENDANTS >>>>>>> constant. What is purpose to add a constant with the same name to >>>>>>> the _AccessibleState? >>>>>>> >>>>>>> Thanks, >>>>>>> Alexandr. >>>>>>>> I tested this with the two test cases which can be found at >>>>>>>> http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/test/ >>>>>>>> running them side by side with VoiceOver. >>>>>>>> >>>>>>>> Note that I will fix the similar JProgressBar and JSlider >>>>>>>> restrictions >>>>>>>> in JDK-8154507 and JDK-8154508 respectively. >>>>>>>> >>>>>>>> Pete >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> > >
