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?

updated patch

Comment on attachment 273602 [details] [diff] [review] patch Looks okay to me.

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.)

(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?

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. :)

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.

(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.

fixed Neil's, Shawn's comments, updated to trunk. Needs approval for accessibility changes.

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.

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. :)

checked in