Closed Bug 369777 Opened 14 years ago Closed 14 years ago

Separate programmatic action names from localized description


(Core :: Disability Access APIs, defect)

Not set





(Reporter: davidb, Assigned: davidb)


(Blocks 1 open bug)


(Keywords: access)


(6 files, 1 obsolete file)

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

Perhaps we need to use the key/value pairs in our localized 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)
Blocks: ia2, atk
Assignee: aaronleventhal → david.bolter
Attached patch patch for discussion (obsolete) — Splinter Review
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"

If we use def's where should they go?
Note current version of nsXULFormControlAccessible.cpp sometimes calls to localize and sometimes does not in the various GetActionName implementations.
Note: this work will proceed. The gnome accessibility developers have agreed it is best not to localize action names.
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")
Attachment #254998 - Attachment is obsolete: true
Attachment #255523 - Flags: review?(aaronleventhal)
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.
Attachment #255523 - Flags: review?(aaronleventhal) → review-
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:
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.
(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.
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.
Attached patch patch v2Splinter Review
Added nsIAccessible getActionDescription.
Changed "_retval" params to "aName".
Attachment #255853 - Flags: review?(aaronleventhal)
Why isn't GetActionDescription() implemented?

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

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.
GetActionName is virtual. All XPCOM interface methods are.
Attachment #255853 - Flags: review?(aaronleventhal)
Attached patch patch v3Splinter Review
Aaron, you are bang on of course. I could have sworn it wasn't acting virtual last night. How's this patch looking?
Attachment #255979 - Flags: review?(aaronleventhal)
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

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.
Attachment #255979 - Flags: review?(aaronleventhal) → review-
Attached patch patch v4Splinter Review
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).
Attachment #256129 - Flags: review?(aaronleventhal)
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.
Attachment #256129 - Flags: review?(aaronleventhal) → review+
Attached patch patch to go inSplinter Review
Thanks Aaron.
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
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.
Aaron, I've tested this patch to succeed (against a clean trunk).  I hope it works!
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.