Closed Bug 347792 Opened 14 years ago Closed 13 years ago

Expose accessible relations in DOM Inspector

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: surkov)

References

Details

(Keywords: access)

Attachments

(2 files, 2 obsolete files)

Since GetAccessibleRelated is a method and not an attribute, we cannot see the results in DOM Inspector. How can we change DOM Inspector so that the relations are exposed in a meaningful way?
Assignee: dom-inspector → nobody
QA Contact: timeless → dom-inspector
Blocks: a11yDOMi
Attached image screenshot
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
Attachment #273601 - Flags: review?(sdwilsh)
Attachment #273601 - Flags: review?(ajvincent)
Attached patch patch (obsolete) — Splinter Review
updated patch
Attachment #273601 - Attachment is obsolete: true
Attachment #273602 - Flags: review?(sdwilsh)
Attachment #273601 - Flags: review?(sdwilsh)
Attachment #273601 - Flags: review?(ajvincent)
Attachment #273602 - Flags: review?(ajvincent)
Comment on attachment 273602 [details] [diff] [review]
patch

Looks okay to me.
Attachment #273602 - Flags: review?(ajvincent) → review+
Attachment #273602 - Flags: review?(Evan.Yan)
Attachment #273602 - Flags: superreview?(neil)
Comment on attachment 273602 [details] [diff] [review]
patch

>-  for (PRUint32 relType = 0; relType < 0x0f; ++relType) {
>+  for (PRUint32 relType = 0x01; relType < 0x0f; ++relType) {
I assume you know what you're doing here.

>+  cmdInspectInNewView: function cmdInspectInNewView()
>+  {
>+    var idx = this.mTargetsTree.currentIndex;
>+    var node = this.mTargetsView.getDOMNode(idx);
>+    if (node)
>+      inspectObject(node);
>+  }
What happens if the tree is empty? (I was going to ask what happens if nothing is selected but I guess the toolkit tree doesn't let you get away with that any more.)
Attachment #273602 - Flags: superreview?(neil) → superreview+
(In reply to comment #5)
> (From update of attachment 273602 [details] [diff] [review])
> >-  for (PRUint32 relType = 0; relType < 0x0f; ++relType) {
> >+  for (PRUint32 relType = 0x01; relType < 0x0f; ++relType) {
> I assume you know what you're doing here.

Yes, Evan will check :). There is no relType=0 relation though relation constant exists.
Comment on attachment 273602 [details] [diff] [review]
patch

I just did a quick skimming, so I would like to see this once more.  Sorry for the delay on this.

Please be sure to address Neil's second comment.

>+  "description for",     // RELATION_DESCRIPTION_FOR
>+  "default button"      // RELATION_DEFAULT_BUTTON
nit: (may not be my area, but I'm still commenting :p) line up // please

>+AccessibleRelationsViewer.prototype =
>+{
>+  // initialization
I'd prefer these to be the full comment blocks - it's easier to find them when skimming the file
  /////////////////////////
  //// initialization

>+  <popup id="popupContext">
shouldn't this be a menupopup?
Attachment #273602 - Flags: review?(sdwilsh) → review-
Comment on attachment 273602 [details] [diff] [review]
patch

OK, I looked closer (I managed to get time today), and this is fine and I don't need to see it again.  Just address my previous comments. :)
Attachment #273602 - Flags: review- → review+
Comment on attachment 273602 [details] [diff] [review]
patch

>   // Latest nsIAccessibleRelation is RELATION_DESCRIPTION_FOR (0xof)
there is a typo (0xof), please correct it since you're here.

>-  for (PRUint32 relType = 0; relType < 0x0f; ++relType) {
>+  for (PRUint32 relType = 0x01; relType < 0x0f; ++relType) {
>     nsCOMPtr<nsIAccessible> accessible;
>     nsresult rv = GetAccessibleRelated(relType, getter_AddRefs(accessible));
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    if (accessible) {
>+    if (NS_SUCCEEDED(rv) && accessible) {
this change makes it always return NS_OK even rv is FAILED, isn't it?

>+//// AccessibleRelationsView
>+
>+function AccessibleRelationsView(aNode)
>+{
>+  this.mNode = aNode;
>+
>+  this.mAccessible = gAccService.getAccessibleFor(aNode);
>+  this.mRelations = this.mAccessible.getRelations();
What if no accessible for the node?
Comment on attachment 273602 [details] [diff] [review]
patch

clearing request until questions' answered.
Attachment #273602 - Flags: review?(Evan.Yan)
(In reply to comment #9)
> (From update of attachment 273602 [details] [diff] [review])
> >   // Latest nsIAccessibleRelation is RELATION_DESCRIPTION_FOR (0xof)
> there is a typo (0xof), please correct it since you're here.
> 
> >-  for (PRUint32 relType = 0; relType < 0x0f; ++relType) {
> >+  for (PRUint32 relType = 0x01; relType < 0x0f; ++relType) {
> >     nsCOMPtr<nsIAccessible> accessible;
> >     nsresult rv = GetAccessibleRelated(relType, getter_AddRefs(accessible));
> >-    NS_ENSURE_SUCCESS(rv, rv);
> >-
> >-    if (accessible) {
> >+    if (NS_SUCCEEDED(rv) && accessible) {
> this change makes it always return NS_OK even rv is FAILED, isn't it?

Yes because I guess we shouldn't fail if one relation is failed.


> >+//// AccessibleRelationsView
> >+
> >+function AccessibleRelationsView(aNode)
> >+{
> >+  this.mNode = aNode;
> >+
> >+  this.mAccessible = gAccService.getAccessibleFor(aNode);
> >+  this.mRelations = this.mAccessible.getRelations();
> What if no accessible for the node?
> 

An exception :). But it's impossible, this code will never be executed because this view won't be enabled if there is no accessible.
ok, then r=me
Comment on attachment 273602 [details] [diff] [review]
patch

I need an approval for ally part.
Attachment #273602 - Flags: approval1.9?
Attached patch patch3Splinter Review
fixed Neil's, Shawn's comments, updated to trunk. Needs approval for accessibility changes.
Attachment #273602 - Attachment is obsolete: true
Attachment #277852 - Flags: approval1.9?
Attachment #273602 - Flags: approval1.9?
Damon, Schrep, could we get an approval1.9 decision here? Poor Alexander has been waiting patiently since August 22nd, because the approval process is apparently broken, and nobody is loading https://bugzilla.mozilla.org/request.cgi?type=approval1.9 occasionally to see what's falling through the cracks.

Alexander: if they don't read bugmail, and nothing happens, you can game the system by just changing the product/component to Core: Accessibility APIs - I did that for a DOMi bug after waiting a week or so, and got approval the next day.
Move to 'disability access api' component as Phil suggested.
Component: DOM Inspector → Disability Access APIs
Product: Other Applications → Core
OK.  I need to ask around about this bug, but I'll get you an answer/approval within 24 hours.  
Should aaronlev review this?
(In reply to comment #18)
> Should aaronlev review this?
> 

Evan gave me r+, see comment #12. I guess he's just forgot to set proper flag.
Comment on attachment 277852 [details] [diff] [review]
patch3

OK.  Sorry for the wait.  :)
Attachment #277852 - Flags: approval1.9? → approval1.9+
checked in
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.