Originally I meant 'Select Element By Click' ability and I'm not sure 'Find Node by Id, Tag or Attribute' makes sense for accessible tree. Though why not? Possibly I would add an ability to search by accessible name and role. Please get more feedback from ally team.

:). I understood.

Find accessible nodes in tree by role, name, node name.

I've only had a quick look, but in startFind() you can set fn to the actual function you want it to call, and then later you can do |this.mFindFn(walker)| instead of |this[this.mFindFn](walker)|. Also, where you have + if (this.mType == "role") { + this.mOpener.startFind("role", el.value); + } else if (this.mType == "name") { + this.mOpener.startFind("name", el.value); + } else if (this.mType == "tag") { + this.mOpener.startFind("tag", el.value); + } you can instead use the simpler |this.mOpener.startFind(this.mType, el.value);| I'll give you a more thorough review soon.

with above points.

(In reply to comment #5) > I'll give you a more thorough review soon. > Daniel, is there any update on this?

Comment on attachment 288442 [details] [diff] [review] patch My apologies for not getting to this sooner. You should have poked me on irc ages ago. :) >+ var fn = null; >+ switch (aType) { >+ case "role": >+ fn = this.doFindElementsByRole; >+ break; >+ case "name": >+ fn = this.doFindElementsByName; >+ break; >+ case "tag": >+ fn = this.doFindElementsByTag; >+ break; >+ }; >+ It seems to me that you use this switch on a type string too often. This instance can be eliminated simply by passing in the function rather than the type. The caller also has a switch on aType, so you would in effect be combining them. >+ this.mFindFn = fn; >+ var root = this.mView.rowToNode(0).accessible; >+ this.mFindWalker = new AccessibleTreeWalker(root); >+ this.findNext(); >+ }, >+ >+ findNext: function() >+ { >+ var walker = this.mFindWalker; >+ var result = null; >+ if (walker) { >+ result = walker.nextNode(); >+ while (result) { >+ if (this.mFindFn(walker)) { >+ break; >+ } >+ result = walker.nextNode(); This feels very clunky. You could replace it with this single line: while (this.mFindFn(result = walker.nextNode())); but I'm not sure that's much better. Brevity is the soul of wit, right? At any rate, you could rename 'result' to 'element' or something. Oh, and invert the test for walker into an early return, so that it doesn't feel like the entire function lives inside of a single if statement. >+ var bundle = this.mPane.panelset.stringBundle; >+ var msg = bundle.getString("findNodesDocumentEnd.message"); >+ var title = bundle.getString("findNodesDocumentEnd.title"); >+ >+ var promptService = XPCU.getService(kPromptServiceCID, "nsIPromptService"); >+ promptService.alert(window, title, msg); This sort of thing would make a nice utility type function to have around, you might check to see if there already is such a function you can use, or if there are other uses of the prompt service that would warrant writing one. If there isn't, then it's fine as is. >+ } >+ } >+ }, >+ >+ doFindElementsByRole: function(aWalker) >+ { >+ var re = new RegExp(this.mFindParam, "i"); >+ >+ accService = XPCU.getService(kAccessibleRetrievalCID, nsIAccessibleRetrieval); >+ var strRole = accService.getStringRole(aWalker.currentNode.role); >+ return aWalker.currentNode && re.test(strRole); >+ }, Cache the regexp and the service here, so that you don't have to do all that work for each and every node in the document. >+ >+ doFindElementsByName: function(aWalker) >+ { >+ var re = new RegExp(this.mFindParam, "i"); >+ >+ return aWalker.currentNode && re.test(aWalker.currentNode.name); >+ }, >+ >+ doFindElementsByTag: function(aWalker) >+ { >+ var re = new RegExp(this.mFindParam, "i"); >+ >+ return aWalker.currentDOMNode && re.test(aWalker.currentDOMNode.nodeName); >+ }, Likewise for these regexps as well. Also, I notice that you're passing in the walker instead of the node. If you pass in the node instead, you can dispense with the 'aWalker.currentNode' bits and just say 'node'. >+function FindAccessNodesDialog() >+{ >+ this.toggleType(window.arguments[0] ? window.arguments[0] : "role"); >+ this.setValue(window.arguments[1] ? window.arguments[1] : ""); No need for the conditional for the second argument. >+ this.mOpener = window.opener.viewer; >+ >+ var txf = document.getElementById("tfText1"); >+ txf.select(); >+ txf.focus(); This should be unnecessary, as the first focusable element in a dialog automatically gains the focus when it's opened. >+ setValue: function(aValue) >+ { >+ if (aValue) { >+ var txf = document.getElementById("tfText1"); >+ txf.value = aValue; >+ } Don't bother to check aValue here. Setting the value of a textbox to an empty string is no crime :) >+ <row id="rwRow1"> >+ <deck id="rwRow1Text"> >+ <label id="txText1Role" value="&findAccessNodesByRole.label;"/> >+ <label id="txText1Name" value="&findAccessNodesByName.label;"/> >+ <label id="txText1Tag" value="&findAccessNodesByTag.label;"/> >+ </deck> This seems unnecessary. Use a stringbundle instead. Also, the ids you give to the various elements are pretty boring. You could use names that describe their purpose rather than merely their positions in the dialog. >+ <radiogroup id="rgType"> >+ <radio id="rdType_role" value="role" >+ label="&findAccessNodesByRole.label;" >+ oncommand="dialog.toggleType('role')"/> >+ <radio id="rdType_name" value="name" >+ label="&findAccessNodesByName.label;" >+ oncommand="dialog.toggleType('name')"/> >+ <radio id="rdType_tag" value="tag" >+ label="&findAccessNodesByTag.label;" >+ oncommand="dialog.toggleType('tag')"/> You could replace those three oncommand attributes on the radio elements with a single one on the radiogroup. Make it look like oncommand="dialog.toggleType(this.value)" >+ <vbox> >+ <button label="&findAccessNodesFind.label;" dlgtype="accept" default="true"/> >+ <button label="&findAccessNodesCancel.label;" dlgtype="cancel"/> >+ </vbox> >+ </hbox> >+ You can get rid of these buttons and let the dialog binding put the buttons in for you, that way you get them in the right order on all platforms. You can customize the labels with attributes on the dialog element. >+ <popupset id="ppsViewerPopupset"> >+ <menupopup id="ppViewerContext-accessibleTree"> >+ <menuitem id="item:accFind" >+ label="&cmdShowFindDialog.label;" >+ accesskey="&cmdShowFindDialog.accesskey;" >+ key="key:find" >+ observes="cmd:find"/> >+ >+ <menuitem id="item:accFindNext" >+ label="&cmdFindNext.label;" >+ key="key:findNext" >+ observes="cmd:findNext"/> These would be more readable if all of the attributes lined up, but it's not very important. > <!ENTITY role.label "Rolle"> > <!ENTITY name.label "Name"> > <!ENTITY nodeName.label "Knoten-Name"> > >+<!ENTITY cmdShowFindDialog.label "Find Nodes..."> >+<!ENTITY cmdShowFindDialog.accesskey "f"> >+ >+<!ENTITY cmdFindNext.label "Find Next"> >+<!ENTITY cmdFindNext.accesskey "n"> The access keys should use the same case as the letter you want underlined, so they should probably both be uppercase. You'll want to change that for all of the locales, of course. So, I guess most of that was just stylistic nits, except for caching the regexps in the search functions. That will probably net you a measurable performance boost.

While tis idea is great in principle, I believe a new bug should be filed for the Dev Tools Accessibility inspector for this if we want it.

(In reply to Marco Zehe (:MarcoZ) from comment #9) > While tis idea is great in principle, I believe a new bug should be filed > for the Dev Tools Accessibility inspector for this if we want it. I think DevTools has something like this, maybe not in production yet.