369777 - Separate programmatic action names from localized description

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

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

Currently, our (would-be "programmatic") action names are localizeable which is dangerous.

Perhaps we need to use the key/value pairs in our localized accessible.properties files in the following way:

1. The key should be used as the programmatic action name, and 
2. the value should be a locale specific description of the action.

We might wish to also have a localizeable action name in addition.

This bug might be a bit of a tracker as we will need to change our GetActionName implementations etc.

Note we might also consider moving these properties files inside the accessible module. (currently in dom module)

Comment 1

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

Attachment: patch for discussion

I'm considering changing to this way of assigning action names, do I need to perhaps use the getter_Copies operator to ensure cleanup?  

Also perhaps a style question do we want to use #defines?

E.g #define ACTION_CLICK "click"
foo.AssignLiteral(ACTION_CLICK);

If we use def's where should they go?

Comment 2

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

Note current version of nsXULFormControlAccessible.cpp sometimes calls to localize and sometimes does not in the various GetActionName implementations.

Comment 3

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

Note: this work will proceed. The gnome accessibility developers have agreed it is best not to localize action names.
(http://mail.gnome.org/archives/gnome-accessibility-devel/2007-February/msg00006.html)

Comment 4

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

Attachment: proposed patch v1

This patch: 
 * Removes localization of programmatic action names.
 * Adds parameter name consistency for GetActionName methods.
 * Implements basic getActionDescriptionCB (in nsMaiInterfaceAction)
 
The basic implementation of getActionDescriptionCB simply calls GetTranslatedString for the action name. Note I had to make GetTranslatedString public for use in this context.

Tested via at-poke, action names are now confirmed not translated (programmatic) and action descriptions are confirmed to have been processed by nsAccessible::GetTranslatedString
(in our case this means for example name="activate" and description="Activate")

Comment 5

[Aaron Leventhal]

Comment on attachment 255523 [details] [diff] [review]
proposed patch v1

>  // XXX TODO: add GetActionDescription API? 
Yes :) Otherwise surkov will have to do it. We need the localized version for IA2 actions.

Also, ins nsXULCombo you don't need to change to name to _retval. In fact whenver you see _retval it's really wrong. Slowly I'm trying to change those to use the Mozilla style, as asked for by reviewers, of aMeaningfulName. The _retval param names were an old mistake by me, from copying generated header files.

Comment 6

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

:)
Shall I file a new bug for that work (and discussion about it)? I don't know the status of the IA2 work (especially sukov's sandbox). Would the bug be "Implement the IA2 Action Interface"?  (As you know I lean towards frequent integration so I'd like to keep this patch small and commit it soon if surkov might be touching the same code -- that is, as long as you feel it is correct). If you think the API work is best done here that's fine too. I just plain don't know how involved that work will be.

Note this patch works already (for atk).

Before I go changing parameter names (for GetActionName) again let's pick one:
aName
aActionName
anActionName
theName
outName
outActionName
snarklegruff
other?

Comment 7

[Aaron Leventhal]

No, just make sure this patch does the real work via an nsIAccessible call. That way when Surkov goes to hook it up he can just call that method.

Param names in Mozilla should always be prefixed by an a, so aName or aActionName are fine.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> I don't know
> the status of the IA2 work (especially sukov's sandbox).

Status is just started :) Firstly, I think to make windows-related part of the work and then to implement IA2 methods that are absent in gecko interfaces. Btw, what is sandbox? If you file new bug and assign it to me then it would be cool with me.

Comment 9

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

Hi Surkov, I'm using the term "sandbox" in terms of cvs, where "sandbox" is slang for a developer's local working copy of the source code (that can contain changes not yet committed to back). Let me see where this bug takes me. If I file a bug for connecting it with the IA2 work I'll let you know.

Comment 10

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

Attachment: patch v2

Added nsIAccessible getActionDescription.
Changed "_retval" params to "aName".

Comment 11

[Aaron Leventhal]

Why isn't GetActionDescription() implemented?

> /* DOMString getAccActionDescription (in PRUint8 index); */
Nit: wrong name in comment.

Comment 12

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

Hi Aaron, I'll correct the comment thanks.

For implementing GetActionDescription do you mean in nsAccessible.cpp or throughout accessible (wherever there is a GetActionName implementation)? If I try to implement in nsAccessible I won't have the action name I need to actually get the localized version from (unless we can somehow make the GetActionName call virtual)?

I'm happy to try either approach.

Comment 13

[Aaron Leventhal]

GetActionName is virtual. All XPCOM interface methods are.

Comment 14

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

Attachment: patch v3

Aaron, you are bang on of course. I could have sworn it wasn't acting virtual last night. How's this patch looking?

Comment 15

[Aaron Leventhal]

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

Getting closer

1) rev the UUID in nsIAccessible
2) if failure here consider returning null (I'll leave it to you to find out if that's better than returning "", or if we already do it correctly).
3) Change in paramater |name| to |aName|
4) You don't need to do this-> ever, and you can just return the rv:
>+    rv = this->GetTranslatedString(name, aDescription);
>+    NS_ENSURE_SUCCESS(rv, nsnull);
This should just be |return GetTranslatedString(aName, aDescription);|
5) change other instances of NS_ENSURE_SUCCESS(rv, nsnull); to
   NS_ENSURE_SUCCESS(rv, rv);

The rest looks good at a glance. It'd be cool if, where you're touching a method, that you make sure it uses |aParamName|, e.g. aIndex. But I wouldn't hold the patch up for that.

Comment 16

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

Attachment: patch v4

Thanks for the catches.
Issues 1-5: all done.
Note for 2): we (i.e. nsMaiInterfaceAction) currently return null in cases like this and not doing so was a sloppy oversight on my part. I think returning null is fine and I've followed suit here.  (GNOME ATK source also returns null on a failure here).

Comment 17

[Aaron Leventhal]

Comment on attachment 256129 [details] [diff] [review]
patch v4

Just make this one change:

nsresult rv = this->GetActionName(aIndex, name);
Remove |this->| -- it's never necessary.

Comment 18

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

Attachment: patch to go in

Thanks Aaron.

Comment 19

[Aaron Leventhal]

I get a malformed patch error when I try to apply:

patching file accessible/public/nsIAccessible.idl
patching file accessible/src/atk/nsMaiInterfaceAction.cpp
patching file accessible/src/base/nsAccessible.cpp
patch: **** malformed patch at line 93: Index: accessible/src/base/nsBaseWidgetAccessible.cpp

Comment 20

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

Strange. My sandbox is at home so I'll have to wait to regenerate the patch and I'll try it out this time before posting it.

Comment 21

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

Attachment: patch to go in (not malformed)

Aaron, I've tested this patch to succeed (against a clean trunk).  I hope it works!