To make comboboxes 100% accessible, bug 363697 needs to be fixed, though I will go ahead with this initial implementation even before that action is available.

This patch mainly makes the location bar (and other combo boxes) show up in the hierarchy, as well as its children (e.g., the feed button in the location bar). The combobox itself can't do much yet, but it's ready when that support is implemented.

Comment on attachment 248660 [details] [diff] [review] Patch v1 >+/* A combobox (in the mac world) is a textfield that may pop up a menu. */ nit: Probably textfield and button that may have popup menu? Or how does mac world show popup menu? :) >+@interface mozComboboxAccessible : mozTextAccessible >+// equivalent to pressing return >+- (void)confirm; Just interesting. What does return do? >+ >+// shows the menu for this combobox >+- (void)showMenu; >+@end >\ No newline at end of file nit: please add new line >Index: mozTextAccessible.mm >=================================================================== >RCS file: /cvsroot/mozilla/accessible/src/mac/mozTextAccessible.mm,v >retrieving revision 1.1 >diff -u -8 -p -r1.1 mozTextAccessible.mm >--- mozTextAccessible.mm 15 Nov 2006 11:08:14 -0000 1.1 >+++ mozTextAccessible.mm 14 Dec 2006 15:58:21 -0000 >@@ -176,8 +176,84 @@ extern const NSString *kTopLevelUIElemen > > - (void)valueDidChange > { > NSAccessibilityPostNotification([self hasRepresentedView] ? [self representedView] : self, > NSAccessibilityValueChangedNotification); > } > > @end >+ >+@implementation mozComboboxAccessible >+ >+- (NSArray*)accessibilityAttributeNames >+{ >+ static NSArray *supportedAttributes = nil; >+ if (!supportedAttributes) { >+ // standard attributes that are shared and supported by all generic elements. >+ supportedAttributes = [[NSArray alloc] initWithObjects:NSAccessibilityParentAttribute, // required You didn't declare Help attribute. Why? >+ NSAccessibilitySubroleAttribute, Sorry, please remind me may every accessible role have subrole? If no then what is subrole supposed for combobox? >+- (NSArray *)accessibilityActionNames >+{ >+ if ([self isEnabled]) { >+ return [NSArray arrayWithObjects:NSAccessibilityConfirmAction, >+ NSAccessibilityShowMenuAction, >+ nil]; >+ } >+ return nil; >+} >+- (void)accessibilityPerformAction:(NSString *)action >+{ >+ // both the ShowMenu and Click action do the same thing. >+ if ([self isEnabled]) { I don't remember exactly we talked already about something similar. And if I'm right then one check of isEnabled is redudrant. >+- (void)showMenu >+{ >+ // currently unimplemented. waiting for support in bug 363697 >+} >+ >+- (void)confirm >+{ >+ // currently unimplemented. waiting for support in bug 363697 I don't see any related with this in bug 363697. That's probably because I don't know what should do confirm method :) > // we'll flatten buttons and checkboxes. usually they have a text node > // child, that is their title. Works in conjunction with IsPruned() below. > PRBool IsFlat() { > PRUint32 role = Role(this); > return (role == ROLE_CHECKBUTTON || > role == ROLE_PUSHBUTTON || > role == ROLE_TOGGLE_BUTTON || > role == ROLE_SPLITBUTTON || >- role == ROLE_ENTRY || >- role == ROLE_AUTOCOMPLETE); >+ role == ROLE_ENTRY); > } Why this change? What does exactly IsFlat do and where is it used? >- case ROLE_AUTOCOMPLETE: >- return [mozTextAccessible class]; >+ { >+ // if this textfield has a popup, it's a combobox. >+ if (HasPopup()) >+ return [mozComboboxAccessible class]; Do you have an example where it is valid? > case ROLE_COMBOBOX: > return [mozPopupButtonAccessible class]; Why not mozComboboxAccessible? >- NSAccessibilityTextFieldRole, // ROLE_AUTOCOMPLETE >+ NSAccessibilityComboBoxRole, // ROLE_AUTOCOMPLETE nit: please do right indent Also, I don't see any changes for ROLE_COMBOBOX. Why?

Thanks for the review, Alex. (In reply to comment #4) > (From update of attachment 248660 [details] [diff] [review] [edit]) > > >+/* A combobox (in the mac world) is a textfield that may pop up a menu. */ > > nit: Probably textfield and button that may have popup menu? Or how does mac > world show popup menu? :) You can either press down-arrow or click the button, yeah. > > >+@interface mozComboboxAccessible : mozTextAccessible > >+// equivalent to pressing return > >+- (void)confirm; > > Just interesting. What does return do? Depends on the application. In the location bar in Firefox, it goes to the URL... > > >+ > >+// shows the menu for this combobox > >+- (void)showMenu; > >+@end > >\ No newline at end of file > > nit: please add new line > > >Index: mozTextAccessible.mm > >=================================================================== > >RCS file: /cvsroot/mozilla/accessible/src/mac/mozTextAccessible.mm,v > >retrieving revision 1.1 > >diff -u -8 -p -r1.1 mozTextAccessible.mm > >--- mozTextAccessible.mm 15 Nov 2006 11:08:14 -0000 1.1 > >+++ mozTextAccessible.mm 14 Dec 2006 15:58:21 -0000 > >@@ -176,8 +176,84 @@ extern const NSString *kTopLevelUIElemen > > > > - (void)valueDidChange > > { > > NSAccessibilityPostNotification([self hasRepresentedView] ? [self representedView] : self, > > NSAccessibilityValueChangedNotification); > > } > > > > @end > >+ > >+@implementation mozComboboxAccessible > >+ > >+- (NSArray*)accessibilityAttributeNames > >+{ > >+ static NSArray *supportedAttributes = nil; > >+ if (!supportedAttributes) { > >+ // standard attributes that are shared and supported by all generic elements. > >+ supportedAttributes = [[NSArray alloc] initWithObjects:NSAccessibilityParentAttribute, // required > > You didn't declare Help attribute. Why? > > >+ NSAccessibilitySubroleAttribute, > > Sorry, please remind me may every accessible role have subrole? If no then what > is subrole supposed for combobox? Good catches. > > >+- (NSArray *)accessibilityActionNames > >+{ > >+ if ([self isEnabled]) { > >+ return [NSArray arrayWithObjects:NSAccessibilityConfirmAction, > >+ NSAccessibilityShowMenuAction, > >+ nil]; > >+ } > >+ return nil; > >+} > >+- (void)accessibilityPerformAction:(NSString *)action > >+{ > >+ // both the ShowMenu and Click action do the same thing. > >+ if ([self isEnabled]) { > > I don't remember exactly we talked already about something similar. And if I'm > right then one check of isEnabled is redudrant. I vaguely remember that too... But in this case, accessibilityPerformAction is called directly from the OS so there is no previous check. > > >+- (void)showMenu > >+{ > >+ // currently unimplemented. waiting for support in bug 363697 > >+} > >+ > >+- (void)confirm > >+{ > >+ // currently unimplemented. waiting for support in bug 363697 > > I don't see any related with this in bug 363697. That's probably because I > don't know what should do confirm method :) Yeah, actually that's a mistake. bug 363697 won't give comboboxes the abilitity to "virtually" press enter (confirm). I'll fix it. > > > // we'll flatten buttons and checkboxes. usually they have a text node > > // child, that is their title. Works in conjunction with IsPruned() below. > > PRBool IsFlat() { > > PRUint32 role = Role(this); > > return (role == ROLE_CHECKBUTTON || > > role == ROLE_PUSHBUTTON || > > role == ROLE_TOGGLE_BUTTON || > > role == ROLE_SPLITBUTTON || > >- role == ROLE_ENTRY || > >- role == ROLE_AUTOCOMPLETE); > >+ role == ROLE_ENTRY); > > } > > Why this change? What does exactly IsFlat do and where is it used? IsFlat() returns true for all the widgets that should be flattened. That means that they do not expose their children. For example, if buttons expose their children, the OS is confused. This was needed for many of these widgets to work with VoiceOver. > > >- case ROLE_AUTOCOMPLETE: > >- return [mozTextAccessible class]; > >+ { > >+ // if this textfield has a popup, it's a combobox. > >+ if (HasPopup()) > >+ return [mozComboboxAccessible class]; > > Do you have an example where it is valid? No, actually not, but it should be okay in theory, no? > > > case ROLE_COMBOBOX: > > return [mozPopupButtonAccessible class]; > > Why not mozComboboxAccessible? Because ROLE_COMBOBOX in our code means a popupmenu. Our constants suck. :(

Comment on attachment 248995 [details] [diff] [review] Combobox v2 looks ok

Checked in, thanks for the reviews!