Closed Bug 412288 Opened 12 years ago Closed 12 years ago
Toolbar menus do not have ATK state focusable
Toolbar menus do not have ATK state 'focusable' even though they trigger focus events and have state 'focused' when in focus.
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.
It's one of the look and feel metrics: nsILookAndFeel::eMetric_SkipNavigatingDisabledMenuItem
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.
Hey Ginn. I am referring to the top menus (File, Edit, View ...).
Aaron, is this what you had in mind?
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.
Attachment #297174 - Flags: review?(aaronleventhal) → review-
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 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?
Attachment #297199 - Flags: review?(aaronleventhal) → review+
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.
Attachment #297210 - Flags: review?(aaronleventhal) → review-
Sorry, fat-fingered that previous one!
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.
Attachment #297215 - Flags: review?(aaronleventhal) → review+
(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.
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?
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 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.
Attachment #297315 - Flags: review?(ginn.chen) → review+
(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.
Checking in nsXULMenuAccessible.cpp; /cvsroot/mozilla/accessible/src/xul/nsXULMenuAccessible.cpp,v <-- nsXULMenuAccessible.cpp new revision: 1.71; previous revision: 1.70 done
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.