Toolbar menus do not have ATK state focusable

VERIFIED FIXED

Status

()

Core
Disability Access APIs
VERIFIED FIXED
10 years ago
10 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

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

Updated

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

Comment 1

10 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.
It's one of the look and feel metrics:

nsILookAndFeel::eMetric_SkipNavigatingDisabledMenuItem

Updated

10 years ago
Whiteboard: orca:low

Comment 3

10 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

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

Comment 5

10 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

10 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

10 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

10 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

10 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

10 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

10 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

10 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

10 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

10 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

10 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

10 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

10 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

10 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

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

Updated

10 years ago
Attachment #297315 - Flags: review+

Updated

10 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

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

Comment 21

10 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.