746111 - Accessible object viewers should be available unconditionally for Accessible Tree viewer

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

the problem, there are accessibles like html:area accessibles that can't be obtained via nsIAccessibleRetieval::getAccessibleFor(areaNode).

Comment 1

[[e-mail]]

Please can you attach an example document exhibiting the problem?

Comment 2

[alexander :surkov (:asurkov)]

Attachment: test

try any area accessible

Comment 3

[alexander :surkov (:asurkov)]

Attachment: letters.gif

image for imagemap of test

Comment 4

[[e-mail]]

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

Sorry for the delay.

>+          // Retrun true unconditionally since accessibleTree view can't show
>+          // inaccessible DOM node.
>+          if (linkedViewer.uid == "accessibleTree")
>+            return true;
I had a look at this and it appears that it only works because every time you select a DOM node in the accessible tree you update its " accessible " property so that the accessible object viewer can find the accessible object.

So that people know what's going on, I suggest you either modify the code to look for the property directly or modify your comment to indicate that this is how the viewer manages to find the correct accessible object.

While you're modifying your comment, you misspelled "Return" ;-)

Comment 5

[alexander :surkov (:asurkov)]

I don't recall if every view sets up " accessible " property, maybe comment would be ok

Comment 6

[[e-mail]]

(In reply to alexander surkov from comment #5)
> I don't recall if every view sets up " accessible " property, maybe comment
> would be ok
I didn't say that every view did, I was just pointing out that it's not obvious that that's how it works in this case at least.

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch2

apparently [" accessible "] approach doesn't work anymore.

Comment 8

[[e-mail]]

(In reply to alexander :surkov from comment #7)
> apparently [" accessible "] approach doesn't work anymore.
Might have been broken by compartment-per-global, perhaps. (Other stuff was.)

Comment 9

[[e-mail]]

Comment on attachment 629099 [details] [diff] [review]
patch2

>+          return object instanceof Components.interfaces.nsIDOMElement ||
>+            "@mozilla.org/accessibleRetrieval;1" in Components.classes &&
>+            object instanceof Components.interfaces.nsIAccessible &&
>+            object.DOMNode instanceof Components.interfaces.nsIDOMElement;
I wonder whether it's possible to tweak ViewerRegistry so we don't have to repeat this all the time; when you first added accessibility support you added a linkedViewer variable to the filter, you could add another variable, although I'm not sure what would work best. For instance, if you added an accessibleNode variable, then the above would become return (accessibleNode || object) instanceof Components.interfaces.nsIDOMElement;

>+                  getBindingURLs(object || object.DOMNode);
This won't work because "object" is always a truthy-value, whether it's an element or an accessible object. (And of course at this point we've forgotten whether accessibility is enabled.)

>+      var accObject = (aObject instanceof nsIAccessible) ?
>+        aObject : accService.getAccessibleFor(aObject);
Nit: ()s not required around instanceof here.

>+  return accessNode || accessNode.DOMNode;
Not sure what this is trying to achieve, see above.

>+  return node ? QIAccessNode(node.accessible) : null;
return node && QIAccessNode(node.accessible);

>+    this.mSubject = aObject instanceof Node ? aObject : aObject.DOMNode;
Mostly we use Components.interfaces.nsIDOMNode in DOM Inspector.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to [e-mail] from comment #9)
> Comment on attachment 629099 [details] [diff] [review]
> patch2
> 
> >+          return object instanceof Components.interfaces.nsIDOMElement ||
> >+            "@mozilla.org/accessibleRetrieval;1" in Components.classes &&
> >+            object instanceof Components.interfaces.nsIAccessible &&
> >+            object.DOMNode instanceof Components.interfaces.nsIDOMElement;
> I wonder whether it's possible to tweak ViewerRegistry so we don't have to
> repeat this all the time; when you first added accessibility support you
> added a linkedViewer variable to the filter, you could add another variable,
> although I'm not sure what would work best. For instance, if you added an
> accessibleNode variable, then the above would become return (accessibleNode
> || object) instanceof Components.interfaces.nsIDOMElement;

it's be really nice but it makes the accessible object away from being an object, i.e. we have an object and an accessible object (theoretically not null accessible object may be used with null object), thus we can't say the accessible object is for the object.

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 12

[[e-mail]]

Comment on attachment 629137 [details] [diff] [review]
patch3

This works great, except for one minor issue, and that is when you have an accessible panel open on the left, then the JavaScript Object and Accessible Object viewers both display the same thing. (I don't know what's right here.)

>+          var node =
>+            object instanceof Components.interfaces.nsIDOMElement && object ||
>+            "@mozilla.org/accessibleRetrieval;1" in Components.classes &&
>+            object instanceof Components.interfaces.nsIAccessible &&
>+            object.DOMNode instanceof Components.interfaces.nsIDOMElement &&
>+            object.DOMNode;
This might be more readable the other way around i.e.
var node = "@mozilla.org/accessibleRetrieval;1" in Components.classes &&
           object instanceof Components.interfaces.nsIAccessible ?
           object.DOMNode : object;
if (node instanceof Components.interfaces.nsIDOMElement) {
  ...
}

Comment 13

[alexander :surkov (:asurkov)]

(In reply to [e-mail] from comment #12)
> Comment on attachment 629137 [details] [diff] [review]
> patch3
> 
> This works great, except for one minor issue, and that is when you have an
> accessible panel open on the left, then the JavaScript Object and Accessible
> Object viewers both display the same thing. (I don't know what's right here.)

previously JS object shown DOM node, Accessible Object shown accessible object. After this patch I think we can get rid Accessible Object viewer at all.

Comment 14

[[e-mail]]

How would you show the accessible object if you were viewing a DOM Node?

Comment 15

[alexander :surkov (:asurkov)]

(In reply to [e-mail] from comment #14)
> How would you show the accessible object if you were viewing a DOM Node?

good point but personally I didn't use js object view for accessible object (except the beginning of times when it was unique available thing :) ). Maybe it's not important after all. Keeping them both when viewing accessible tree is a dupe. So we could keep acc obj viewer for DOM tree viewers.

Comment 16

[alexander :surkov (:asurkov)]

Attachment: patch4

make accobj view working

Comment 17

[[e-mail]]

Comment on attachment 629147 [details] [diff] [review]
patch4

>   setSubject: function JSOVr_SetSubject(aObject)
>   {
>-    aObject = this.unwrapObject(aObject);
>-    this.mSubject = aObject;
>-    this.mView = new JSObjectView(aObject);
>+    this.mSubject = this.unwrapObject(aObject);
>+    this.mView = new JSObjectView(this.mSubject);
>     this.mTree.view = this.mView;
> 
>-    this.mObsMan.dispatchEvent("subjectChange", { subject: aObject });
>+    this.mObsMan.dispatchEvent("subjectChange", { subject: this.mSubject });
[Not sure what the point of this change is.]

Comment 18

[alexander :surkov (:asurkov)]

(In reply to [e-mail] from comment #17)

> >-    this.mObsMan.dispatchEvent("subjectChange", { subject: aObject });
> >+    this.mObsMan.dispatchEvent("subjectChange", { subject: this.mSubject });
> [Not sure what the point of this change is.]

just to be consistent with other viewers.

Comment 19

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/dom-inspector/rev/d96d6863fe08

Comment 20

[Colby Russell :crussell (no longer Mozilla-ing)]

I don't like this.  It complicates the panel and viewer interfaces.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Colby Russell :crussell from comment #20)
> I don't like this.  It complicates the panel and viewer interfaces.

please open a new bug?

Comment 22

[[e-mail]]

Comment on attachment 629147 [details] [diff] [review]
patch4

>+    <rdf:li>
>+      <rdf:Description
>+        ins:uid="styleRules"
>+        ins:panels="bxObjectPanel bxObjPanel"
>+        ins:description="&styleRules.label;">
>+      <ins:filter><![CDATA[
>+        return object instanceof Components.interfaces.nsIDOMElement ||
>+          object instanceof Components.interfaces.nsIDOMCSSStyleSheet ||
>+          "@mozilla.org/accessibleRetrieval;1" in Components.classes &&
>+          object instanceof Components.interfaces.nsIAccessible &&
>+          object.DOMNode instanceof Components.interfaces.nsIDOMElement;
>+      ]]></ins:filter>
>+      </rdf:Description>
Oops, styleRules accepts nsIDOMCSSStyleSheet...

>diff --git a/resources/content/viewers/styleRules/styleRules.js b/resources/content/viewers/styleRules/styleRules.js
>--- a/resources/content/viewers/styleRules/styleRules.js
>+++ b/resources/content/viewers/styleRules/styleRules.js
>@@ -83,27 +83,28 @@ StyleRulesViewer.prototype =
> 
>   get subject()
>   {
>     return this.mSubject
>   },
> 
>   set subject(aObject)
>   {
>-    this.mSubject = aObject;
>+    this.mSubject = aObject instanceof Components.interfaces.nsIDOMNode ?
>+      aObject : aObject.DOMNode;
...but we fail to check for that here :-(

Comment 23

[alexander :surkov (:asurkov)]

Attachment: followup

Comment 24

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/dom-inspector/rev/30db0aec22c8