make accessibilityEvents view to filter events

RESOLVED FIXED

Status

Other Applications
DOM Inspector
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

8 years ago
Created attachment 413283 [details] [diff] [review]
patch
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #413283 - Flags: superreview?(neil)
Attachment #413283 - Flags: review?(sdwilsh)

Updated

8 years ago
Blocks: 530187
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.
Attachment #413283 - Flags: review?(sdwilsh) → review+

Comment 3

8 years ago
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.
(Assignee)

Comment 4

8 years ago
Created attachment 414692 [details] [diff] [review]
patch

with Shawn's and Neil's comments fixed.
Attachment #413283 - Attachment is obsolete: true
Attachment #414692 - Flags: superreview?(neil)
Attachment #413283 - Flags: superreview?(neil)

Comment 5

8 years ago
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...
(Assignee)

Comment 6

8 years ago
(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...
(Assignee)

Comment 7

8 years ago
(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

8 years ago
(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.)
(Assignee)

Comment 9

8 years ago
(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.
Attachment #414692 - Flags: superreview?(neil) → superreview-
(Assignee)

Comment 12

8 years ago
(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?
(Assignee)

Comment 13

8 years ago
(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?
(Assignee)

Comment 14

8 years ago
(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
(Assignee)

Comment 15

8 years ago
(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
(Assignee)

Comment 16

8 years ago
Created attachment 415308 [details] [diff] [review]
patch3
Attachment #414692 - Attachment is obsolete: true
Attachment #415308 - Flags: superreview?(neil)
(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...
(Assignee)

Comment 19

8 years ago
Created attachment 415555 [details] [diff] [review]
patch4
Attachment #415308 - Attachment is obsolete: true
Attachment #415308 - Flags: superreview?(neil)
Attachment #415555 - Flags: superreview?(neil)
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.
Attachment #415555 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 21

8 years ago
landed http://hg.mozilla.org/dom-inspector/rev/b5e547cbfa12 with Neil's commet fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.