440568 - Mnemonics not exposed for clickable items in Firefox and Thunderbird

Status: RESOLVED DUPLICATE of bug 672092

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

According to this Atk documentation:
http://library.gnome.org/devel/atk/1.22/AtkAction.html#atk-action-get-keybinding

atk_action_get_keybinding "Returns a keybinding associated with this action, if
one exists. The returned string is in the format "<a>;<b>;<c>" (i.e.
semicolon-delimited), where <a> is the keybinding which activates the object if
it is presently enabled onscreen, <b> corresponds to the keybinding or sequence
of keys which invokes the action even if the relevant element is not currently
posted on screen (for instance, for a menu item it posts the parent menus
before invoking). The last token in the above string, if non-empty, represents
a keyboard shortcut which invokes the same action without posting the component
or its enclosing menus or dialogs."

When we ask for the keybinding, all we get is element c from the above string.

Comment 1

[Joanmarie Diggs]

Note that this applies to menus as well as to dialog box controls.

Comment 2

[Aaron Leventhal]

Marco, this hurts WebVisum usability with Orca (as I read on the mailing lists).

Comment 3

[Marco Zehe (:MarcoZ)]

(In reply to comment #0)
> atk_action_get_keybinding "Returns a keybinding associated with this action, if
> one exists. The returned string is in the format "<a>;<b>;<c>" (i.e.
> semicolon-delimited), where <a> is the keybinding which activates the object if
> it is presently enabled onscreen, <b> corresponds to the keybinding or sequence
> of keys which invokes the action even if the relevant element is not currently
> posted on screen (for instance, for a menu item it posts the parent menus
> before invoking). The last token in the above string, if non-empty, represents
> a keyboard shortcut which invokes the same action without posting the component
> or its enclosing menus or dialogs."

Sorry, I've tried to parse this three times now, and I still don't get what this doc is trying to tell me.

Can you give me a simple example and tell me what you are getting, and what, in contrast to that, you're expecting to get?

Comment 4

[Joanmarie Diggs]

As an example, I happen to be in Evolution.  If I look at the Close button of the Preferences dialog in Accerciser, I see that <Alt C> is the key binding associated with the click action. That tells me that I don't have to have focus on the button and can activate/click it with Alt C. (I know you know that *smile*, but for the sake of being thorough....)

Another example: If I use Accerciser to examine the New Window menu item of Evolution's File menu I see the following exposed in association with the click action: "w;<Alt>f:w;<Shift><Control>W".  Breaking that down:

1. w - that's the underlined letter.  (i.e. if I'm in the menu, but not on the item, w will activate/click it).

2. <Alt>f:w - the full list of keys I need to press to activate it from outside of the menu, but ultimately using the menu item.

3. <Shift><Control>W - the sequence of keys I need to press if I want to perform that task without actually activating its associated menu item.

In comparison, in Firefox's Preference dialog, the Close button doesn't have an underlined letter/hotkey, so I went with the Help button.  There is no click action.  There is only a press action.  They keybinding associated with this action is listed in Accerciser as ";".

In terms of menu items, looking at Firefox's Close Window menu item, there is a click action with an associated keybinding of ";<Control><Shift>W.  So:

1. d is the underlined letter and should be exposed to us as the first component.  It's not.

2. <Alt>f:d is the full list of keys I'd need to press to access it via the menu.  Personally, I've always thought of this component as being redundant (What I really care about is the underlined letter).  But technically it's supposed to be provided as well.

3. The third component <Control><Shift>W is where it should be.

Hopefully this clarifies things a bit.

As for the impact, we can't provide users with the mnemonics associated with buttons (including those "toolbar" alerts for remembering passwords and accessing blocked add-ons) or of menu items.  So it would really nice if a fix for this issue could be implemented.

Thanks!!

Comment 5

[Joanmarie Diggs]

Ping.

Comment 6

[Fernando Herrera]

Menu items shortcuts and keybindings are show correctly here with the lastest code.  However buttons still do not show them.

Currently nsMaiInterfaceAction.cpp only implements shortcuts for those elements whose parent is ATK_ROLE_MENU_BAR, ATK_ROLE_MENU, or ATK_ROLE_MENU_ITEM.

Comment 7

[Fernando Herrera]

Attachment: Patch for getKeyBindingCB for push buttons

This patch fills allKeyBinding for elements not contained in menus (right now only push button)

Comment 8

[Fernando Herrera]

Comment on attachment 474070 [details] [diff] [review]
Patch for getKeyBindingCB for push buttons

btw, the comment show be

//it is a button, change from "Alt+h" to "h;<Alt>h"

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 474070 [details] [diff] [review]
Patch for getKeyBindingCB for push buttons

Hey thanks for the patch!

>+++ b/accessible/src/atk/nsMaiInterfaceAction.cpp	Fri Sep 10 17:48:40 2010 +0200
>@@ -156,6 +156,17 @@ getKeyBindingCB(AtkAction *aAction, gint
>                 allKeyBinding = accessKey + NS_LITERAL_STRING(";<Alt>") +
>                                 allKey;
>             }
>+            else {
>+                //it is not insise a menu

Nit: "// it is not inside a menu"

>+                if (atkRoleMap[accWrap->NativeRole()] == ATK_ROLE_PUSH_BUTTON) {

It would seem to match the style of this source file better to just collapse the else and if into else if. So instead of else above, just put else if (...).

>+                    //it is a button, change from "Alt+h" to "f;<Alt>h"

Nit: should be "h;<Alt>h".

The rest looks good thanks. Please re-ask for review with an updated patch.

Comment 10

[Fernando Herrera]

Attachment: Updated patch

Updated patch addressing comments in the review and adding also shortcuts information for RadioButton, ToggleButton and CheckBox

Comment 11

[Fernando Herrera]

Attachment: Updated patch

Updated the comment to reflect that button shortcuts as in the form "<Alt>b;" as per:
http://library.gnome.org/devel/atk/unstable/AtkAction.html#atk-action-get-keybinding

as they can only be activated when they are shown on screen.

Comment 12

[Fernando Herrera]

Attachment: Updated patch adding the acel for all widgets not in menus

Sorry for the spam :)

After digging into more widgets, ComboBox, SpinButton, etc.. were missing, so I think it's better just to make this "<Alt>a;" assing for every widget not in a menu.

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 474117 [details] [diff] [review]
Updated patch adding the acel for all widgets not in menus

r=me thanks, because I can't think of reasons to special case further. Asking Ginn for sign off.

Comment 14

[Ginn Chen]

Comment on attachment 474117 [details] [diff] [review]
Updated patch adding the acel for all widgets not in menus

Thanks for fixing this!

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

Great.

Fernando, we are currently in a stabilization period for Firefox 4 whereby we allow only must-have features, and must-have fixes. This means only work that is done on a blocking bug (a show stopper bug for Firefox 4 release), or alternately, approved patches may land on trunk at this time. So if you would like to have this landed for FF4 here is what you do:
1. click on the details link for the final patch to land.
2. click on the approval2.0 combo box and select '?'
3. submit.

If you don't get an answer soon, ping me.

Comment 16

[Ginn Chen]

Comment on attachment 474117 [details] [diff] [review]
Updated patch adding the acel for all widgets not in menus

I'm sorry, I have to revert my review.
I didn't read the whole spec yesterday.
I think we should use ";<Alt>h;" not "<Alt>h;".
Asking Joanie's feedback to confirm.

Comment 17

[Joanmarie Diggs]

I've admittedly not tried the patch yet. BUT, looking at what the spec says:

The returned string is in the format "<a>;<b>;<c>" (i.e. semicolon-delimited), where <a> is the keybinding which activates the object if it is presently enabled onscreen

[[joanie->ginn: I think dialog box widgets fall under this <a> category. And the keybinding to activate a dialog box control which is visible is Alt+UnderlinedLetter.]]

<b> corresponds to the keybinding or sequence of keys which invokes the action even if the relevant element is not currently posted on screen (for instance, for a menu item it posts the parent menus before invoking).

[[joanie->ginn: I don't think dialog box widgets belong in <b>. Looking at gtk-demo for menu items, <b> is <Alt>f:s (whereas <a> is just s). This makes sense to me: If I'm in the File menu, I can activate the Save menu item by just pressing s. If I'm not yet in the File menu, I have to do <Alt>f first, then s. If there were going to be a <b> for widgets which are not in menus, wouldn't the value be the keystroke needed to get into the dialog box first, followed by <Alt>whatever?]]

...

So, again, just from reading; not from trying it yet... I *believe* Fer is correct (unless <Alt>h;; is an option. ;-) ). Why do you think it should be ;<Alt>h; instead?

Comment 18

[Joanmarie Diggs]

Sorry for the spam. Dunno how this got changed from Linux to Win CE. :-/

Comment 19

[Ginn Chen]

Comment on attachment 474117 [details] [diff] [review]
Updated patch adding the acel for all widgets not in menus

I was confused by the word "activates" and "invokes the action".
Yes, the patch is doing the same thing as GTK+ programs.
r=me

Comment 20

[Marco Zehe (:MarcoZ)]

Fernando, anything on this one?

Comment 21

[Joanmarie Diggs]

Another a11y bug filed in 2008.... Ping?

Comment 22

[Trevor Saunders (:tbsaunde)]

(In reply to Joanmarie Diggs from comment #21)
> Another a11y bug filed in 2008.... Ping?

uhm, I have no idea why we didn't land fer's patch :(

However I think this was fixed by bug 672092.  I seem to remember checking pretty careful, but if you know of cases that are still broken please mention them.

Comment 23

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #22)
> (In reply to Joanmarie Diggs from comment #21)
> > Another a11y bug filed in 2008.... Ping?
> 
> uhm, I have no idea why we didn't land fer's patch :(

I think fer just didn't landed it and nobody did.

> However I think this was fixed by bug 672092.

that seems correct.

>  I seem to remember checking
> pretty careful, but if you know of cases that are still broken please
> mention them.

please reopen it in that case, otherwise it's dupe of bug 672092