235690 - mozilla menu items' AtkAction interface is nonfunctional

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Bill Haneman]

Mozilla's menu items, on Linux with ATK implementation, need to implement the
AtkAction interface.  They do expose this interface, but there are two serious
problems that mean that menu items cannot be used with assistive technologies:

(1) the "action name" and "action description" are corrupt (possibly not valid
UTF-8?) and thus cannot be read/parsed or presented to the user;

(2) if the actions are invoked via atk_action_do_action, nothing happens.

The expected result of #2 would be for the menu items to be activated/invoked,
as if they were selected by the user via keynav or mouse.

To reproduce:
1) start mozilla on a11y-enabled gnome environment;
2) run at-poke to see menus;
3) try to use at-poke to activate menu items (via 'Take Action' button).

Expected result: first action in action list is named "click", and doing "Take
Action" should activate/invoke the menu item.

Actual result: first action in action list has mangled/invalid name, and "Take
Action" has no effect.

Similar results can be seen with other test frameworks, so I don't think at-poke
is to blame.

This makes menus unusable by users of GOK and similar assistive technologies for
mobility problems.

Comment 1

[Kyle Yuan]

-> Louie

Comment 2

[Kyle Yuan]

Look at
http://lxr.mozilla.org/seamonkey/source/accessible/src/xul/nsXULMenuAccessible.cpp#191

I think the code is wrong here. |selectItem| is alwasys not nsnull when the
accessible object is a xul menu item.

Comment 3

[Louie Zhao]

Attachment: patch v1

Comment 4

[Louie Zhao]

Attachment: patch v2

update according to kyle's advise

Comment 5

[Kyle Yuan]

Comment on attachment 142897 [details] [diff] [review]
patch v2

r=kyle

Comment 6

[Henry Jia]

Comment on attachment 142897 [details] [diff] [review]
patch v2

Just one thing, why using PL_strlen(name)?

Comment 7

[Kyle Yuan]

Because PL_strlen is null-pointer-proof, I think.

Comment 8

[Henry Jia]

No. In this case, PL_strlen doesn't help. Also, PL_strlen is changed to strlen
directly.

Comment 9

[Kyle Yuan]

Please see its implementation: 
http://lxr.mozilla.org/seamonkey/source/nsprpub/lib/libc/src/strlen.c#40

Seems it is only defined to strlen in libjar.
http://lxr.mozilla.org/seamonkey/source/modules/libjar/zipstub.h#43

Comment 10

[Philip K. Warren]

*** Bug 233207 has been marked as a duplicate of this bug. ***

Comment 11

[Philip K. Warren]

*** Bug 234533 has been marked as a duplicate of this bug. ***

Comment 12

[Henry Jia]

We should use strlen instead of PL_strlen to decide the length of a string. The
programmer should be responsable for the string usage. 

You know that "" and a null pointer are different. But PL_strlen all return 0.
So in the code, if we just want to check if this is a null pointer, we should
roll back to the previous usage. If we also want to check if the string is equal
to "", we may use strlen.

Comment 13

[Louie Zhao]

Attachment: patch v3

Using strlen instead of PL_strlen

Comment 14

[Henry Jia]

Comment on attachment 142986 [details] [diff] [review]
patch v3

sr=Henry

Also, take r=Kyle from previous patch review.

Thanks.

Comment 15

[Michael Lew]

Is this patch ready to be checked in?

Comment 16

[Louie Zhao]

Thanks for r and sr. Patch checked in.

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #12)
> If we also want to check if the string is equal
> to "", we may use strlen.

faster would be to check if *str == 0...

Comment 18

[Philip K. Warren]

This bug was not really fixed. The patch was checked in, but later backed out.

Comment 19

[Philip K. Warren]

Comment on attachment 142986 [details] [diff] [review]
patch v3

>Index: accessible/src/atk/nsMaiInterfaceAction.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/atk/nsMaiInterfaceAction.cpp,v
>retrieving revision 1.3
>diff -u -r1.3 nsMaiInterfaceAction.cpp
>--- accessible/src/atk/nsMaiInterfaceAction.cpp	19 Feb 2004 02:43:11 -0000	1.3
>+++ accessible/src/atk/nsMaiInterfaceAction.cpp	4 Mar 2004 09:40:21 -0000
>@@ -140,7 +140,7 @@
>     NS_ENSURE_TRUE(action, nsnull);
> 
>     const char *name = action->GetName();
>-    if (!name) {
>+    if (!name || (strlen(name) == 0)) {

Can this be changed to:

if (!name || !*name) {

as biesi has suggested in comment 17?

Comment 20

[Louie Zhao]

Attachment: updated patch v3

minor change following the advise above

Comment 21

[Louie Zhao]

Comment on attachment 144815 [details] [diff] [review]
updated patch v3

This bug is important for Accessibility (disabled in default build).

Comment 22

[chris hofmann]

Comment on attachment 144815 [details] [diff] [review]
updated patch v3

a=chofmann for 1.7

Comment 23

[Philip K. Warren]

Fixed.

Checking in atk/nsMaiInterfaceAction.cpp;
/cvsroot/mozilla/accessible/src/atk/nsMaiInterfaceAction.cpp,v  <-- 
nsMaiInterfaceAction.cpp
new revision: 1.6; previous revision: 1.5
done
Checking in xul/nsXULFormControlAccessible.cpp;
/cvsroot/mozilla/accessible/src/xul/nsXULFormControlAccessible.cpp,v  <-- 
nsXULFormControlAccessible.cpp
new revision: 1.27; previous revision: 1.26
done
Checking in xul/nsXULMenuAccessible.cpp;
/cvsroot/mozilla/accessible/src/xul/nsXULMenuAccessible.cpp,v  <-- 
nsXULMenuAccessible.cpp
new revision: 1.21; previous revision: 1.20
done
Checking in xul/nsXULTabAccessible.cpp;
/cvsroot/mozilla/accessible/src/xul/nsXULTabAccessible.cpp,v  <-- 
nsXULTabAccessible.cpp
new revision: 1.19; previous revision: 1.18
done