412288 - Toolbar menus do not have ATK state focusable

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Scott Haeger]

Toolbar menus do not have ATK state 'focusable' even though they trigger focus events and have state 'focused' when in focus.

Comment 1

[Aaron Leventhal]

Neil, do you know what API we can use to find if a menuitem is focusable? For example, a disabled menu item would return FALSE on Linux but TRUE on Windows, since it's only focusable on Windows.

Comment 2

[Neil Deakin]

It's one of the look and feel metrics:

nsILookAndFeel::eMetric_SkipNavigatingDisabledMenuItem

Comment 3

[Ginn Chen]

Scott, what do you mean by "Toolbar menus"?

Do you mean the popup menu when you clicks "Smart Bookmarks" or "Places"?

I think every menu/menuitem in Firefox has this issue, and it's not Linux only.

We call frame->IsFocusable() in GetState(), but it seems IsFocusable() is a checking for tab-able.  

Comment 4

[Scott Haeger]

Hey Ginn.  I am referring to the top menus (File, Edit, View ...).  

Comment 5

[Marco Zehe (:MarcoZ)]

Attachment: Set focusable and selectable state if the look and feel is not to skip disabled menu items.

Aaron, is this what you had in mind?

Comment 6

[Marco Zehe (:MarcoZ)]

Attachment: Patch v2, also consider if disabled menu items should be skipped, but current item is available

Comment 7

[Aaron Leventhal]

Comment on attachment 297174 [details] [diff] [review]
Patch v2, also consider if disabled menu items should be skipped, but current item is available

Very close! But, you only need to use the look and feel service if the menuitem is disabled. In the most common case you don't even need to check.

Comment 8

[Marco Zehe (:MarcoZ)]

Attachment: Patch v3, address Aaron's comment

Now I first check if the menu item is disabled. If that's the case, ask the service if the item should be navigable. If not, return, if yes, simply continue after this if block with regular adding of states selectable and focusable.

Comment 9

[Aaron Leventhal]

Comment on attachment 297199 [details] [diff] [review]
Patch v3, address Aaron's comment

There is an extra space in front of this line:
                nsIAccessibleStates::STATE_SELECTABLE);

If you want you could try to save a little code and put the focusable/selectable handling outside of the if/else. Are disabled combo box options skipped on the various platforms?

Comment 10

[Marco Zehe (:MarcoZ)]

Attachment: Patch v4, unify logic for selectable and focusable.

Comment 11

[Aaron Leventhal]

Comment on attachment 297210 [details] [diff] [review]
Patch v4, unify logic for selectable and focusable.

From the logic it looks like you are setting the state before the } but it should be after, and thus are only setting it if the item is disabled.

Comment 12

[Marco Zehe (:MarcoZ)]

Attachment: Patch v5, addressing Aaron's comment

Sorry, fat-fingered that previous one!

Comment 13

[Aaron Leventhal]

Comment on attachment 297215 [details] [diff] [review]
Patch v5, addressing Aaron's comment

Okay, I'm not sure the role check is really necessary so a comment of some kind there might be helpful, but no new patch is necessary.

Comment 14

[Ginn Chen]

(In reply to comment #9)
> (From update of attachment 297199 [details] [diff] [review])
> There is an extra space in front of this line:
>                 nsIAccessibleStates::STATE_SELECTABLE);
> 
> If you want you could try to save a little code and put the
> focusable/selectable handling outside of the if/else. Are disabled combo box
> options skipped on the various platforms?
> 

I think patch v4 adds an unnecessary call of Role(this).
We'd better avoid it.

Comment 15

[Marco Zehe (:MarcoZ)]

Attachment: Patch v6

1. Eliminate duplicate call of Role(this).
2. Add comment explaining why combobox items are included in the check for whether focusable and selectable should be set.

Ginn, is this what you meant?

Comment 16

[Ginn Chen]

close,

I think we can use a PRBool isOption, instead of PRInt32 itemRole.
Also can you do word wrapping at column 80,
it will look better in terminal.

And there 2 extra spaces before last line
        nsIAccessibleStates::STATE_SELECTABLE);

Comment 17

[Marco Zehe (:MarcoZ)]

Attachment: Patch v7, address Ginn's comments

Comment 18

[Ginn Chen]

Comment on attachment 297315 [details] [diff] [review]
Patch v7, address Ginn's comments

r=me

We can correct the spacing and wrapping when check it in.

Comment 19

[Reed Loden [:reed]]

(In reply to comment #18)
> We can correct the spacing and wrapping when check it in.

If you want me to check it in for you, please submit a final patch with all nits addressed and then add the checkin-needed keyword. Otherwise, I guess Ginn or Aaron can do it.

Comment 20

[Ginn Chen]

Checking in nsXULMenuAccessible.cpp;
/cvsroot/mozilla/accessible/src/xul/nsXULMenuAccessible.cpp,v  <--  nsXULMenuAccessible.cpp
new revision: 1.71; previous revision: 1.70
done

Comment 21

[Marco Zehe (:MarcoZ)]

Verified using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3. This fix also applied to Windows, not just Linux, so verifying it on Win is OK.