393355 - make accessibilityEvents view to filter events

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[alexander :surkov (:asurkov)]

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

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Shawn Wilsher :sdwilsh]

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 3

[[e-mail]]

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.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch

with Shawn's and Neil's comments fixed.

Comment 5

[[e-mail]]

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

Comment 6

[alexander :surkov (:asurkov)]

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

Comment 7

[alexander :surkov (:asurkov)]

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

Comment 8

[[e-mail]]

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

Comment 9

[alexander :surkov (:asurkov)]

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

Comment 10

[[e-mail]]

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

[[e-mail]]

Comment on attachment 414692 [details] [diff] [review]
patch

But still r- because of the other issues as previously noted.

Comment 12

[alexander :surkov (:asurkov)]

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

Comment 13

[alexander :surkov (:asurkov)]

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

Comment 14

[alexander :surkov (:asurkov)]

(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

Comment 15

[alexander :surkov (:asurkov)]

(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

Comment 16

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 17

[[e-mail]]

(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 ;]

Comment 18

[[e-mail]]

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

[alexander :surkov (:asurkov)]

Attachment: patch4

Comment 20

[[e-mail]]

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.

Comment 21

[alexander :surkov (:asurkov)]

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