[Mac a11y] Make all buttons with a popup get the ShowMenu action

RESOLVED FIXED

Status

()

Core
Disability Access APIs
--
enhancement
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Håkan Waara, Assigned: Håkan Waara)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

12 years ago
I can make all buttons that has the STATE_HASPOPUP (which mean the button will pop up a menu) be instances of mozPopupButtonAccessible. That way they get the ShowMenu action too, in addition to the button's Click action.
(Assignee)

Comment 1

12 years ago
Created attachment 247844 [details] [diff] [review]
Fix v1
Attachment #247844 - Flags: review?(surkov.alexander)
Comment on attachment 247844 [details] [diff] [review]
Fix v1

Patch looks ok.

>+    PRBool HasPopup () {
>+      PRUint32 state = 0;
>+      GetState(&state);
>+      return (state & nsIAccessible::STATE_HASPOPUP);
>+    }

nit: since HasPopup() is very short, then probably it's fine if you place that code inside case statement. But it's up to you.

The other things is not related to this patch but perhaps they makes sense.

> nsAccessibleWrap::GetNativeType () 

I guess it's not good name for this method. Is it GetMacAccessible better?

>   PRUint32 role = Role(this);
>   switch (role) {

This switch statement looks like kind of dublicate of RoleMap.h. Should you use mac roles instead gecko roles (probably not here)?

>     case ROLE_PUSHBUTTON:
>     case ROLE_SPLITBUTTON:
>     case ROLE_TOGGLE_BUTTON:
>+    {

RoleMap.h maps these roles to NSAccessibilityButtonRole. But NSAccessibilityButtonRole doesn't assume to have popup I guess.
Attachment #247844 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 3

12 years ago
(In reply to comment #2)
> (From update of attachment 247844 [details] [diff] [review] [edit])
> Patch looks ok.
> 
> >+    PRBool HasPopup () {
> >+      PRUint32 state = 0;
> >+      GetState(&state);
> >+      return (state & nsIAccessible::STATE_HASPOPUP);
> >+    }
> 
> nit: since HasPopup() is very short, then probably it's fine if you place that
> code inside case statement. But it's up to you.

I find it much easier to understand code if specific actions are divided up in helper methods (that can also be reused(, instead of keeping all logic in one place.

> 
> The other things is not related to this patch but perhaps they makes sense.
> 
> > nsAccessibleWrap::GetNativeType () 
> 
> I guess it's not good name for this method. Is it GetMacAccessible better?

Hmm, note that it just gets the type (class type), it does not actually get an instance of an object. That's what GetNativeInterface() does (that's an nsIAccessible method).

> 
> >   PRUint32 role = Role(this);
> >   switch (role) {
> 
> This switch statement looks like kind of dublicate of RoleMap.h. Should you use
> mac roles instead gecko roles (probably not here)?

Not sure what you mean, this role check is done before a mac object exists. It's so I can know what class type the native mac object should have.

> 
> >     case ROLE_PUSHBUTTON:
> >     case ROLE_SPLITBUTTON:
> >     case ROLE_TOGGLE_BUTTON:
> >+    {
> 
> RoleMap.h maps these roles to NSAccessibilityButtonRole. But
> NSAccessibilityButtonRole doesn't assume to have popup I guess.
> 

Good catch, and that's intentional. The nice thing is that buttons (that should have the button role) will under-the-hood be mozPopupButtonAccessible instances, so they will have the ShowMenu action (which is correct).
(Assignee)

Comment 4

12 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.