The default bug view has changed. See this FAQ.

Toolbar menus do not have ATK state focusable

VERIFIED FIXED

Status

()

Core
Disability Access APIs
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: Scott Haeger, Assigned: MarcoZ)

Tracking

(Blocks: 1 bug, {access})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: orca:low)

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

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

Updated

9 years ago
Assignee: aaronleventhal → marco.zehe
Blocks: 343213, 396346
Keywords: access

Comment 1

9 years ago
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

9 years ago
It's one of the look and feel metrics:

nsILookAndFeel::eMetric_SkipNavigatingDisabledMenuItem

Updated

9 years ago
Whiteboard: orca:low

Comment 3

9 years ago
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.  
(Reporter)

Comment 4

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

Comment 5

9 years ago
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?
Attachment #297167 - Flags: review?(aaronleventhal)
(Assignee)

Comment 6

9 years ago
Created attachment 297174 [details] [diff] [review]
Patch v2, also consider if disabled menu items should be skipped, but current item is available
Attachment #297167 - Attachment is obsolete: true
Attachment #297174 - Flags: review?(aaronleventhal)
Attachment #297167 - Flags: review?(aaronleventhal)

Comment 7

9 years ago
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-
(Assignee)

Comment 8

9 years ago
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.
Attachment #297174 - Attachment is obsolete: true
Attachment #297199 - Flags: review?(aaronleventhal)

Comment 9

9 years ago
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+
(Assignee)

Comment 10

9 years ago
Created attachment 297210 [details] [diff] [review]
Patch v4, unify logic for selectable and focusable.
Attachment #297199 - Attachment is obsolete: true
Attachment #297210 - Flags: review?(aaronleventhal)

Comment 11

9 years ago
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-
(Assignee)

Comment 12

9 years ago
Created attachment 297215 [details] [diff] [review]
Patch v5, addressing Aaron's comment

Sorry, fat-fingered that previous one!
Attachment #297210 - Attachment is obsolete: true
Attachment #297215 - Flags: review?(aaronleventhal)

Comment 13

9 years ago
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+

Comment 14

9 years ago
(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.


(Assignee)

Comment 15

9 years ago
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?
Attachment #297215 - Attachment is obsolete: true
Attachment #297311 - Flags: review?(ginn.chen)

Comment 16

9 years ago
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);

(Assignee)

Comment 17

9 years ago
Created attachment 297315 [details] [diff] [review]
Patch v7, address Ginn's comments
Attachment #297311 - Attachment is obsolete: true
Attachment #297315 - Flags: review?(ginn.chen)
Attachment #297311 - Flags: review?(ginn.chen)

Comment 18

9 years ago
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+
(Assignee)

Updated

9 years ago
Attachment #297315 - Flags: approval1.9?

Updated

9 years ago
Attachment #297315 - Flags: review+

Updated

9 years ago
Attachment #297315 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
(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.
Keywords: checkin-needed

Comment 20

9 years ago
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
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 21

9 years ago
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.