Last Comment Bug 412288 - Toolbar menus do not have ATK state focusable
: Toolbar menus do not have ATK state focusable
Status: VERIFIED FIXED
orca:low
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Marco Zehe (:MarcoZ) on PTO until August 15
:
Mentors:
Depends on:
Blocks: aria fox3access
  Show dependency treegraph
 
Reported: 2008-01-14 08:53 PST by Scott Haeger
Modified: 2008-02-06 01:47 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Set focusable and selectable state if the look and feel is not to skip disabled menu items. (1.75 KB, patch)
2008-01-15 07:13 PST, Marco Zehe (:MarcoZ) on PTO until August 15
no flags Details | Diff | Splinter Review
Patch v2, also consider if disabled menu items should be skipped, but current item is available (1.80 KB, patch)
2008-01-15 08:09 PST, Marco Zehe (:MarcoZ) on PTO until August 15
aaronlev: review-
Details | Diff | Splinter Review
Patch v3, address Aaron's comment (1.84 KB, patch)
2008-01-15 10:30 PST, Marco Zehe (:MarcoZ) on PTO until August 15
aaronlev: review+
Details | Diff | Splinter Review
Patch v4, unify logic for selectable and focusable. (2.39 KB, patch)
2008-01-15 11:03 PST, Marco Zehe (:MarcoZ) on PTO until August 15
aaronlev: review-
Details | Diff | Splinter Review
Patch v5, addressing Aaron's comment (2.38 KB, patch)
2008-01-15 11:32 PST, Marco Zehe (:MarcoZ) on PTO until August 15
aaronlev: review+
Details | Diff | Splinter Review
Patch v6 (2.98 KB, patch)
2008-01-15 22:46 PST, Marco Zehe (:MarcoZ) on PTO until August 15
no flags Details | Diff | Splinter Review
Patch v7, address Ginn's comments (2.96 KB, patch)
2008-01-15 23:49 PST, Marco Zehe (:MarcoZ) on PTO until August 15
ginn.chen: review+
aaronlev: review+
mtschrep: approval1.9+
Details | Diff | Splinter Review

Description Scott Haeger 2008-01-14 08:53:47 PST
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 2008-01-14 09:33:49 PST
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 2008-01-14 09:38:53 PST
It's one of the look and feel metrics:

nsILookAndFeel::eMetric_SkipNavigatingDisabledMenuItem
Comment 3 Ginn Chen 2008-01-15 04:43:47 PST
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 2008-01-15 06:26:55 PST
Hey Ginn.  I am referring to the top menus (File, Edit, View ...).  
Comment 5 Marco Zehe (:MarcoZ) on PTO until August 15 2008-01-15 07:13:10 PST
Created attachment 297167 [details] [diff] [review]
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) on PTO until August 15 2008-01-15 08:09:37 PST
Created attachment 297174 [details] [diff] [review]
Patch v2, also consider if disabled menu items should be skipped, but current item is available
Comment 7 Aaron Leventhal 2008-01-15 10:08:22 PST
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) on PTO until August 15 2008-01-15 10:30:50 PST
Created attachment 297199 [details] [diff] [review]
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 2008-01-15 10:39:20 PST
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) on PTO until August 15 2008-01-15 11:03:30 PST
Created attachment 297210 [details] [diff] [review]
Patch v4, unify logic for selectable and focusable.
Comment 11 Aaron Leventhal 2008-01-15 11:17:44 PST
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) on PTO until August 15 2008-01-15 11:32:00 PST
Created attachment 297215 [details] [diff] [review]
Patch v5, addressing Aaron's comment

Sorry, fat-fingered that previous one!
Comment 13 Aaron Leventhal 2008-01-15 11:39:31 PST
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 2008-01-15 21:44:20 PST
(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) on PTO until August 15 2008-01-15 22:46:32 PST
Created attachment 297311 [details] [diff] [review]
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 2008-01-15 23:08:16 PST
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) on PTO until August 15 2008-01-15 23:49:26 PST
Created attachment 297315 [details] [diff] [review]
Patch v7, address Ginn's comments
Comment 18 Ginn Chen 2008-01-15 23:56:07 PST
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] (use needinfo?) 2008-01-16 22:03:00 PST
(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 2008-01-16 22:30:37 PST
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) on PTO until August 15 2008-02-06 01:47:13 PST
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.

Note You need to log in before you can comment on or make changes to this bug.