Hi Alexandr, Thanks very much for the review. I updated the webrev. See http://cr.openjdk.java.net/~ptbrunet/JDK-8145207/webrev.01/
>From your separate email you suggested the following in order to provide the AccessibleAction interface without changing the public API of JList.AccessibleJList.AccessibleJListChild: >Just create a private subclass of the AccessibleJListChild which implements AccessibleAction and return it in all places where new instance of the AccessibleJListChild is returned. I made that change. On 7/4/16 4:14 AM, Alexandr Scherbatiy wrote: > On 6/18/2016 5:31 AM, Pete Brunet wrote: >> Please review the following patch. >> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8145207 >> Patch: http://cr.openjdk.java.net/~ptbrunet/JDK-8145207/webrev.00/ >> >> This fixes the following functionality that was not working with the >> JList of ListDemo of SwingSet2. >> - start VoiceOver >> - start SwingSet2 >> - start the ListDemo >> - press Tab until focus is on the list, should hear VO when changing >> selections with up/down arrow >> - when interacting with list should hear that there are 30 (total) >> items, not 26 (visible) items >> - when using control+option+up/downarrow should be able to move to and >> select (control+option+spacebar) non-visible items past the 26th visible >> item >> - should be able to multi-select both visible and invisible items using >> control+option+command+return and VO should read the item just added >> - should be able to shift extend items with shift up or shift down arrow >> and VO should announce the item just added or removed > > CAccessibility: > 639 childrenAndRoles.clear(); > 640 childrenAndRoles.addAll(newArray); > > - Is it possible just to assign the newArray to the childrenAndRoles? > Is it necessary that the childrenAndRoles has final keyword? I made those changes. > - Please, format the code on lines 630-631 to romevo unnessary spaces > in round brackets. I prefer to keep the spaces in this kind of split line. Over the years I've found that style easier to read. > > CAccessible: > > - static method getActiveDescendant() is not used in the CAccessible > class but only in CAccessibility. Is it possible to move it to the > CAccessibility class? It's there because it accesses the private field, activeDescendant. > - Please, split the long lines. You may use static imports for constants. Done. > > JavaComponentAccessibility: > 716 if (returnValue == -1) { > 717 return NSNotFound; > 718 } else { > 719 return returnValue; > 720 } > > - This can be written shorter: return (returnValue == -1) ? > NSNotFound : returnValue; Done. > > 998 if ([self isSelectable:[ThreadUtilities getJNIEnv]]) { > 999 return YES; > 1000 } else { > 1001 return NO; > 1002 } > > - Is there a macros which can convert jboolean to BOOL? I could not find one. > - Could you also split the modified lines where it is possible? Done. Pete > > Thanks, > Alexandr. > >> >> Pete >> >> >
