accessibilityEvents view should have an ability to filter events by type (see nsIAccessibleEvent for possible types).

Comment on attachment 413283 [details] [diff] [review] patch r=sdwilsh, with the caveat that you add a bunch of javadoc style comments to these methods.

Comment on attachment 413283 [details] [diff] [review] patch >+ watchEvents: function watchEvents(aDoWatch) watchAllEvents perhaps? >+const kEvent = Components.interfaces.nsIAccessibleEvent; [Confusing name (I know you wanted a short one).] >+ this.toggleOpenState = function watchview_toogleOpenState(aRowIndex) Nit: need to invalidate the row, so as to update the twisty. (Clicking the twisty with the mouse ends up invalidating the row for some as yet undetermined reason but toggling with the keyboard does not.) >+ this.setCellValue = function watchview_setCellValue(aRowIndex, aCol, aValue) Bah, aValue is a string, isn't it. I think it would make more sense to convert it to a boolean here than fiddling around with conversions everywhere else. >+ this.mTree.invalidateColumnRange(aRowIndex, aRowIndex + length, aCol); No variable called length. >+ if (parentData.value && aValue == "false") { >+ parentData.value = "false"; [But this will still evaluate as true next time around...] >+ this.registerEventTypes = function watchview_registerEventTypes() Could all the events for one type be listed in an array perhaps? >+ this.mRowCount = 0; Not used? >+treechildren::-moz-tree-checkbox(checked) { >+ list-style-image: url("chrome://global/skin/checkbox/cbox-check.gif"); >+} At least in Modern, if you're going to override the checked state then you should override the unchecked state to match.

with Shawn's and Neil's comments fixed.

Comment on attachment 414692 [details] [diff] [review] patch >+ this.mRowCount = 0; Is this the same or another unused mRowCount? >+ this.__defineGetter__("rowCount", function() { return this.getRowCount(); }); I wonder, does this.__defineGetter__("rowCount", this.getRowCount); work? >+ this.toggleOpenState = function watchview_toogleOpenState(aRowIndex) >+ { >+ var data = this.getData(aRowIndex); >+ >+ data.open = !data.open; >+ var rowCount = data.children.length; >+ Nit: spaces on line (better than last time though, you had three before!) For some reason I've only just noticed that you don't implement hasNextSibling, which means that the tree lines draw incorrectly. >+ this.registerEventGroup(kMutationEvents, >+ gBundle.getString("mutationEvents")); [I'm wondering whether it's worth creating an array of property names too.] >+ // Register event types. >+ for (var idx = 0; idx < gEventTypesMap.length; idx++) { Could start at 1 since you know you'll ignore the first entry. >+function eventProps(aGroup, aDefValue) >+{ >+ var obj = { >+ group: aGroup, >+ defValue: aDefValue That's not really true, because it gets passed to the aIgnored parameter, so maybe call these ignored and aIgnored? >+ new eventProps(kIgnoredEvents), // No event Since you haven't written eventProps as a true constructor (it returns an object) then just writing eventProps(kIgnoredEvents) will work. Alternatively if you want to write eventProps as a constructor then use function eventProps(aGroup, aIgnored) { this.group = aGroup; this.ignored = aIngored; } >+ new eventProps(kNotificationEvents, true), // EVENT_DRAGDROP_START >+ new eventProps(kNotificationEvents, true), // EVENT_DRAGDROP_END Is it likely that you'll only have a few events that you want to ignore by default? I'm wondering because you might be better off creating the initial array of just the event group, and then going back and ignoring these two. >+treechildren::-moz-tree-checkbox(checked) { >+ list-style-image: url("chrome://global/skin/checkbox/cbox-check.gif"); >+} No update here...

(In reply to comment #5) > (From update of attachment 414692 [details] [diff] [review]) > >+ this.mRowCount = 0; > Is this the same or another unused mRowCount? It's another used mRowCount :) > >+ new eventProps(kNotificationEvents, true), // EVENT_DRAGDROP_START > >+ new eventProps(kNotificationEvents, true), // EVENT_DRAGDROP_END > Is it likely that you'll only have a few events that you want to ignore by > default? That's true for now. In the future they (I) will find probably we need to disable more events. > I'm wondering because you might be better off creating the initial > array of just the event group, and then going back and ignoring these two. I didn't catch you going back and ignoring. Do you mean if statement in 'for' cycle? > >+treechildren::-moz-tree-checkbox(checked) { > >+ list-style-image: url("chrome://global/skin/checkbox/cbox-check.gif"); > >+} > No update here...

(In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 414692 [details] [diff] [review] [details]) > > >+treechildren::-moz-tree-checkbox(checked) { > > >+ list-style-image: url("chrome://global/skin/checkbox/cbox-check.gif"); > > >+} > > No update here... I forgot to say I forgot to fix this. Sorry!

(In reply to comment #6) > (In reply to comment #5) > > >+ new eventProps(kNotificationEvents, true), // EVENT_DRAGDROP_START > > >+ new eventProps(kNotificationEvents, true), // EVENT_DRAGDROP_END > > Is it likely that you'll only have a few events that you want to ignore by > > default? > That's true for now. In the future they (I) will find probably we need to > disable more events. > > I'm wondering because you might be better off creating the initial > > array of just the event group, and then going back and ignoring these two. > I didn't catch you going back and ignoring. Do you mean if statement in 'for' > cycle? Rather than one having one "very big" array, I thought it might be better having one medium array of all the events showing which group they are in, and one small array of all the initially disabled events. (Originally since there are only two disabled events I thought maybe you could just write a bit of code to disable them but now I know that other events might get added.)

(In reply to comment #8) > Rather than one having one "very big" array, I thought it might be better > having one medium array of all the events showing which group they are in, and > one small array of all the initially disabled events. I still don't get if this will be nicer. Now I have one structure (an array of objects). When I initialize the tree children then I run through the array one time and initialize them. If I will have two plain arrays then I should have nested loops what doesn't look very nice and it sounds the code should be readable lesser in this case. Probably I miss something in your idea and the code snippet could make it more visual. I could try to have one array (map event types into event group) and one object (map event types into 'ignore' flag, i.e. if event type is presented in the object then it is ignored by default, otherwise it's handled). It would save some memory but would it be nicer I'm not sure.

(In reply to comment #9) > would it be nicer I'm not sure. In that case we'll stick with the general approach in your latest patch.

Comment on attachment 414692 [details] [diff] [review] patch But still r- because of the other issues as previously noted.

(In reply to comment #10) > (In reply to comment #9) > > would it be nicer I'm not sure. > In that case we'll stick with the general approach in your latest patch. So do you prefer to have two arrays and nested loops?

(In reply to comment #3) > >+treechildren::-moz-tree-checkbox(checked) { > >+ list-style-image: url("chrome://global/skin/checkbox/cbox-check.gif"); > >+} > At least in Modern, if you're going to override the checked state then you > should override the unchecked state to match. Does modern style provide the style for checked/unchecked states by default? If so then I could the style for checked state. How can I switch between classic and modern styles?

(In reply to comment #13) > > At least in Modern, if you're going to override the checked state then you > > should override the unchecked state to match. > > Does modern style provide the style for checked/unchecked states by default? If > so then I could the style for checked state. Ignore this please

(In reply to comment #5) > >+ this.__defineGetter__("rowCount", function() { return this.getRowCount(); }); > I wonder, does this.__defineGetter__("rowCount", this.getRowCount); work? It doesn't work here. I get an error: Error: invalid getter usage Source File: chrome://inspector/content/viewers/accessibleEvents/accessibleEvents.js Line: 305

(In reply to comment #15) > (In reply to comment #5) > > >+ this.__defineGetter__("rowCount", function() { return this.getRowCount(); }); > > I wonder, does this.__defineGetter__("rowCount", this.getRowCount); work? > It doesn't work here. I get an error: > Error: invalid getter usage > Source File: > chrome://inspector/content/viewers/accessibleEvents/accessibleEvents.js > Line: 305 That's because you did it before setting this.getRowCount! In fact, I now think it would be better to change this.getRowCount = function watchview_getRowCount() { ... }; to this.__defineGetter__("rowCount", function watchview_getRowCount() { ... }); [Note that all of the this.foo = function() {} statements should end with ;]

(In reply to comment #5) > For some reason I've only just noticed that you don't implement hasNextSibling, > which means that the tree lines draw incorrectly. They're still not quite right, I need to look into it...

Comment on attachment 415555 [details] [diff] [review] patch4 >+ this.hasNextSibling = function(aRowIndex, aAfterIndex) >+ { >+ var info = this.getInfo(aRowIndex); >+ var siblings = info.parentData.children; >+ return siblings[aAfterIndex + 1] ? true : false; aAfterIndex is also a row index, which is why indexing siblings is wrong. Given the way hasNextSibling works, and the way you are representing the data in the tree, I'd say the easiest way to fix this is to use return siblings[siblings.length - 1] != info.data; i.e. any data that is not the last child has a next sibling. sr=me with that fixed.

landed http://hg.mozilla.org/dom-inspector/rev/b5e547cbfa12 with Neil's commet fixed.