Separate programmatic action names from localized description

RESOLVED FIXED

Status

()

Core
Disability Access APIs
--
major
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: davidb, Assigned: davidb)

Tracking

(Blocks: 1 bug, {access})

Trunk
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

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)

Updated

11 years ago
Blocks: 368873, 368881
Assignee: aaronleventhal → david.bolter
Status: NEW → ASSIGNED
Created attachment 254998 [details] [diff] [review]
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?
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.
(http://mail.gnome.org/archives/gnome-accessibility-devel/2007-February/msg00006.html)
Created attachment 255523 [details] [diff] [review]
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")
Attachment #254998 - Attachment is obsolete: true
Attachment #255523 - Flags: review?(aaronleventhal)

Comment 5

11 years ago
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:
aName
aActionName
anActionName
theName
outName
outActionName
snarklegruff
other?

Comment 7

11 years ago
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

11 years ago
(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.
Created attachment 255853 [details] [diff] [review]
patch v2

Added nsIAccessible getActionDescription.
Changed "_retval" params to "aName".
Attachment #255853 - Flags: review?(aaronleventhal)

Comment 11

11 years ago
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.

Comment 13

11 years ago
GetActionName is virtual. All XPCOM interface methods are.
Attachment #255853 - Flags: review?(aaronleventhal)
Created attachment 255979 [details] [diff] [review]
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?
Attachment #255979 - Flags: review?(aaronleventhal)

Comment 15

11 years ago
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.
Attachment #255979 - Flags: review?(aaronleventhal) → review-
Created attachment 256129 [details] [diff] [review]
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).
Attachment #256129 - Flags: review?(aaronleventhal)

Comment 17

11 years ago
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+
Created attachment 256163 [details] [diff] [review]
patch to go in

Thanks Aaron.

Comment 19

11 years ago
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.
Created attachment 256243 [details] [diff] [review]
patch to go in (not malformed)

Aaron, I've tested this patch to succeed (against a clean trunk).  I hope it works!
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.