347791 - Expose accessible role and state as strings in Inspector

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Aaron Leventhal]

Since Alexander's work in bug 337674, we can now use the DOM Inspector extension to view accessible object info in DOM Inspector. This is a great thing, but it just makes me want more :)

First of all we need to be able to see role/state/extState as strings. Perhaps the easiest way to do this in Inspector will be to add GetRoleString and  GetStateString to nsIAccessible. GetStateString would need to gather the extended states as well.

Comment 1

[alexander :surkov (:asurkov)]

(In reply to comment #0)
> Since Alexander's work in bug 337674, we can now use the DOM Inspector
> extension to view accessible object info in DOM Inspector. This is a great
> thing, but it just makes me want more :)
> 
> First of all we need to be able to see role/state/extState as strings. Perhaps
> the easiest way to do this in Inspector will be to add GetRoleString and 
> GetStateString to nsIAccessible. GetStateString would need to gather the
> extended states as well.
> 

I like it. Though if nsIAccessible will have GetStateString and etc then localization will be lost. Should role/state/extState be localized?

Comment 2

[Aaron Leventhal]

I'd say don't localize them, because it's output for programmers. To me, that would be localizing element names and attributes.

For example, the string "ROLE_CHECKBOX" would be what I expect for a checkbox.

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

Patch doesn't contain any stlyles and locales.

Comment 4

[alexander :surkov (:asurkov)]

Vasiliy, you should look at the existing patch, fix UI issue there and probably reimplement constants convertion of nsIAccessibleRole and nsIAccessibleStates interfaces to human readable strings (get Aaron's opinion for this).

Comment 5

[Aaron Leventhal]

The unique part of each programmatic name for the strings would be fine.
For example, for nsIAccessibleStates::STATE_BUSY, you can just use "busy". I don't think it should be localized, otherwise it will make it more difficult to find the string in nsIAccessibleStates.idl or nsIAccessibleRole.idl.

Surkov what do you think?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> The unique part of each programmatic name for the strings would be fine.
> For example, for nsIAccessibleStates::STATE_BUSY, you can just use "busy". I
> don't think it should be localized, otherwise it will make it more difficult to
> find the string in nsIAccessibleStates.idl or nsIAccessibleRole.idl.
> 
> Surkov what do you think?
> 

Agree, they shoudn't be localized but who will provide interface for conversion? nsIAccessible? nsIAccessibilityRetrieval? nsIAccessibilityService?

Comment 7

[Aaron Leventhal]

I'm not sure. How does JS know the name?

Comment 8

[alexander :surkov (:asurkov)]

I would suggest to extend nsIAccessibilityService interface by the following way:

interface nsIAccessibilityService : nsIAccessibleRetrieval
{
  // bla bla
  AString getStringRole(in unsigned long aRole);
  nsIDOMDOMStringList getStringStates(in unsigned long aStates, in unsigned long aExtraStates);
};

It looks like common approach enough. Though I'm not sure I like names of proposed methods.

Comment 9

[alexander :surkov (:asurkov)]

Vasiliy, feel free to file new bug about conversion methods for ally interface and fix it otherwise patch of this bug will be too large.

Comment 10

[Vasiliy Potapenko]

Attachment: patch

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 270702 [details] [diff] [review]
patch

cancelling request until comments are addressed.

>+
>+  destroy: function destroy() {},

Where is the method used? Does viewer object requires it?

>+  // private
>+  updateView: function updateView() {
>+    try {
>+      this.mAccSubject = accService.getAccessibleFor(this.mSubject);
>+    } catch(e) {
>+      dump("Failed to get accessible object for node.");
>+      this.emptyTree();

You should clean up all fields, not only the attributes tree.

>+    var attrs = this.mAccSubject.attributes;
>+    if (attrs) {
>+      var enumerate = attrs.enumerate();
>+      while (enumerate.hasMoreElements()) {
>+          this.addAttr(enumerate.getNext());
>+      }
>+    } else {
>+      dump("Attributes is not available.\n");

it's possible nsIAccessible::GetAttributes() may throw an exception but it's ok if accessible has not attributes thereofre you shouldn't distinguish the cases when attrs is null and when attrs length is 0.

>+    }

>+  },
>+
>+  addAttr: function addAttr(element) {

Probably it's better to name the method like 'addARIAAttribute' or something similar.

>+    try {
>+      var prop = element.QueryInterface(Components.interfaces.nsIPropertyElement);
>+      var trAttrBody = document.getElementById("trAttrBody");
>+      var ti = document.createElement("treeitem");
>+      var tr = document.createElement("treerow");
>+      var tc = document.createElement("treecell");
>+      tc.setAttribute("label", prop.key);
>+      tr.appendChild(tc);
>+      tc = document.createElement("treecell");
>+      tc.setAttribute("label", prop.value);
>+      tr.appendChild(tc);
>+      ti.appendChild(tr);
>+      trAttrBody.appendChild(ti);
>+    } catch(e) {
>+      dump(e+"\n");

What can fail here?

>+    }
>+  },
>+  
>+  emptyTree: function emptyTree() {

Probably 'removeARIAAttributes'?

>+   - Contributor(s):
>+   -  Alexander Surkov <[e-mail]> (original author)

Feel free to put your name in files.

>+  <tree flex="1">
>+    <treecols>
>+      <treecol label="attrKey" flex="1"/>

I'm sure timeless will say you must use DTD here. So you can fix it now.

Comment 12

[Vasiliy Potapenko]

Attachment: patch

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 270846 [details] [diff] [review]
patch

r=me for accessibility part with the folowing addressed comments.

>+  // private
>+  updateView: function updateView() {
>+    try {
>+      this.mAccSubject = accService.getAccessibleFor(this.mSubject);
>+    } catch(e) {
>+      dump("Failed to get accessible object for node.");
>+      this.clearView();
>+      return;
>+    }
>+    
>+    this.clearView();

You can call once clearView() before try/catch statement.

>+  addARIAAttribute: function addARIAAttribute(element) {

nit: arguemnts should have prefix 'a' like aElement.

Comment 14

[Vasiliy Potapenko]

Attachment: screen

Comment 15

[Vasiliy Potapenko]

Attachment: patch

Comment 16

[Shawn Wilsher :sdwilsh]

Comment on attachment 270849 [details] [diff] [review]
patch

>+              Components.classes['@mozilla.org/accessibleRetrieval;1']
nit: " instead of '

>+//////////// global constants ////////////////////
>+
>+var nsIAccessible = Components.interfaces.nsIAccessible;
if it's a const, use the const keyword please

>+function AccessiblePropsViewer_initialize()
>+{
>+  accService = Components.classes['@mozilla.org/accessibleRetrieval;1']
nit: " instead of '

>+  <script type="application/x-javascript"
application/javascript


>+        <description>Role: </description>
localize

>+        <description>Name: </description>
localize

>+        <description>Description: </description>
localize

>+        <description>Value: </description>
localize

>+        <description>State: </description>
localize

>+        <description>Action names: </description>
localize

I'll do a more in-depth review with these fixed.

Comment 17

[Vasiliy Potapenko]

Attachment: patch

Comment 18

[Shawn Wilsher :sdwilsh]

Comment on attachment 270999 [details] [diff] [review]
patch

global nit: please have { on a newline after the function header (this includes getters and setters, unless you have them entirely on one line).  eg:
foo: function foo(aParam)
{
}

>+/***************************************************************
>+* AccessiblePropsViewer --------------------------------------------
>+*  The viewer for the accessible object properties.
>+* - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
>+* REQUIRED IMPORTS:
>+*   chrome://inspector/content/jsutil/events/ObserverManager.js
>+****************************************************************/
nothing you have to worry about - but we should really start using Components.utils.import in DOMi...

>+//////////// global variables /////////////////////
nit: so I know older files are like his, but I find this way to be a lot cleaner (and would be the way I'd like it done in DOMi from here on out:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/inspector/resources/content/viewers/domNode/domNodeDialog.js&rev=1.1#38

>+
>+var viewer;
>+var accService;
>+
>+//////////// global constants ////////////////////
>+
>+const nsIAccessible = Components.interfaces.nsIAccessible;
I'm also very much OK (in fact, encouraging) you to declare Ci and Cc and use them to help with line wrapping in this code.

>+  accService = Components.classes["@mozilla.org/accessibleRetrieval;1"]
>+                         .getService(Components.interfaces.nsIAccessibleRetrieval);
Is there any reason you don't make this a member of AccessiblePropsViewer?  I think it'd be a bit cleaner there.

>+function AccessiblePropsViewer()
>+{
>+  this.mURL = window.location;
>+  this.mObsMan = new ObserverManager(this);
>+}
nit: add heading above this with comment indicating that the "class" is being defined here (see previous link)

>+AccessiblePropsViewer.prototype =
>+{
>+  mSubject: null,
>+  mPane: null,
>+  mAccSubject: null,
>+
>+  // inIViewer interface
nit:
  ////////////////...
  //// inIViewer
(this is true for all sections of the "class", but I'll stop pointing it out)

>+      while (enumerate.hasMoreElements()) {
>+          this.addARIAAttribute(enumerate.getNext());
>+      }
nit: too much indent, and no braces for one line while statement


>+  addARIAAttribute: function addARIAAttribute(aElement) {
Could you add a javadoc header on this function explaining it a bit?  Or maybe you just need to clarify it a bit because I'm not even user how to read it because there are so many capital letters there.

>+  removeARIAAttributes: function removeARIAAttributes() {
ditto

>+  get actionNames() {
>+    var list = [];
>+
>+    var count = this.mAccSubject.numActions;
>+    for (var i = 0; i < count; i++) {
>+      list.push(this.mAccSubject.getActionName(i));
>+    }
>+    return list;
>+  }
>+}
nit: semicolon needed after the prototype


>+<?xml-stylesheet href="chrome://inspector/skin"?>
need a / after skin

>+        <description>&descRole.label;:</description>
colon should be part of the locale strings (rtl vs ltr it would matter).  In some you add a space, and I don't see why that is necessary.

>+* locale/@AB_CD@/inspector/viewers/accessibleProps.dtd(@AB_CD@/viewers/accessibleProps.dtd)
hrm, does that ( actually line up with the rest?  If it does, we should consider re-aligning the rest.


>Index: extensions/inspector/resources/locale/ca/viewers/accessibleProps.dtd
>+   - The Initial Developer of the Original Code is
>+   - Netscape Communications Corporation.
?  I think this is you ;)

As for the rest of the locales - you only need to update the ones that are currently building in the Makefile (http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/inspector/resources/Makefile.in&rev=1.37#48), and file follow-up bugs with each localization team to get those updated.

Go ahead and request sr for your next version as I don't expect to have any additional comments.

Comment 19

[Vasiliy Potapenko]

Attachment: patch

Comment 20

[[e-mail]]

Comment on attachment 271482 [details] [diff] [review]
patch

>+  this.mAccService = Components.classes["@mozilla.org/accessibleRetrieval;1"]
>+                         .getService(Components.interfaces.nsIAccessibleRetrieval);
I suppose lining up the .s would break the 80-column barrier?

Comment 21

[[e-mail]]

Comment on attachment 271482 [details] [diff] [review]
patch

>+    var containers = document.getElementsByAttribute("class", "accessibleProp");
Is this the only reason you use that class? I'd prefer if you used document.getElementsByAttribute("prop", "*"); that way you don't have to null-check prop.

Comment 22

[Vasiliy Potapenko]

Attachment: patch

I have made corrections.

Comment 23

[Shawn Wilsher :sdwilsh]

I'd check this in, but the patch doesn't apply cleanly (as a result of Bug 378696).  Please get an updated version, and I'll gladly check this in.

Comment 24

[alexander :surkov (:asurkov)]

   - The Initial Developer of the Original Code is
   - Vasiliy Potapenko <[e-mail]>

Specify the email in contributors section.

   - Portions created by the Initial Developer are Copyright (C) 2003

2003?

   - the Initial Developer. All Rights Reserved.
   -
   - Contributor(s):

Write yourself here.
   -

Comment 25

[Vasiliy Potapenko]

Attachment: patch

Updated version.

Comment 26

[Vasiliy Potapenko]

Attachment: patch

I have corrected license block

Comment 27

[alexander :surkov (:asurkov)]

Comment on attachment 271638 [details] [diff] [review]
patch

checked in for vasiliy