Now we use DOM tree and then look wheter certain DOM node can have an accessible object. But accessible object tree and DOM tree may be a different. So we need to have separate view to show accessible tree.

add AccessibleTree view for the right panel.

Comment on attachment 272626 [details] [diff] [review] patch >Index: extensions/inspector/resources/content/viewers/accessibleTree/accessibleTree.js >+inAccTreeView.prototype.isContainer = >+function isContainer(aRow) >+{ >+ var node = this.rowToNode(aRow); >+ if (!node) >+ return false; >+ >+ return node.isContainer; >+} Nit: I'd rewrite this as: function isContainer(aRow) { var node = this.rowToNode(aRow); return node ? node.isContainer : false; } Ditto for a few similar functions here. >+inAccTreeView.prototype.expandNode = >+function expandNode(aRow) >+{ >+ //inspectObject(this.mNodes, true); >+ for (var i = 0; i < kidCount; ++i) { >+ var accessible = kids.queryElementAt(i, nsIAccessible); >+ newNode = this.createNode(accessible, node); >+ this.mNodes.splice(aRow + i + 1, 0, newNode); >+ //inspectObject(this.mNodes, true); Why commented out? >+inAccTreeView.prototype.getLastDescendantOf = >+function getLastDescendantOf(aNode, aRow) >+{ >+ return row-1; row - 1, please (spacing) >+inAccTreeView.prototype.rowToNode = >+function rowToNode(aRow, aNode) Stupid question: is this method defined by an interface, or is it a utility method? In the former case, would you be willing to provide a one-line comment on the method stating where it comes for ("// nsIFoo")? In the latter case, would you be willing to JavaDoc this method? The same applies to your other functions in here. I can get away with suggesting this because it's a new file. :-) >Index: extensions/inspector/resources/locale/en-US/viewers/accessibleTree.dtd >+<!-- ***** BEGIN LICENSE BLOCK ***** >+#if 0 ... >+#endif >+ - ***** END LICENSE BLOCK ***** --> I'm no fan of XUL preprocessing, but the fact is you're leaving these "BEGIN LICENSING BLOCK. END LICENSING BLOCK" lines in the file - which frankly doesn't make much sense. Also, shouldn't this file be added to extensions/inspector/resources/locale/jar.mn ? These comments are not intended as a review. Nonetheless, the patch looks quite good to me.

(In reply to comment #2) > Nit: I'd rewrite this as: nice, fixed. > Why commented out? my debug code :), removed > >+inAccTreeView.prototype.getLastDescendantOf = > >+function getLastDescendantOf(aNode, aRow) > >+{ > >+ return row-1; > > row - 1, please (spacing) > > >+inAccTreeView.prototype.rowToNode = > >+function rowToNode(aRow, aNode) > > Stupid question: is this method defined by an interface, or is it a utility > method? utils one. In the former case, would you be willing to provide a one-line comment > on the method stating where it comes for ("// nsIFoo")? In the latter case, > would you be willing to JavaDoc this method? I added javadoc styled comments for utils methods. > I'm no fan of XUL preprocessing, but the fact is you're leaving these "BEGIN > LICENSING BLOCK. END LICENSING BLOCK" lines in the file - which frankly > doesn't make much sense. I'm not too. Just copy/paste. I removed this. > Also, shouldn't this file be added to > extensions/inspector/resources/locale/jar.mn ? you;re right. > These comments are not intended as a review. Nonetheless, the patch looks > quite good to me. > thank you for your comments and an attention to the bug.

Aaron, need r for ally part.

fix the collapse problem

Comment on attachment 273077 [details] [diff] [review] patch3 Can we see some screenshots of the UI please? >+ <rdf:Description ins:uid="accessibleTree" >+ ins:panels="bxDocPanel" >+ ins:description="Accessible Tree"> >+ <ins:filter> >+ <![CDATA[ >+ return object instanceof Components.interfaces.nsIDOMDocument; >+ ]]> >+ </ins:filter> nit: <![CDATA[ after <ins:filter>... >+// global variables nit: comment style (global) >+// nsITreeView interface of inAccTreeView I don't understand this comment >+inAccTreeView.prototype.__defineGetter__("rowCount", >+function() please name this function >+// tree utils of inAccTreeView again, this comment is not totally clear to me >+ >+/** >+ * Expands a tree node on the given row. >+ */ If you have javadoc style comments, please have the proper @param and @return bits. I'd greatly appreciate it if you did this for all your functions (that aren't defined in an idl) >+inAccTreeView.prototype.getLastDescendantOf = >+function getLastDescendantOf(aNode, aRow) >+{ >+ var rowCount = this.rowCount; >+ for (var row = aRow + 1; row < rowCount; ++row) { >+ if (this.mNodes[row].level <= aNode.level) >+ break; >+ } >+ >+ return row; >+} Er, so I know that JS lets you do this, but I find it to be confusing to read. Please declare row outside of the loop.

Comment on attachment 273077 [details] [diff] [review] patch3 >Index: extensions/inspector/resources/content/viewers/accessibleTree/accessibleTree.js >+inAccTreeView.prototype.getLastDescendantOf = >+function getLastDescendantOf(aNode, aRow) >+{ >+ var rowCount = this.rowCount; >+ for (var row = aRow + 1; row < rowCount; ++row) { >+ if (this.mNodes[row].level <= aNode.level) >+ break; >+ } >+ >+ return row; >+} Why not replace that break with a |return row| there? Also, do you want row to equal rowCount if the loop isn't broken, and return that value? r=ajvincent with sdwilsh's nits and this one answered. (By answered, I don't mean you have to do what I say, just consider it.) I haven't tested this patch, though, and I reserve the right to retract r+ if it turns out to be horribly broken. I also should ask a higher-level question about DOM Inspector accessibility in general: what happens if the developer has set --disable-accessibility in their MOZCONFIG? It sounds like we'd have at least three different accessibility panels that would be broken. I think it's worth investigating, and if necessary, filing a new bug to remove them by XUL preprocessing on the jar.mn file.

with Shawn and Alex comments

(In reply to comment #6) > >+// global variables > nit: comment style (global) > > >+// nsITreeView interface of inAccTreeView > I don't understand this comment Shawn, please check whether comments are fine with you now.

(In reply to comment #8) > (From update of attachment 273077 [details] [diff] [review]) > Why not replace that break with a |return row| there? fixed. > Also, do you want row to > equal rowCount if the loop isn't broken, and return that value? I return aRow. I believe it shouldn't happen but if it will then it won't broke us now. > I also should ask a higher-level question about DOM Inspector accessibility in > general: what happens if the developer has set --disable-accessibility in > their MOZCONFIG? It sounds like we'd have at least three different > accessibility panels that would be broken. I think it's worth investigating, > and if necessary, filing a new bug to remove them by XUL preprocessing on the > jar.mn file. > It will be fine. nsIAccessibleRetrieval service won't be available so that ally views won't be loaded.

Comment on attachment 273212 [details] [diff] [review] patch4 >+/** >+ * Create a tree node. >+ * >+ * @param aAccessible - an accessible object associated with created tree node. >+ * @param aParent - parent tree node for the created tree node. >+ */ >+inAccTreeView.prototype.createNode = >+function createNode(aAccessible, aParent) >+{ >+ var node = new inAccTreeViewNode(aAccessible); >+ node.level = aParent ? aParent.level + 1 : 0; >+ node.parent = aParent; >+ node.isContainer = aAccessible.children.length > 0; >+ >+ return node; >+} Nit: You missed the @return lines on these. See comment 6. (Don't submit a new patch for just that, though.)

(In reply to comment #11) > It will be fine. nsIAccessibleRetrieval service won't be available so that ally > views won't be loaded. Let me rephrase: will the three panels be built with DOM Inspector? Will they be reachable via the panel selector?

(In reply to comment #13) > (In reply to comment #11) > > It will be fine. nsIAccessibleRetrieval service won't be available so that ally > > views won't be loaded. > > Let me rephrase: will the three panels be built with DOM Inspector? Yes. > Will they > be reachable via the panel selector? > They won't.

(In reply to comment #12) > Nit: You missed the @return lines on these. See comment 6. (Don't submit a > new patch for just that, though.) > Ok, I'll fix this after another reviews :)

(In reply to comment #2) >>Index: extensions/inspector/resources/locale/en-US/viewers/accessibleTree.dtd >>+<!-- ***** BEGIN LICENSE BLOCK ***** >>+#if 0 >... >>+#endif >>+ - ***** END LICENSE BLOCK ***** --> >I'm no fan of XUL preprocessing, but the fact is you're leaving these "BEGIN >LICENSING BLOCK. END LICENSING BLOCK" lines in the file - which frankly >doesn't make much sense. It makes perfect sense to me. It means you can load the file into a generic .dtd editor which has no knowledge of preprocessing. It also means that I can kill the * from my jar.mn to speed up my build :-P

Odd, I can't get this to show up in my build... I'll try a full build tonight.

(In reply to comment #16) > It makes perfect sense to me. It means you can load the file into a generic > .dtd editor which has no knowledge of preprocessing. It also means that I can > kill the * from my jar.mn to speed up my build :-P > So should I use preprocessing? No?

(In reply to comment #18) > So should I use preprocessing? No? You should if you aren't already, yes.

(In reply to comment #19) >(In reply to comment #18) >>So should I use preprocessing? No? >You should if you aren't already, yes. Well, there are three angles to the preprocessing camp. The first angle, which is typified by people like Shawn, is that you must preprocess absolutely everything because it's right to slow your build down by hours so that you don't waste microseconds at run time skipping licence blocks. The second angle is that you should use the preprocessor but wherever possible make it possible to use the original file unpreprocessed (which is what you have done here by only preprocessing inside the comment). The third angle, which is my preference, is that you shouldn't preprocess unless absolutely necessary as then you can build with symlinked chrome (at least on non-Windows platforms) which allows you to develop more quickly.

I like the third angle and I like it more because I don't need to change anything in my patch but I guess I'm not right person to take a decision.

(In reply to comment #20) > The first angle, which is typified by people like Shawn, is that you must Not true! I prefer to not preprocess, but all other locale files are, so I'm saying that to follow suite!

(In reply to comment #22) >(In reply to comment #20) >>The first angle, which is typified by people like Shawn, is that you must >Not true! I prefer to not preprocess, but all other locale files are, so I'm >saying that to follow suite! My apologies, but you're definitely not following suite :-P (In reply to comment #21) >I like the third angle and I like it more because I don't need to change >anything in my patch but I guess I'm not right person to take a decision. Shawn is right, you should follow suit with Axel and his preprocessing style.

(In reply to comment #23) > (In reply to comment #21) > >I like the third angle and I like it more because I don't need to change > >anything in my patch but I guess I'm not right person to take a decision. > Shawn is right, you should follow suit with Axel and his preprocessing style. > Ok. Let me fix this within following up comments.

Comment on attachment 273212 [details] [diff] [review] patch4 Some of your functions don't look quite right: >+inAccTreeView.prototype.getParentIndex = >+function getParentIndex(aRow) >+{ >+ var node = this.rowToNode(aRow); >+ if (!node) >+ return -1; >+ >+ var checkNode = null; >+ var i = aRow - 1; >+ do { >+ var node = this.rowToNode(i); >+ if (!node) >+ return -1; >+ >+ if (checkNode == node.parent) >+ return i; >+ --i; >+ } while (checkNode); >+ >+ return -1; >+} You never assign to checkNode. This makes the lines draw incorrectly. >+inAccTreeView.prototype.collapseNode = >+function collapseNode(aRow) >+{ >+ var node = this.rowToNode(aRow); >+ if (!node) >+ return; >+ >+ var row = this.getLastDescendantOf(node, aRow); >+ this.mNodes.splice(aRow + 1, row - aRow); >+ >+ node.isOpen = false; >+} This splices out one too many row - you need to do row - (aRow + 1) (or row - aRow - 1, whichever you prefer). >+inAccTreeView.prototype.getLastDescendantOf = >+function getLastDescendantOf(aNode, aRow) >+{ >+ var rowCount = this.rowCount; >+ >+ var row = aRow + 1; >+ for (; row < rowCount; ++row) { >+ if (this.mNodes[row].level <= aNode.level) >+ return row; >+ } >+ >+ return aRow; >+} This should return rowCount if there are no more descendants. Additionally, I think it would help if you expanded the document node when the pane first loads, like the DOM tree does. sr=me with these fixed.

Comment on attachment 273212 [details] [diff] [review] patch4 >+inAccTreeView.prototype.getParentIndex = >+function getParentIndex(aRow) >+{ >+ var node = this.rowToNode(aRow); >+ if (!node) >+ return -1; >+ >+ var checkNode = null; >+ var i = aRow - 1; >+ do { >+ var node = this.rowToNode(i); >+ if (!node) >+ return -1; >+ >+ if (checkNode == node.parent) >+ return i; >+ --i; >+ } while (checkNode); >+ >+ return -1; >+} And the other thing is that I don't see what stops i from becoming negative.

(In reply to comment #25) > (From update of attachment 273212 [details] [diff] [review]) > >+inAccTreeView.prototype.collapseNode = > >+function collapseNode(aRow) > >+{ > >+ var node = this.rowToNode(aRow); > >+ if (!node) > >+ return; > >+ > >+ var row = this.getLastDescendantOf(node, aRow); > >+ this.mNodes.splice(aRow + 1, row - aRow); > >+ > >+ node.isOpen = false; > >+} > This splices out one too many row - you need to do row - (aRow + 1) (or row - > aRow - 1, whichever you prefer). Why? If I have one descedant then I return row+1. row-(row+1) is a count of items I should remove. Am I wrong? > >+inAccTreeView.prototype.getLastDescendantOf = > >+function getLastDescendantOf(aNode, aRow) > >+{ > >+ var rowCount = this.rowCount; > >+ > >+ var row = aRow + 1; > >+ for (; row < rowCount; ++row) { > >+ if (this.mNodes[row].level <= aNode.level) > >+ return row; > >+ } > >+ > >+ return aRow; > >+} > This should return rowCount if there are no more descendants. If I would return rowCount then I collapse all items after aRow on collapsing. Isn't it?

(In reply to comment #27) >Why? If I have one descedant then I return row+1. No, you actually return row+2... you could change that instead though. >>This should return rowCount if there are no more descendants. >If I would return rowCount then I collapse all items after aRow on collapsing. Sorry, I meant if there are no more siblings, i.e any node along the bottom edge of the tree. (Also rowCount - 1 if you want to adjust it here.)

(In reply to comment #28) > (In reply to comment #27) > >Why? If I have one descedant then I return row+1. > No, you actually return row+2... you could change that instead though. Sorry I didn't get why. Please explain. Is it because of ++row in for statement instead of row++, or? > >>This should return rowCount if there are no more descendants. > >If I would return rowCount then I collapse all items after aRow on collapsing. > Sorry, I meant if there are no more siblings, i.e any node along the bottom > edge of the tree. (Also rowCount - 1 if you want to adjust it here.) > I didn't get again. If I return aRow when I didn't find any child then I won't remove anything because aRow - aRow == 0. Why should it be changed?

0 1 2 3 4 5 Given the above tree: 0 1 2 3 4 5 row in tree 0 4 4 4 4 5 getLastDescendantOf return value as written 6 4 4 4 6 6 correct return value with collapseNode adjusted 5 3 3 3 5 5 correct return value including adjustment

> >+inAccTreeView.prototype.getLastDescendantOf = > >+function getLastDescendantOf(aNode, aRow) > >+{ > >+ var rowCount = this.rowCount; > >+ > >+ var row = aRow + 1; > >+ for (; row < rowCount; ++row) { > >+ if (this.mNodes[row].level <= aNode.level) > >+ return row -1 ; > >+ } > >+ > >+ return rowCount - 1; > >+} Neil, correct?

Comment on attachment 273212 [details] [diff] [review] patch4 >+inAccTreeView.prototype.getCellText = >+function getCellText(aRow, aCol) >+{ >+ var node = this.rowToNode(aRow); >+ if (!node) >+ return ""; >+ >+ var accessible = node.accessible; >+ >+ if (aCol.id == "olcRole") >+ return this.mAccService.getStringRole(accessible.finalRole); >+ else if (aCol.id == "olcName") >+ return accessible.name; >+ else if (aCol.id == "olcNodeName") { >+ var node = this.getDOMNodeFor(accessible); >+ return node ? node.nodeName : ""; >+ } >+ >+ return ""; >+} Sorry for not noticing before, but you're using else after return again.

(In reply to comment #30) > 0 > 1 > 2 > 3 > 4 > 5 > Given the above tree: > 0 1 2 3 4 5 row in tree > 0 4 4 4 4 5 getLastDescendantOf return value as written > 6 4 4 4 6 6 correct return value with collapseNode adjusted > 5 3 3 3 5 5 correct return value including adjustment > You're right. I'm a bit sleeping :) Thank you for clarification.

(In reply to comment #31) > > >+ return row -1 ; > Neil, correct? Looks right, except for the odd spacing on that line ;-)

with Neil's comments.

Drivers, patch adds new UI view for DOM Inspector for accessibility stuff, it's low risk I think since patch has three r+ and one sr+ :)

AFAIK we're only in Gecko feature freeze, DOMI is still open for checkins.

(In reply to comment #37) > AFAIK we're only in Gecko feature freeze, DOMI is still open for checkins. > Do you mean I don't need approval to check in, right?

bsmedberg said I need an approval since DOMI in firefox build by default (see http://groups.google.com/group/mozilla.dev.planning/browse_frm/thread/a6700ec4506b0865/c1de812b910d4ecb#c1de812b910d4ecb). So any chance to get an approval for this?

UI Features for Firefox don't need approval. The only thing I think you'd need approval to land is the a11y back-end code. (I just confirmed this with schrep, so you just need approval for a11y code)

(In reply to comment #40) > UI Features for Firefox don't need approval. The only thing I think you'd need > approval to land is the a11y back-end code. (I just confirmed this with schrep, > so you just need approval for a11y code) > But here there are no changes in ally module. So I can check in this patch as is without approval, correct?

Right, so in this case you can just check in.

Comment on attachment 275237 [details] [diff] [review] patch5 My bad, yes you can. I must have been confused with a different bug :)

checked in