Closed
Bug 347791
Opened 18 years ago
Closed 17 years ago
Expose accessible role and state as strings in Inspector
Categories
(Other Applications :: DOM Inspector, defect)
Other Applications
DOM Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9beta1
People
(Reporter: aaronlev, Assigned: vasiliy.potapenko)
References
Details
(Keywords: access)
Attachments
(3 files, 7 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
image/jpeg
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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•18 years ago
|
||
(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?
Reporter | ||
Comment 2•18 years ago
|
||
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•18 years ago
|
||
Patch doesn't contain any stlyles and locales.
Comment 4•18 years ago
|
||
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).
Assignee: dom-inspector → vasiliy.potapenko
Reporter | ||
Comment 5•18 years ago
|
||
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•18 years ago
|
||
(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?
Reporter | ||
Comment 7•18 years ago
|
||
I'm not sure. How does JS know the name?
Comment 8•18 years ago
|
||
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•18 years ago
|
||
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.
Assignee | ||
Comment 10•17 years ago
|
||
Attachment #270702 -
Flags: review?(surkov.alexander)
Comment 11•17 years ago
|
||
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 <surkov.alexander@gmail.com> (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.
Attachment #270702 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 12•17 years ago
|
||
Attachment #270702 -
Attachment is obsolete: true
Attachment #270846 -
Flags: review?(surkov.alexander)
Comment 13•17 years ago
|
||
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.
Attachment #270846 -
Flags: superreview?(neil)
Attachment #270846 -
Flags: review?(timeless)
Attachment #270846 -
Flags: review?(surkov.alexander)
Attachment #270846 -
Flags: review+
Assignee | ||
Comment 14•17 years ago
|
||
Assignee | ||
Comment 15•17 years ago
|
||
Attachment #270846 -
Attachment is obsolete: true
Attachment #270849 -
Flags: superreview?(neil)
Attachment #270849 -
Flags: review?(timeless)
Attachment #270846 -
Flags: superreview?(neil)
Attachment #270846 -
Flags: review?(timeless)
Comment 16•17 years ago
|
||
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.
Attachment #270849 -
Flags: review?(timeless) → review-
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 17•17 years ago
|
||
Attachment #270849 -
Attachment is obsolete: true
Attachment #270999 -
Flags: review?(sdwilsh)
Attachment #270849 -
Flags: superreview?(neil)
Updated•17 years ago
|
Hardware: PC → All
Target Milestone: --- → mozilla1.9beta
Version: unspecified → Trunk
Comment 18•17 years ago
|
||
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.
Attachment #270999 -
Flags: review?(sdwilsh) → review-
Updated•17 years ago
|
QA Contact: timeless → dom-inspector
Assignee | ||
Comment 19•17 years ago
|
||
Attachment #270999 -
Attachment is obsolete: true
Attachment #271482 -
Flags: superreview?(neil)
Comment 20•17 years ago
|
||
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?
Attachment #271482 -
Flags: superreview?(neil) → superreview+
Comment 21•17 years ago
|
||
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.
Assignee | ||
Comment 22•17 years ago
|
||
I have made corrections.
Attachment #271482 -
Attachment is obsolete: true
Comment 23•17 years ago
|
||
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•17 years ago
|
||
- The Initial Developer of the Original Code is
- Vasiliy Potapenko <vasiliy.potapenko@gmail.com>
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.
-
Assignee | ||
Comment 25•17 years ago
|
||
Updated version.
Attachment #271608 -
Attachment is obsolete: true
Assignee | ||
Comment 26•17 years ago
|
||
I have corrected license block
Attachment #271636 -
Attachment is obsolete: true
Comment 27•17 years ago
|
||
Comment on attachment 271638 [details] [diff] [review]
patch
checked in for vasiliy
Updated•17 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•