553364 - allow user handlers for accessible events

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[alexander :surkov (:asurkov)]

It's nice to have an ability allowing the user register event listeners on accessible events so that his handler is triggered and the user can see an output. For example it's very handy for accessible state checking when accessible event is handled.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Colby Russell :crussell (no longer Mozilla-ing)]

You might want to try review from Shawn first. (Additionally, I'll probably won't be able to get to this in the next two and a half weeks.)

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> You might want to try review from Shawn first. (Additionally, I'll probably
> won't be able to get to this in the next two and a half weeks.)

done

Comment 4

[Colby Russell :crussell (no longer Mozilla-ing)]

Hooray for idle time. Some stuff I noticed:

(From update of attachment 442318 [details] [diff] [review])
>    } else {
...
>    } else if (aCol.id == "welIsHandlerEnabled") {
...
>    } else if (aCol.id == "welIsHandlerEnabled") {

else on a new line

>    <description>&descrHandlerOutput.label;</description>

<description value="&descrHandlerOutput.label;"/>?

>+    var str = gBundle.getString("eventHandlerEditorLabel");
>+
>+    if (idx == -1 || this.isContainer(idx)) {
>+      this.mHandlerEditor.setAttribute("disabled", "true");
>+      this.mHandlerEditorLabel.value = str[0].toUpperCase() + str.substr(1);
>+      return;
>+    }
...
>+eventHandlerEditorLabel = event handler sources:

Why not just make the string in the stringbundle capitalized to begin with?

Comment 5

[Colby Russell :crussell (no longer Mozilla-ing)]

I can't apply this to trunk.

Comment 6

[[e-mail]]

(In reply to comment #5)
> I can't apply this to trunk.
Maybe it depends on bug 551404?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> (In reply to comment #5)
> > I can't apply this to trunk.
> Maybe it depends on bug 551404?

right. thank you, Neil. Sorry I didn't said this. Usually I work on patches up on top.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> Hooray for idle time. Some stuff I noticed:

thank you

> else on a new line

fixed locally

> >    <description>&descrHandlerOutput.label;</description>
> 
> <description value="&descrHandlerOutput.label;"/>?

that's how we do in this file, no difference expect no @value allows to broke description into mutliple lines

> >+    var str = gBundle.getString("eventHandlerEditorLabel");
> >+
> >+    if (idx == -1 || this.isContainer(idx)) {
> >+      this.mHandlerEditor.setAttribute("disabled", "true");
> >+      this.mHandlerEditorLabel.value = str[0].toUpperCase() + str.substr(1);
> >+      return;
> >+    }
> ...
> >+eventHandlerEditorLabel = event handler sources:
> 
> Why not just make the string in the stringbundle capitalized to begin with?

because eventHandlerEditorLabel = event handler sources: is used as is few lines below.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> (In reply to comment #5)
> > I can't apply this to trunk.
> Maybe it depends on bug 551404?

bug 551404 was landed, this patch should be applied cleanly

Comment 10

[alexander :surkov (:asurkov)]

Attachment: patch2

a slightly changed UI: handler controls are hidden by default

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch3

add some syntax sugar for handler sources and add helper dialog that lists sugar methods.

Comment 12

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 445982 [details] [diff] [review]
patch3

> +  onWatchViewHandlerStateChanged: function onWatchViewHandlerStateChanged(aState)
> +  {

Break for line length.

> +/**
> + *
> + */
> +var gEventHandlerOutput = "";

?

> +    } catch (ex) {

catch on separate line

> +  if (aRow == -1)
> +    return null;
> +
...
> +    if (aCol.id == "welIsWatched" || aCol.id == "welIsHandlerEnabled")
> +      return true;

Brace these.

> +    var updateCheckbox = !updateData || aCol != undefined || aRowIdx != undefined;

Break for line length.

> +    var handlerState = "(" +
> +      gBundle.getString(this.mHandlerState.checked ? "active" : "inactive") +
> +      ").";

var checkedState = this.mHandlerState.checked ? "active" : "inactive";
var handlerState = "(" + gBundle.getString(checkedState) + ").";

Or some other approach that lines up the string parts without making the line too long.

> +var accRetrieval = Components.classes["@mozilla.org/accessibleRetrieval;1"].
> +  getService(Components.interfaces.nsIAccessibleRetrieval);

Don't we have Ci or XPCU available?

> +  for (var i = 0; i < states.length; i++)
> +    list.push(states.item(i));

Brace this.

> new file mode 100644

?

+      <tab label="Event list"/>
+      <tab label="Watched events"/>

Need to localize these.

+            <toolbarbutton label="?"

This too?

I say thumbs up to some kind of contextual help, but I'm not sure I like the use of a toolbarbutton or if this is the right approach. With my widget set, at least, toolbarbuttons don't look like buttons until you're hovering over them, so the floating question mark just looks like some misplaced text.

> --- /dev/null
> +++ b/resources/content/viewers/accessibleEvents/handlerHelpDialog.xul

This would probably work best as an iframe/browser that includes the appropriate localized doc or integrated with the help viewer with regard to SeaMonkey. But I suppose it can stay, pending some kind of decision about how to handle contextual help.

> +<!DOCTYPE dialog [
> +  <!ENTITY % dtd1 SYSTEM "chrome://inspector/locale/viewers/accessibleEventsHandlerHelpDialog.dtd"> %dtd1;
> +]>

<!DOCTYPE dialog SYSTEM
  "chrome://inspector/locale/viewers/accessibleEventsHandlerHelpDialog.dtd">

> +      var label = gBundle.getString("handlerEditorNoEventLabel");
> +      this.mHandlerState.hidden = true;
> +      this.mHandlerEditor.disabled = true;
> +      this.mHandlerEditorLabel.value = label[0].toUpperCase() + label.substr(1);
...
> +handlerEditorNoEventLabel = Choose event type to add a handler.

Okay, this one seems to be used in only one place, and it *is* already capitalized, so no need for the toUpperCase and string building. (Although I might be overlooking something again...)

> +    this.mHandlerEditorLabel.value = "'" + data.text + "' " + label;

Should be 
  this.mHandlerEditorLabel.value = "\"" + data.text + "\" " + label;

or
  this.mHandlerEditorLabel.value = '"' + data.text + '" ' + label;

> +active = active
> +inactive = inactive
...
> +<!ENTITY isHandlerEnabled.label "Is handler enabled">

Need to be consistent about enabled/disabled versus active/inactive. I prefer enabled/disabled.

> +        <tree id="watchEventList"

You should go ahead and make this tree seltype="single".

> +            <treecol id="welIsHandlerEnabled"
> +                     label="&isHandlerEnabled.label;"
> +                     type="checkbox"

I'd rather have this column be "Handler State" with three states: enabled, disabled, and no handler (or something similar). This is so you can see for which events you have handlers, even if they're disabled.

Also, the "Event Type" column uses title capitalization (all significant words are capitalized), but this column (and the welIsWatched column) use sentence capitalization (only the first word is capitalized).

> +          <button label="&handler.label;"
> +                  oncommand="viewer.showHideHandlerEditor();"/>
...
> +<!ENTITY handler.label "Handler">

Maybe "Add/Edit Handlers" would be better? I think I'd rather have a splitter between the tree and the hbox, though. In my DOM Inspector window, I can only see two and a half rows in the event types tree, and there's no way to resize the bottom "pane". If you want to gain some vertical space, you could get rid of the other two buttons, and make them entries in the tree's context menu.

Also, the same capitalization issue exists as above with the "Select all" and "Unselect all" buttons. Compare it with the "Clear Event List" button in the "Event list" tab. (It looks like the tab titles use sentence capitalization, too. Should they be changed as well?)

> +            <label control="welHandlerEditor"
...
> +            <checkbox id="welHandlerState" hidden="true"
...
> +            <spacer flex="1"/>
> +            <toolbarbutton label="?"

If you put the spacer here, it means the enable/disable checkbox will move around as you move through the event types tree, because its position depends on the length of the label.

I think the spacer should go between the label and the checkbox.

> +          <textbox id="welHandlerEditor" multiline="true" rows="8" flex="1"/>

This should have a fixed width font.

> +  <!ENTITY general.descr "You write and attach accessibility event handler. When event is handled you can select it in events list and see an result of your handler evaluation by 'accessible event' viewer in the right pane.">

Suggested wording:
  Write and attach an accessibility event handler. When the event occurs, you
  can select it in the events list and see the result of the handler
  evaluation in the linked Accessible Event viewer.

Or just:
  When the event occurs, you can select it in the events list and see the
  result of the handler evaluation in the linked Accessible Event viewer.

> +<!-- ***** BEGIN LICENSE BLOCK *****
> +#if 0
> +   - Version: MPL 1.1/GPL 2.0/LGPL 2.1
...
> +#endif
> +   - ***** END LICENSE BLOCK ***** -->

If the preprocessing instructions are going to go inside the comments (per bug 386818, comment 16; more discussion in bug 388203), the #if should be moved one line down, so the preprocessed file reads:

<!-- ***** BEGIN LICENSE BLOCK *****
   - Version: MPL 1.1/GPL 2.0/LGPL 2.1
   - ***** END LICENSE BLOCK ***** -->

instead of:

<!-- ***** BEGIN LICENSE BLOCK *****
   - ***** END LICENSE BLOCK ***** -->

This eliminates what would look like spurious comments to any curious person poking through the unpacked XPI, while leaving intact some semblance of a license "block" that actually communicates something that someone can use to find further information.

You also forgot to remove the ru locale from the Makefile.

There's another issue where selecting or deselecting certain items makes the width of the viewer grow larger than its pane and become clipped. See the attached screenshot.

I can't actually get any events to show up.

I have a suggestion that you can choose to take or not: for onchange for the handler editor, if the textbox is empty, switch the handler state to disabled. If the value was previously empty, but now it's non-empty, switch the handler state to enabled.

Please attach a new patch with the noted changes.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #12)
> > new file mode 100644

That's how hg diff --git works on Windows. All text files in repo has 644 permissions so it's ok I think.

> 
> I say thumbs up to some kind of contextual help, but I'm not sure I like the
> use of a toolbarbutton or if this is the right approach. With my widget set, at
> least, toolbarbuttons don't look like buttons until you're hovering over them,
> so the floating question mark just looks like some misplaced text.

Probably it should be nice image I don't know. I didn't care a lot about nice UI look and I'm not sure I would like. I was focused on functionality.

> > --- /dev/null
> > +++ b/resources/content/viewers/accessibleEvents/handlerHelpDialog.xul
> 
> This would probably work best as an iframe/browser that includes the
> appropriate localized doc or integrated with the help viewer with regard to
> SeaMonkey. But I suppose it can stay, pending some kind of decision about how
> to handle contextual help.

Yeah, it's the nineties UI. Primary I care to not force users to look into sources to get an idea what they can do, so I implemented basic approach I figured out.

> > +<!DOCTYPE dialog [
> > +  <!ENTITY % dtd1 SYSTEM "chrome://inspector/locale/viewers/accessibleEventsHandlerHelpDialog.dtd"> %dtd1;
> > +]>
> 
> <!DOCTYPE dialog SYSTEM
>   "chrome://inspector/locale/viewers/accessibleEventsHandlerHelpDialog.dtd">

I prefer the first one since it allows to add more than dtd, it's just more flexible. I changed it to fit 80 lines restriction.

> > +            <treecol id="welIsHandlerEnabled"
> > +                     label="&isHandlerEnabled.label;"
> > +                     type="checkbox"
> 
> I'd rather have this column be "Handler State" with three states: enabled,
> disabled, and no handler (or something similar). This is so you can see for
> which events you have handlers, even if they're disabled.

I like to have checkbox column, all the user needs is to see what event has enabled handler, so tri-state (cycler column) and especially if you mean to show text in the column then it would be harder to important info.

Also we could think handler exists always (expecting for event groups which aren't events).


> Also, the "Event Type" column uses title capitalization (all significant words
> are capitalized), but this column (and the welIsWatched column) use sentence
> capitalization (only the first word is capitalized).
> 
> > +          <button label="&handler.label;"
> > +                  oncommand="viewer.showHideHandlerEditor();"/>
> ...
> > +<!ENTITY handler.label "Handler">
> 
> Maybe "Add/Edit Handlers" would be better? I think I'd rather have a splitter
> between the tree and the hbox, though. In my DOM Inspector window, I can only
> see two and a half rows in the event types tree, and there's no way to resize
> the bottom "pane". If you want to gain some vertical space, you could get rid
> of the other two buttons, and make them entries in the tree's context menu.

I like splitter, however I don't like to move buttons into tree's context menu, it's lesser discoverable and they don't belong to selected item. I don't like vertical space is lost with the splitter. However once we have splitter it's perhaps not so critical.

> > +            <label control="welHandlerEditor"
> ...
> > +            <checkbox id="welHandlerState" hidden="true"
> ...
> > +            <spacer flex="1"/>
> > +            <toolbarbutton label="?"
> 
> If you put the spacer here, it means the enable/disable checkbox will move
> around as you move through the event types tree, because its position depends
> on the length of the label.

> I think the spacer should go between the label and the checkbox.

spacer is not repsonsible on checkbox movement, it's used to keep help toolbarbutton in the right. I think it's fine checkbox is moving since it should be considered as text continuation.

> There's another issue where selecting or deselecting certain items makes the
> width of the viewer grow larger than its pane and become clipped. See the
> attached screenshot.
> 
> I can't actually get any events to show up.

I need steps to reproduce.

> I have a suggestion that you can choose to take or not: for onchange for the
> handler editor, if the textbox is empty, switch the handler state to disabled.
> If the value was previously empty, but now it's non-empty, switch the handler
> state to enabled.

Thanks. However personally I do not feel it's needed, at least I didn't catch myself thinking about this ability. Probably because I wasn't needed more than one event handler the same time but I haven't deal with complex cases yet. But I'll keep your suggestion in mind. 

> Please attach a new patch with the noted changes.

Thank you for detailed review. Patch is coming.

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch4

Comment 15

[[e-mail]]

Comment on attachment 446205 [details] [diff] [review]
patch4

>+    openDialog("chrome://inspector/content/viewers/accessibleEvents/handlerHelpDialog.xul");
IIRC the Mac treats a lack of chrome flags as meaning no chrome at all. Or something; I seem to remember having problems with chrome flags when we were replacing the calls to moveToAlertPosition with the centerscreen flag, which incidentally may be a useful flag to use here ;-)

>+      if (accessible instanceof gAccInterfaces[idx]) {
>+        accessible.QueryInterface(gAccInterfaces[idx]);
instanceof actually does a QueryInterface "behind the scenes", so you're duplicating work.

>+      var f = Function("event", "target", "output", expr);
>+      var result = f(event, accessible, output);
[Should this be in a sandbox? I guess it doesn't really matter.]

>+    var idx = this.mTree.treeBody.parentNode.currentIndex;
Later on you simplified this to this.mTree.view.selection.currentIndex ...

+      this.mTree.view.selection.currentIndex : aRowIdx;
... but this.mTree.view == this, which saves you a step ;-)

>+      var col = aCol ?
>+        aCol : this.mTree.columns.getNamedColumn("welIsHandlerEnabled");
Nit: aCol ? aCol : is the same as aCol ||

>+    var checkedState = this.mHandlerState.checked ? "enabled" : "disabled";
>+    var handlerState = "(" + gBundle.getString(checkedState) + ").";
>+    this.mHandlerState.label = handlerState;
I found this disconcerting ;-) I would have preferred something like
[X] Enable "reorder" event handler
where checking/unchecking doesn't change the label.

>+    var idx = this.mTree.treeBody.parentNode.currentIndex;
See above.

>+        <hbox align="right">
Don't use "right", it's not RTL-friendly. Use "end" instead.

>+          <button label="&unselectAll.label;"
>+                  oncommand="viewer.watchAllEvents(false);"/>
[How did this not get called Clear All or Select None?]

>+          <grippy pack="center">
>+            <label value="&editHandler.label;"/>
>+          </grippy>
This looks really ugly in Modern, and I doubt it looks better in other themes (gnomestripe I believe also gives the grippy a background). I guess you could always theme it to have no appearance or background as appropriate...

>+            <checkbox id="welHandlerState" hidden="true"
>+                      oncommand="viewer.onWatchViewHandlerStateChanged(this.checked);"/>
I notice that this worked well. Unfortunately the reverse did not appear to be true, as the check state was out of sync, although it wasn't obvious why.

Comment 16

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #15)
> (From update of attachment 446205 [details] [diff] [review])
...
> >+          <button label="&unselectAll.label;"
> >+                  oncommand="viewer.watchAllEvents(false);"/>
> [How did this not get called Clear All or Select None?]

Actually, I was thinking about this. I'd rather it be "Watch All" and "Unwatch All" (or "Watch None"). "Select" already has a specific meaning when working with tree widgets. Plus "Watch All"/"Unwatch All" gets the real verb in there, instead of a proxy.

Comment 17

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 446205 [details] [diff] [review]
patch4

> +      var newValue = aValue == "true" ? true : false;

Just
  var newValue = aValue == "true";

> +          <treecols>
> +            <treecol id="welEventType"
...
> +            <treecol id="welIsWatched"
...
> +            <treecol id="welIsHandlerEnabled"

We should have tree splitters between these columns.

> +<!ENTITY isWatched.label "Is Watched">
> +<!ENTITY isHandlerEnabled.label "Is Handler Enabled">

You can drop the "Is " part, since these are checkbox columns.

> +        <splitter state="collapsed" collapse="after" persist="state"
> +                  resizebefore="farthest">

This resizes the label above the tree, since that's what's farthest.

I wonder why captions inside a groupbox don't have a "control" attribute (or default to that kind of behavior anyway).

> +        <splitter state="collapsed" collapse="after" persist="state"
> +                  resizebefore="farthest">

This needs an id for persist to work.

(In reply to comment #13)
> > I say thumbs up to some kind of contextual help, but I'm not sure I like the
> > > use of a toolbarbutton or if this is the right approach. With my widget set, at
> > > least, toolbarbuttons don't look like buttons until you're hovering over them,
> > > so the floating question mark just looks like some misplaced text.
> Probably it should be nice image I don't know. I didn't care a lot about nice
> UI look and I'm not sure I would like. I was focused on functionality.

I was more talking about that if it's a toolbarbutton, I can't really tell that it's anything other than a label until I'm hovering over it, and that I'm sure there's a better approach to contextual help than putting these buttons up everywhere. In any case, there's no set alternative right now about how to do it, and it's not required in this bug to figure out what it might be.

> Also we could think handler exists always (expecting for event groups which
> aren't events).

Assuming that all events have handlers doesn't help if you're looking for disabled handlers but don't remember the event type you set it for. Alternatively this could be accomplished with another "Handler Set" column.

> > There's another issue where selecting or deselecting certain items makes the
> > width of the viewer grow larger than its pane and become clipped. See the
> > attached screenshot.
> > 
> > I can't actually get any events to show up.
> 
> I need steps to reproduce.

Oops! It looks like I forgot to attach the screenshot last time.

It looks like the no events issue is the result of bug 551404, since I can't get any events with current dom-inspector/ (but I can see them in 2.0.5).

(In reply to comment #15)
> > +          <grippy pack="center">
> > +            <label value="&editHandler.label;"/>
> > +          </grippy>
> This looks really ugly in Modern, and I doubt it looks better in other themes
> (gnomestripe I believe also gives the grippy a background). I guess you could
> always theme it to have no appearance or background as appropriate...

We could just take the label out and leave a bare <grippy/>.

> I found this disconcerting ;-) I would have preferred something like
> [X] Enable "reorder" event handler
> where checking/unchecking doesn't change the label.

Bonus: if we do it like this (replacing the current label + checkbox), then it gives a consistently positioned target for the checkbox, instead of one that moves around, as I mentioned in comment 12.

Comment 18

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: viewer stretching

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> 
> >+      var f = Function("event", "target", "output", expr);
> >+      var result = f(event, accessible, output);
> [Should this be in a sandbox? I guess it doesn't really matter.]

I tried to define local variables but they aren't visible, only global variables can be used, if you're about this.
 
> +      this.mTree.view.selection.currentIndex : aRowIdx;
> ... but this.mTree.view == this, which saves you a step ;-)

It works but I don't understand how. My view class doesn't have selection attribute. I thought my view is wrapped somehow by c++ other view when I set it to the tree but I didn't know 'this' points inside my view class to this C++ view if there is any c++ view at all.
 
> >+          <button label="&unselectAll.label;"
> >+                  oncommand="viewer.watchAllEvents(false);"/>
> [How did this not get called Clear All or Select None?]

replaced on "watch all" and "watch none" as Colby suggested.

> >+          <grippy pack="center">
> >+            <label value="&editHandler.label;"/>
> >+          </grippy>
> This looks really ugly in Modern, and I doubt it looks better in other themes
> (gnomestripe I believe also gives the grippy a background). I guess you could
> always theme it to have no appearance or background as appropriate...

Ok, I put a toolbarbutton into splitter instead a grippy. Hope that's better

> >+            <checkbox id="welHandlerState" hidden="true"
> >+                      oncommand="viewer.onWatchViewHandlerStateChanged(this.checked);"/>
> I notice that this worked well. Unfortunately the reverse did not appear to be
> true, as the check state was out of sync, although it wasn't obvious why.

Hope, it's fixed.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> > +        <splitter state="collapsed" collapse="after" persist="state"
> > +                  resizebefore="farthest">
> 
> This resizes the label above the tree, since that's what's farthest.

At least visually it looks ok, tree is changed, if I miss this attribute then buttons container is resized.

> I wonder why captions inside a groupbox don't have a "control" attribute (or
> default to that kind of behavior anyway).

I'm not sure I got you.

> > > I say thumbs up to some kind of contextual help, but I'm not sure I like the
> > > > use of a toolbarbutton or if this is the right approach. With my widget set, at
> > > > least, toolbarbuttons don't look like buttons until you're hovering over them,
> > > > so the floating question mark just looks like some misplaced text.
> > Probably it should be nice image I don't know. I didn't care a lot about nice
> > UI look and I'm not sure I would like. I was focused on functionality.
> 
> I was more talking about that if it's a toolbarbutton, I can't really tell that
> it's anything other than a label until I'm hovering over it, and that I'm sure
> there's a better approach to contextual help than putting these buttons up
> everywhere. In any case, there's no set alternative right now about how to do
> it, and it's not required in this bug to figure out what it might be.

I'm not saying toolbarbutton is great. I share your point. I just don't care a lot. I'll file a bug for this, is it ok?
 
> > Also we could think handler exists always (expecting for event groups which
> > aren't events).
> 
> Assuming that all events have handlers doesn't help if you're looking for
> disabled handlers but don't remember the event type you set it for.
> Alternatively this could be accomplished with another "Handler Set" column.

I see your point, but this case is kind of impossible until handlers are saved when you switch between documents or views. Currently user handlers support is very primitive.

Sure I would like to have a multiple handlers per event, be able to see quickly what event has user handler and etc. But it's out of scope of this bug I guess.

> > > There's another issue where selecting or deselecting certain items makes the
> > > width of the viewer grow larger than its pane and become clipped. See the
> > > attached screenshot.

Sounds like XUL tree bug, do you want me to do anything with this?

> It looks like the no events issue is the result of bug 551404, since I can't
> get any events with current dom-inspector/ (but I can see them in 2.0.5).

It's very strange because I can see events. Steps to reproduce?

Comment 21

[alexander :surkov (:asurkov)]

Attachment: patch5

Comment 22

[[e-mail]]

Comment on attachment 446976 [details] [diff] [review]
patch5

>+    openDialog("chrome://inspector/content/viewers/accessibleEvents/handlerHelpDialog.xul",
>+               "chrome,modal,centerscreen");
Not quite right; the features are the third parameter; the second parameter is the window name, although we don't normally name chrome windows.

>-      var newValue = aValue == "true" ? true : false;
>+      var newValue = aValue == "true";
...
>+      var newValue = aValue == "true" ? true : false;
Missed one ;-)

>+        <splitter>
>+        <vbox>
Rather than an extra <vbox> couldn't you set resizeafter on the splitter?

>+          <hbox align="baseline">
>+            <label control="welHandlerEditor"
>+                   value="&handlerEditorNoEvent.label;"
>+                   id="welHandlerEditorLabel"/>
>+            <checkbox id="welHandlerState" hidden="true"
>+                      oncommand="viewer.onWatchViewHandlerStateChanged(this.checked);"/>
>+            <spacer flex="1"/>
>+            <toolbarbutton label="&contextHelp.label;"
>+                           oncommand="viewer.showWatchViewHandlerHelp();"/>
Some of the accessible labels are too long to fit at the default width, and that's just in English; the problem may be worse in other languages. The solution is to ditch the spacer and add flex="1" to the label and checkbox instead. You might also consider putting the no event message as a child text node of the label, in case it needs to wrap in other languages.

I noticed that "descendant" was misspelled as "decendent" in one of the events.

>+<!-- ***** BEGIN LICENSE BLOCK *****
>+   - Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+#if 0
The #if goes before the Version in all the other files.

>+#welGrippyButton {
>+  cursor: default;
>+}
Nit: missing from Modern.

Comment 23

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #22)
> >+<!-- ***** BEGIN LICENSE BLOCK *****
> >+   - Version: MPL 1.1/GPL 2.0/LGPL 2.1
> >+#if 0
> The #if goes before the Version in all the other files.

I asked for this in comment #12. Thoughts?

Comment 24

[Shawn Wilsher :sdwilsh]

(In reply to comment #23)
> I asked for this in comment #12. Thoughts?
I don't see much benefit in processing out the license block...

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #22)
> (From update of attachment 446976 [details] [diff] [review])
> >+    openDialog("chrome://inspector/content/viewers/accessibleEvents/handlerHelpDialog.xul",
> >+               "chrome,modal,centerscreen");
> Not quite right; the features are the third parameter; the second parameter is
> the window name, although we don't normally name chrome windows.

thanks for the catch.

> 
> >+        <splitter>
> >+        <vbox>
> Rather than an extra <vbox> couldn't you set resizeafter on the splitter?

I tried, it collapses element after splitter, it doesn't collapse all of them.

> I noticed that "descendant" was misspelled as "decendent" in one of the events.

It sounds it comes from IAccessible2 spec :)

Comment 26

[alexander :surkov (:asurkov)]

Attachment: patch6

Comment 27

[alexander :surkov (:asurkov)]

Attachment: patch7

the previous one was old patch

Comment 28

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #24)
> (In reply to comment #23)
> > I asked for this in comment #12. Thoughts?
> I don't see much benefit in processing out the license block...

Whatever precedent/decision there is about commenting out licensing blocks, given bug 386818, comment 16 and discussion in bug 388203, beginning with bug 388203, comment 4, people want the preprocessor directives to go inside the comments so it's still valid XML. If we *are* going to remove the license blocks, it seems like it makes the most sense to do one of the following:

<!-- ***** BEGIN LICENSE BLOCK *****
   - Version: MPL 1.1/GPL 2.0/LGPL 2.1
#if 0
...
#endif
   - ***** END LICENSE BLOCK ***** -->

or:

<!--
#if 0
   - ***** BEGIN LICENSE BLOCK *****
...
   - ***** END LICENSE BLOCK *****
#endif
   -->

Preprocessing gives us

<!-- ***** BEGIN LICENSE BLOCK *****
   - Version: MPL 1.1/GPL 2.0/LGPL 2.1
   - ***** END LICENSE BLOCK ***** -->

or

<!--
   -->

The last one's not so great, but what we get now is worse:

<!-- ***** BEGIN LICENSE BLOCK *****
   - ***** END LICENSE BLOCK ***** -->

As for why we'd be commenting anything out at all, I don't know where that's from.

Comment 29

[Shawn Wilsher :sdwilsh]

(In reply to comment #28)
> As for why we'd be commenting anything out at all, I don't know where that's
> from.
There used to be a push to get the binary size down.  I don't think we should be fretting about bytes in inspector code though.

Comment 30

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 447020 [details] [diff] [review]
patch7

First, the nits:
> +  if (aRow == -1) {
> +    return null;
> +  }

If there are going to be lines to make this check, I'd prefer they check for any aRow < 0.

> +    var idx = this.selection.currentIndex;
> +
> +    if (idx == -1 || this.isContainer(idx)) {

You can change this to < 0, too if you want to (it's shorter), but you don't have to since it's not getting its value from a parameter.

> +  if (DOMNode)
> +    DOMNode[" accessible "] = accessNode;

Brace this.

> +          if (object instanceof Components.interfaces.nsIDOMNode ||
> +              "@mozilla.org/accessibleRetrieval;1" in Components.classes &&
> +              object instanceof Components.interfaces.nsIAccessible) {

Parenthesize here, please. I couldn't tell at first which direction it waskgoing to be evaluated.

+        <splitter state="collapsed" collapse="after" persist="state"
+                  resizebefore="farthest" id="welSplitter">
+          <toolbarbutton label="&editHandler.label;" id="welGrippyButton"
+                         oncommand="viewer.toggleHandlerEditor(this.parentNode);"/>
+        </splitter>
+        <vbox>

If you want to do that instead of a grippy, you'll need to do something like this in the classic skin to prevent the default splitter look from "shining through" the toolbarbutton (on GTK2):

#welSplitter {
  -moz-appearance: none;
}

It doesn't look like the modern theme needs updating. I don't know how this might affect Windows (but I think it should be fine); you'll have to check. I think Shawn should be able to tell us how it looks on OS X.

Also, you should persist the height of the resizable elements if you want to persist the state of the splitter.

I can't really r+ this while the events viewer is broken.

(In reply to comment #22)
> (From update of attachment 446976 [details] [diff] [review])
...
> Some of the accessible labels are too long to fit at the default width, and
> that's just in English

It looks like this is what was causing the viewer stretching.

Comment 31

[alexander :surkov (:asurkov)]

(In reply to comment #30)

> Also, you should persist the height of the resizable elements if you want to
> persist the state of the splitter.

It would be nice but @persist="height" on vbox'es (on farthest above and nearest bottom) doesn't provide expected behavior.

Comment 32

[alexander :surkov (:asurkov)]

Attachment: patch8

Comment 33

[alexander :surkov (:asurkov)]

Attachment: patch9

Add outputTree function, the patch is build on top of bug 551404.

Comment 34

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 481418 [details] [diff] [review]
patch9

>+    // Update handler output.
>+    var outputElm = document.getElementById("handlerOutput");
>+    var outputList = this.mSubject[" eventHandlerOutput "];

I'm not sure that continuing to put data in properties of the nodes themselves (and exposing it to content) just to pass it between viewers is a good idea.

>+  onWatchViewItemSelected: function onWatchViewItemSelected()
>+  {
>+    this.mWatchView.updateHandlerEditor();
>+    this.mWatchView.updateHandlerStateColumn();

Why does the handler state column need updating here?  In which cases would this be expected to change anything?

>+  /**
>+   * Opend hide handler editor.
>+   */

Typo.

>+/**
>+ * Global variable used to store user's event handler output from helper
>+ * functions.
>+ */
>+var gEventHandlerOutput = [ ];
...
>+  // Execute user handlers.
>+  gEventHandlerOutput = [ ];

What's the point of initializing it twice?  I'm not sure making this global is the best idea.

>+  var expr = this.mWatchView.getHandlerExpr(type);
>+  if (expr) {
>+    for (let idx = 0; idx < gAccInterfaces.length; idx++) {
>+      // Accessibility interfaces implicit query.
>+      accessible instanceof gAccInterfaces[idx];
>+    }
>+
>+    try {
>+      var f = Function("event", "target", "output", expr);
>+      var result = f(event, accessible, output);

Should these be in a sandbox?  Why pass output as a parameter, but not accRetrieval or any of the other functions?

>-AccessibleEventsView.prototype.getDOMNode =
>-function getDOMNode(aRow)
>+AccessibleEventsView.prototype.getObject =
>+function getObject(aRow)

Why the name change?  "getObject" is kind of ambiguous.

>-  DOMNode[" accessible "] = accessNode;
>-  DOMNode[" accessible event "] = event;
>-  return DOMNode;
>+  var object = DOMNode || { __proto__: accessNode };

What's this about?

>   this.isEditable = function watchview_isEditable(aRowIndex, aCol)
>   {
>-    return true;
>+    if (aCol.id == "welIsWatched" || aCol.id == "welIsHandlerEnabled") {
>+      return true;

Should to return false here for event group rows when the column is welIsHandlerEnabled, instead of just ignoring it in setCellValue.

>+    else if (aCol.id == "welIsHandlerEnabled") {
>+      // Event groups aren't allowed to register handlers for their items.
>+      if (this.isContainer(aRowIndex)) {
>+        return;
>+      }
>+
>+      var newValue = aValue == "true";
>+      this.updateHandlerStateColumn(newValue, aRowIndex, aCol);
>+
>+      this.updateHandlerEditor();

Does the editor need to be updated?  Nothing will change as far as I can tell.

>+  this.updateHandlerStateColumn =
>+    function watchview_updateHandlerStateColumn(aValue, aRowIdx, aCol)

Why take the aCol parameter if this is only used to update welIsHandlerEnabled?  Also, it would be nice to document the parameters here, since they're optional.

>+function outputTree(aAccessible, aHighlightList)
>+{
>+  function genTree(aAccessible, aHighlightList)

Move this out to a separate method, please, and name it genTreeRows or generateTreeChildren or generateTreeData or something.

>+    treecellID.setAttribute("label",
>+                            (accessible.DOMNode.id ? accessible.DOMNode.id : ""));

You'll get a warning about undefined property accessible.DOMNode.id if the node isn't an element; non-element nodes aren't required to have an "id" property.

>+  var treecols = document.createElement("treecols");
>+  var treecolRole = document.createElement("treecol");
>+  treecolRole.setAttribute("id", "outputtree-role");
>+  treecolRole.setAttribute("label", gBundle.getString("role"));
>+  treecolRole.setAttribute("flex", "2");
>+  treecolRole.setAttribute("primary", "true");
>+  treecolRole.setAttribute("persist", "width,hidden,ordinal");
>+  treecols.appendChild(treecolRole);
>+  var splitter = document.createElement("splitter");
>+  splitter.setAttribute("class", "tree-splitter");
>+  treecols.appendChild(splitter);
>+  var treecolName = document.createElement("treecol");
>+  treecolName.setAttribute("id", "outputtree-name");
>+  treecolName.setAttribute("label", gBundle.getString("name"));
>+  treecolName.setAttribute("flex", "2");
>+  treecolName.setAttribute("persist", "width,hidden,ordinal");
>+  treecols.appendChild(treecolName);
>+  splitter = document.createElement("splitter");
>+  splitter.setAttribute("class", "tree-splitter");
>+  treecols.appendChild(splitter);
>+  var treecolTag = document.createElement("treecol");
>+  treecolTag.setAttribute("id", "outputtree-nodename");
>+  treecolTag.setAttribute("label", gBundle.getString("nodeName"));
>+  treecolTag.setAttribute("flex", "1");
>+  treecolTag.setAttribute("persist", "width,hidden,ordinal");
>+  treecols.appendChild(treecolTag);
>+  splitter = document.createElement("splitter");
>+  splitter.setAttribute("class", "tree-splitter");
>+  treecols.appendChild(splitter);
>+  var treecolID = document.createElement("treecol");
>+  treecolID.setAttribute("id", "outputtree-id");
>+  treecolID.setAttribute("label", gBundle.getString("id"));
>+  treecolID.setAttribute("flex", "2");
>+  treecolID.setAttribute("persist", "width,hidden,ordinal");
>+  treecols.appendChild(treecolID);

Please put some linebreaks in here.

>+      <!-- Event list -->
>+      <vbox>
>+        <label value="&handledEvents.label;" control="olAccessibleEvents"/>

This tree lists all events, not just the ones that executed handlers.

>+          <textbox id="welHandlerEditor" multiline="true" rows="8" flex="1"/>

Does setting the rows have any effect when this is a flexible element?

>+treechildren::-moz-tree-row(highlight) {
>+  background-color: #FEEECD;
>+}

If we're going to set this, it means we also need to set the text color, in case the user has their text set to this color.  If their normal background color or selected row background color is already close to this, I guess they're just unlucky.

Comment 35

[alexander :surkov (:asurkov)]

(In reply to comment #34)

> >+    var outputList = this.mSubject[" eventHandlerOutput "];
> 
> I'm not sure that continuing to put data in properties of the nodes themselves
> (and exposing it to content) just to pass it between viewers is a good idea.

It's not nice but I don't keep in mind a better way.

> >+  onWatchViewItemSelected: function onWatchViewItemSelected()
> >+  {
> >+    this.mWatchView.updateHandlerEditor();
> >+    this.mWatchView.updateHandlerStateColumn();
> 
> Why does the handler state column need updating here?  In which cases would
> this be expected to change anything?

to update checkbox of the event handler.

> >+var gEventHandlerOutput = [ ];
> ...
> >+  // Execute user handlers.
> >+  gEventHandlerOutput = [ ];
> 
> What's the point of initializing it twice?

I just initialize variable when it's defined to show it's an array and it's initialized, just in case. The second time is I clear the array before output dumping for new handled event.

>  I'm not sure making this global is
> the best idea.

me neither, but I need to use it from object and from global functions, so perhaps it's not nice but it's not so ugly and works.

> >+      var f = Function("event", "target", "output", expr);
> >+      var result = f(event, accessible, output);
> 
> Should these be in a sandbox?  Why pass output as a parameter, but not
> accRetrieval or any of the other functions?

I think output as a parameter is artifact, I should remove it.

> >-AccessibleEventsView.prototype.getDOMNode =
> >-function getDOMNode(aRow)
> >+AccessibleEventsView.prototype.getObject =
> >+function getObject(aRow)
> 
> Why the name change?  "getObject" is kind of ambiguous.
> 
> >-  DOMNode[" accessible "] = accessNode;
> >-  DOMNode[" accessible event "] = event;
> >-  return DOMNode;
> >+  var object = DOMNode || { __proto__: accessNode };
> 
> What's this about?

because it can return either DOM node or accessible.

> >   this.isEditable = function watchview_isEditable(aRowIndex, aCol)
> >   {
> >-    return true;
> >+    if (aCol.id == "welIsWatched" || aCol.id == "welIsHandlerEnabled") {
> >+      return true;
> 
> Should to return false here for event group rows when the column is
> welIsHandlerEnabled, instead of just ignoring it in setCellValue.
> 
> >+    else if (aCol.id == "welIsHandlerEnabled") {
> >+      // Event groups aren't allowed to register handlers for their items.
> >+      if (this.isContainer(aRowIndex)) {
> >+        return;
> >+      }

Ok.

> >+
> >+      var newValue = aValue == "true";
> >+      this.updateHandlerStateColumn(newValue, aRowIndex, aCol);
> >+
> >+      this.updateHandlerEditor();
> 
> Does the editor need to be updated?  Nothing will change as far as I can tell.

you're right.

> >+  this.updateHandlerStateColumn =
> >+    function watchview_updateHandlerStateColumn(aValue, aRowIdx, aCol)
> 
> Why take the aCol parameter if this is only used to update welIsHandlerEnabled?

right

>  Also, it would be nice to document the parameters here, since they're
> optional.

done

> 
> >+      <!-- Event list -->
> >+      <vbox>
> >+        <label value="&handledEvents.label;" control="olAccessibleEvents"/>
> 
> This tree lists all events, not just the ones that executed handlers.
> 
> >+          <textbox id="welHandlerEditor" multiline="true" rows="8" flex="1"/>
> 
> Does setting the rows have any effect when this is a flexible element?

it sounds it's respected when you expand it for the first time.

> 
> >+treechildren::-moz-tree-row(highlight) {
> >+  background-color: #FEEECD;
> >+}
> 
> If we're going to set this, it means we also need to set the text color, in
> case the user has their text set to this color.  If their normal background
> color or selected row background color is already close to this, I guess
> they're just unlucky.

Can user preferences be used to change color and background-color of XUL trees or it can be done by skins only? If the last is valid only then the skin should take care about this I think.

Comment 36

[alexander :surkov (:asurkov)]

Attachment: patch10

Comment 37

[alexander :surkov (:asurkov)]

(In reply to comment #35)

> > >+      var f = Function("event", "target", "output", expr);
> > >+      var result = f(event, accessible, output);
> > 
> > Should these be in a sandbox?  Why pass output as a parameter, but not
> > accRetrieval or any of the other functions?
> 
> I think output as a parameter is artifact, I should remove it.

I forgot to fix this in patch10 but I did a change locally.

Comment 38

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 482468 [details] [diff] [review]
patch10

(In reply to comment #35)
> > >+    var outputList = this.mSubject[" eventHandlerOutput "];
> > 
> > I'm not sure that continuing to put data in properties of the nodes themselves
> > (and exposing it to content) just to pass it between viewers is a good idea.
> 
> It's not nice but I don't keep in mind a better way.

It's also a security problem.  I can think of a straightforward way for a page to execute arbitrary code with chrome privileges, since we have implementation details exposed to content through these properties.  (The exploit I have in mind also relies on how outputTree works.)

I was already thinking that outputTree should work differently.  I'm thinking that the tree generation should all take place in the Accessible Event viewer.  The handler should just tell the viewer which objects it should generate trees for, and it will do it.  That should take care of the exploit I have in mind, but I can't be sure it's entirely safe.

> > >+  onWatchViewItemSelected: function onWatchViewItemSelected()
> > >+  {
> > >+    this.mWatchView.updateHandlerEditor();
> > >+    this.mWatchView.updateHandlerStateColumn();
> > 
> > Why does the handler state column need updating here?  In which cases would
> > this be expected to change anything?
> 
> to update checkbox of the event handler.

I see.  updateHandlerStateColumn actually updates the column and the checkbox above the editor.  Please rename it to just updateHandlerState or something.

> 
> > >+var gEventHandlerOutput = [ ];
> > ...
> > >+  // Execute user handlers.
> > >+  gEventHandlerOutput = [ ];
> > 
> > What's the point of initializing it twice?
> 
> I just initialize variable when it's defined to show it's an array

That seems unnecessary.

> >  I'm not sure making this global is
> > the best idea.
> 
> me neither, but I need to use it from object and from global functions, so
> perhaps it's not nice but it's not so ugly and works.

It wouldn't be the case if it were evaluated in a sandbox, though.

> > >-  DOMNode[" accessible "] = accessNode;
> > >-  DOMNode[" accessible event "] = event;
> > >-  return DOMNode;
> > >+  var object = DOMNode || { __proto__: accessNode };
> > 
> > What's this about?
> 
> because it can return either DOM node or accessible.

Could you add a short comment in the source about what it's supposed to accomplish?

> > >+treechildren::-moz-tree-row(highlight) {
> > >+  background-color: #FEEECD;
> > >+}
> > 
> > If we're going to set this, it means we also need to set the text color, in
> > case the user has their text set to this color.  If their normal background
> > color or selected row background color is already close to this, I guess
> > they're just unlucky.
> 
> Can user preferences be used to change color and background-color of XUL trees
> or it can be done by skins only? If the last is valid only then the skin should
> take care about this I think.

These colors come from the theme.  Note that for most users, the theme is just going to report the system colors.


> -<?xml-stylesheet href="chrome://inspector/skin"?>
> +<?xml-stylesheet href="chrome://inspector/skin/viewers/accessibleEvent/accessibleEvent.css"?>

Break for 80 columns.
 
> +  treecellID.setAttribute("label",
> +                          ("id" in accessible.DOMNode ? accessible.DOMNode.id : ""));

Break for 80 columns.

Comment 39

[alexander :surkov (:asurkov)]

(In reply to comment #38)
> Comment on attachment 482468 [details] [diff] [review]
> patch10
> 
> (In reply to comment #35)
> > > >+    var outputList = this.mSubject[" eventHandlerOutput "];
> > > 
> > > I'm not sure that continuing to put data in properties of the nodes themselves
> > > (and exposing it to content) just to pass it between viewers is a good idea.
> > 
> > It's not nice but I don't keep in mind a better way.
> 
> It's also a security problem.  I can think of a straightforward way for a page
> to execute arbitrary code with chrome privileges, since we have implementation
> details exposed to content through these properties.  (The exploit I have in
> mind also relies on how outputTree works.)

eventHandlerOutput is an array of strings and DOM nodes (which are imported to new document), I can't see how it's different from " accessible event " property for example. If setting the property to JS object and passing it in different window to use it there is security problem in general then it looks for me as different bug. Colby, could you file security bug where you can describe security issues, otherwise I'm not sure I follow you.

> I was already thinking that outputTree should work differently.  I'm thinking
> that the tree generation should all take place in the Accessible Event viewer. 
> The handler should just tell the viewer which objects it should generate trees
> for, and it will do it.  That should take care of the exploit I have in mind,
> but I can't be sure it's entirely safe.

Do you keep in mind an exploit related with importing node from one document to another?

I like the way outputTree works because i allows me to add new output function and I need to add it into accessibleEvents view and do not change accessibleEvent view. I like the idea to import DOM nodes from one view to another one. On the another hand it allows a user to create DOM node in accessible event handler to output them into accessibleEvent view.

> I see.  updateHandlerStateColumn actually updates the column and the checkbox
> above the editor.  Please rename it to just updateHandlerState or something.

ok

> > I just initialize variable when it's defined to show it's an array
> 
> That seems unnecessary.

right, just my preference, but I don't mind if you want me to change this.

> 
> > >  I'm not sure making this global is
> > > the best idea.
> > 
> > me neither, but I need to use it from object and from global functions, so
> > perhaps it's not nice but it's not so ugly and works.
> 
> It wouldn't be the case if it were evaluated in a sandbox, though.

right, but that means I should pass all of them as arguments to the function. It's not flexible in terms I should change arguments once I add new output function.

> > > >+  var object = DOMNode || { __proto__: accessNode };
> > > 
> > > What's this about?
> > 
> > because it can return either DOM node or accessible.
> 
> Could you add a short comment in the source about what it's supposed to
> accomplish?

sure.

> > Can user preferences be used to change color and background-color of XUL trees
> > or it can be done by skins only? If the last is valid only then the skin should
> > take care about this I think.
> 
> These colors come from the theme.  Note that for most users, the theme is just
> going to report the system colors.

so the theme doesn't care about extensions and etc?

Comment 40

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #39)
> If setting the property to JS object and passing it in
> different window to use it there is security problem in general then it looks
> for me as different bug. Colby, could you file security bug where you can
> describe security issues, otherwise I'm not sure I follow you.

I haven't filed a security bug, because it's not an exploit that works with current DOM Inspector builds; it's an exploit that we'd be vulnerable to after the introduction of this patch.

> Do you keep in mind an exploit related with importing node from one document to
> another?

Yes, this is exactly what I was referring to.  With this patch, we'd have XUL nodes with chrome privileges hanging off the properties of content nodes.  Content script could easily take advantage of this.

> > > Can user preferences be used to change color and background-color of XUL trees
> > > or it can be done by skins only? If the last is valid only then the skin should
> > > take care about this I think.
> > 
> > These colors come from the theme.  Note that for most users, the theme is just
> > going to report the system colors.
> 
> so the theme doesn't care about extensions and etc?

Huh?  If the user has their default window text color in their OS settings set to the same color as the one you use to highlight the nodes, or close to it, they will be unable to read the text in those rows.  Therefore, we'll need to add a CSS rule to override their OS text color for those rows.

Comment 41

[alexander :surkov (:asurkov)]

(In reply to comment #40)

> > Do you keep in mind an exploit related with importing node from one document to
> > another?
> 
> Yes, this is exactly what I was referring to.  With this patch, we'd have XUL
> nodes with chrome privileges hanging off the properties of content nodes. 
> Content script could easily take advantage of this.

Is it existing bug of DOMdocument::importNode? Btw, is the untrusted script allowed to operate with chrome JS objects?

Comment 42

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #35)
> > I'm not sure that continuing to put data in properties of the nodes themselves
> > (and exposing it to content) just to pass it between viewers is a good idea.
> 
> It's not nice but I don't keep in mind a better way.

A better approach would probably be to finally fix the other viewers to just accept nsIAccessibleEvent objects.  Either that, or we should change the viewer contract to give a list of potential elements instead of just one, and the filtering that the registry performs would try to fall back to the (i + 1)-th item if the i-th item doesn't pass the viewer's filter.

Comment 43

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #41)
> Is it existing bug of DOMdocument::importNode?

No.  importNode works fine.  It's the fact that XUL nodes with chrome privileges whose ownerDocument is accessibleEvents.xul would be able to be seen from content.

> Btw, is the untrusted script
> allowed to operate with chrome JS objects?

No.  That's a dead end and raises security exceptions.  But the untrusted script can still manipulate the accessibleEvents.xul document and its nodes.

Comment 44

[alexander :surkov (:asurkov)]

(In reply to comment #42)
> (In reply to comment #35)
> > > I'm not sure that continuing to put data in properties of the nodes themselves
> > > (and exposing it to content) just to pass it between viewers is a good idea.
> > 
> > It's not nice but I don't keep in mind a better way.
> 
> A better approach would probably be to finally fix the other viewers to just
> accept nsIAccessibleEvent objects.

That doesn't work for event outputs.

>  Either that, or we should change the viewer
> contract to give a list of potential elements instead of just one, and the
> filtering that the registry performs would try to fall back to the (i + 1)-th
> item if the i-th item doesn't pass the viewer's filter.

yeah, that should work, should I file a bug?

Comment 45

[alexander :surkov (:asurkov)]

(In reply to comment #43)
> (In reply to comment #41)
> > Is it existing bug of DOMdocument::importNode?
> 
> No.  importNode works fine.  It's the fact that XUL nodes with chrome
> privileges whose ownerDocument is accessibleEvents.xul would be able to be seen
> from content.

I must missing something. How does that happens? Is it because DOM node is stored in JS object or because it was imported from another document? Could you give me possible scenario?

Comment 46

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #44)
> (In reply to comment #42)
> > (In reply to comment #35)
> > > > I'm not sure that continuing to put data in properties of the nodes themselves
> > > > (and exposing it to content) just to pass it between viewers is a good idea.
> > > 
> > > It's not nice but I don't keep in mind a better way.
> > 
> > A better approach would probably be to finally fix the other viewers to just
> > accept nsIAccessibleEvent objects.
> 
> That doesn't work for event outputs.

Well no, but then you could put output on the nsIAccessibleEvent objects in the same way you're doing with to the nsIDOMNodes right now.  Then that stuff wouldn't be accessible to the content.  But I think the second approach would be cleaner:

> >  Either that, or we should change the viewer
> > contract to give a list of potential elements instead of just one, and the
> > filtering that the registry performs would try to fall back to the (i + 1)-th
> > item if the i-th item doesn't pass the viewer's filter.
> 
> yeah, that should work, should I file a bug?

Go for it.

Comment 47

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #45)
> (In reply to comment #43)
> > (In reply to comment #41)
> > > Is it existing bug of DOMdocument::importNode?
> > 
> > No.  importNode works fine.  It's the fact that XUL nodes with chrome
> > privileges whose ownerDocument is accessibleEvents.xul would be able to be seen
> > from content.
> 
> I must missing something. How does that happens? Is it because DOM node is
> stored in JS object or because it was imported from another document? Could you
> give me possible scenario?

theNodeAfterTheOutputTreeHandlerFired[" eventHandlerOutput "][0]
  .ownerDocument.documentElement.setAttribute(
    "onfocus", "alert('this is malicious code')"
  )

Comment 48

[Colby Russell :crussell (no longer Mozilla-ing)]

Note that even if it weren't the case that you could execute malicious code through the on* event attributes, content having free reign over the accessibleEvents.xul document is undesirable, because the page could completely trash it the Accessible Events pane.

Comment 49

[alexander :surkov (:asurkov)]

so the problem can be fixed if output node is created by owner document of event target node, right?

Comment 50

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #49)
> so the problem can be fixed if output node is created by owner document of
> event target node, right?

I don't know.  Can content create XUL elements?  Even then, I'm not sure about how secure it would be to import a content node into a chrome document—even if the node is never imported into the event target's owner document tree (are there other ways the content would be able to get at the node?)—but it sure doesn't sound like a good idea.

Comment 51

[Boris Zbarsky [:bzbarsky]]

> Can content create XUL elements?

In what sense?

Comment 52

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #51)
> > Can content create XUL elements?
> 
> In what sense?

Here we'd have all kind of inspectable documents including including unprivileged Web pages which we're watching for accessible events.  In this bug, surkov is attempting to programmatically build a XUL tree.  From comment 49, I guess he wants to use the inspected document to create the trees, treerows, et cetera, then use importNode from a chrome document and append it at a given location.

Is that right?

It looks like the "content creating XUL elements" is already a dead end though.

I CCed you, Boris, mostly about the notion of importing nodes from unsafe documents into chrome, which seems like a really bad idea.  In this case, privileged scripts would use the unprivileged inspected document to create the element, then the privileged chrome would set a property a node in the inspected document, then *another* chrome document would look at that node the property was set on, and import its value (the created node) into the chrome documents own tree.

In any case, I'd greatly prefer we just go with one of the approaches from comment 42 instead of these hacky ways to accomplish data passing, though.

Comment 53

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #52)
> privileged chrome would set a property a node

That should be "...set a property *on* a node with that created element as its value..."

Comment 54

[Boris Zbarsky [:bzbarsky]]

> then the privileged chrome would set a property a node in the
> inspected document, then *another* chrome document would look at that node the
> property was set on, and import its value (the created node) into the chrome
> documents own tree.

Ah, ok.  Yeah, Don't Do That (TM).

Comment 55

[alexander :surkov (:asurkov)]

Attachment: patch11

Comment 56

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 484241 [details] [diff] [review]
patch11

I noticed that if you add a handler to an event, then click on an event group, the handler editor gets disabled, but its value doesn't get set to empty.  You can still see the source for the handler you were previously editing.

Before I get into this patch, the first issue:

> +          tree.treeBoxObject.view = output.view;

Layout code calling into (potentially) content-level scripts still makes me uneasy.  In this case, though, there are still obvious security issues.  Content-level scripts can still obtain references to chrome-level nodes by overriding any of the view's methods that get XUL elements passed in as parameters, e.g., setTree, getCellText, etc.

But what happened to just passing the accessible event to the object pane and fixing up the other viewers to derive the node from the event (comment 42)?

> + * The Original Code is mozilla.org code.

Not DOM Inspector?  Also, you should make sure you use the license boilerplate from http://www.mozilla.org/MPL/boilerplate-1.1/ for all the new files, instead of copying them from other files.  I see that at least one of them doesn't match the boilerplate from the URL; I didn't check the others yet.

> +///////////////////////////////////////////////////////////////////////////////
> +//// inDataTreeView. nsITreeView interface

"Implements nsITreeView"?

> +inDataTreeView.prototype.isContainer =
> +function isContainer(aRowIdx)
> +{
> +  var row = this.rowAt(aRowIdx);
> +  return row ? row.node.children.length > 0 : false;
> +}
...
> +inDataTreeView.prototype.isContainerEmpty =
> +function isContainerEmpty(aRowIdx)
> +{
> +  var row = this.rowAt(aRowIdx);
> +  return row.node.children.length == 0;
> +}

If you're going to say that rows are containers only if they have children, you can define isContainerEmpty as !isContainer.

> +inDataTreeView.prototype.toggleOpenState =
> +function toggleOpenState(aRowIdx)
...
> +inDataTreeView.prototype.appendChild =
> +function inDataTreeView_appendChild(aParent, aData)

Sometimes you prefix the function names and sometimes you don't.  Please prefix them all in new files or files which already prefix some methods.

> +///////////////////////////////////////////////////////////////////////////////
> +//// inDataTreeView. Tree utils.
> +
> +/**
> + * Expands a tree node on the given row.
> + *
> + * @param aRow - row index.
> + */

These should look like

/**
 * This is a description of the method.  It is not followed by a blank line.
 * @param aParam
 *        This is a description of aParam.
 * @param aOtherParam [optional]
 *        This is a description of aOtherParam.  The difference in behavior
 *        when aOtherParam is omitted/included should also be mentioned.
 */

There are several other occurrences like this throughout the patch.

> +inDataTreeView.prototype.expandNode =
> +function expandNode(aRowIdx)
> +{
> +  var row = this.rowAt(aRowIdx);
> +  if (!row)
> +    return;

Brace this, please.  There are other occurrences like this.

> +  for (var i = 0; i < kidCount; ++i) {
> +    var newRow = new inDataTreeViewRow(kids[i]);
> +    this.mRows.splice(aRowIdx + i + 1, 0, newRow);

This is O(kidCount^2) data movements.  It's trivial to achieve O(kidCount) data movements.  See styleRules.js.

> +inDataTreeView.prototype.dataAt =
> +function rowAt(aRowIdx)

Wrong function name here.

> +function inDataTreeViewNode(aData)
> +{
> +  this.parent = null;
> +  this.children = [];
> +
> +  this.level = 0;
> +  this.data = aData;
> +}
> +
> +function inDataTreeViewRow(aNode)
> +{
> +  this.node = aNode;
> +  this.isOpen = false;
> +}

Nit: maybe level should be an attribute of the row instead of the node?

> +          outputElm.JSView = output.view;

(What's this JSView about, anyway?)

> +        } else {

else on a new line.

> -<?xml-stylesheet href="chrome://inspector/skin"?>
> +<?xml-stylesheet
> +  href="chrome://inspector/skin/viewers/accessibleEvent/accessibleEvent.css"?>

Two more spaces before the href if you're going to wrap this, please.
 
> +    if (aCol.id == "welIsWatched" ||
> +        aCol.id == "welIsHandlerEnabled" && !this.isContainer(aRowIndex)) {

Parenthesize these, please.  (The line-breaking is also a little weird.)

> +  /**
> +   * Updates state of handler (enabled or disabled) and UI displaying the state.
> +   *
> +   * @param  aValue   [in, optional] new value of handler state column at
> +   *                    the given row index, if undefined then data wasn't
> +   *                    changed, needs to update UI
> +   * @param  aRowIdx  [in, optional] the given row index, if missed then
> +   *                    current row index is used
> +   */

See previous comments about formatting.  Also, you don't need the "in" stuff.

> +  /**
> +   * Listen 'input' event from handler source textbox to record the handler.
> +   */

"Listen for 'input'..."

> +<!--
> +#if 0

We don't need to preprocess these out, per comment 29.

> +<!ENTITY descrHandlerOutput.label "Event handler output: ">

Need to remove ru localization from the makefile.

> +  <!ENTITY helpers.descr "The following objects and functions can be used within handler editor:">
> +  

There's a space on this line.

Comment 57

[alexander :surkov (:asurkov)]

(In reply to comment #56)

> But what happened to just passing the accessible event to the object pane and
> fixing up the other viewers to derive the node from the event (comment 42)?

iirc there are problems by inheritance from native object and we did this before here already.

I think it's better to allow subject be an object like
subject = {
  node: null,
  accessible: null,
  accevent: null,
  acceventOutput: []
};

and then viewer can choose the property it supports. How does it sound?

Comment 58

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #57)
> (In reply to comment #56)
> 
> > But what happened to just passing the accessible event to the object pane and
> > fixing up the other viewers to derive the node from the event (comment 42)?
> 
> iirc there are problems by inheritance from native object and we did this
> before here already.

I wasn't talking about trying to get the DOM node to use the event as its proto or for the event to use the DOM node as its proto.  I was saying to just set mSelection to the event, then in the DOM Nodes viewer, CSS Rules, etc., you would just obtain the DOM node from the event in the same way that the Accessible Events viewer right now is getting it.

> I think it's better to allow subject be an object like
> subject = {
>   node: null,
>   accessible: null,
>   accevent: null,
>   acceventOutput: []
> };
> 
> and then viewer can choose the property it supports. How does it sound?

Mmmm, I don't think harcoding a multi-valued object with those keys is such a good design.  It's really easy for the Accessibility Events viewer to implement what you're saying without having to change much at all (the number of lines to achieve that would be roughly the same as the quoted portion above), but it really complicates all the other viewers and contracts.

I have filed bug 606112.

Comment 59

[alexander :surkov (:asurkov)]

(In reply to comment #58)
> (In reply to comment #57)
> > (In reply to comment #56)
> > 
> > > But what happened to just passing the accessible event to the object pane and
> > > fixing up the other viewers to derive the node from the event (comment 42)?
> > 
> > iirc there are problems by inheritance from native object and we did this
> > before here already.
> 
> I wasn't talking about trying to get the DOM node to use the event as its proto
> or for the event to use the DOM node as its proto.  I was saying to just set
> mSelection to the event, then in the DOM Nodes viewer, CSS Rules, etc., you
> would just obtain the DOM node from the event in the same way that the
> Accessible Events viewer right now is getting it.

I might missing something but how event output is going to be passed to accessibleView?

> > I think it's better to allow subject be an object like
> > subject = {
> >   node: null,
> >   accessible: null,
> >   accevent: null,
> >   acceventOutput: []
> > };
> > 
> > and then viewer can choose the property it supports. How does it sound?
> 
> Mmmm, I don't think harcoding a multi-valued object with those keys is such a
> good design.  It's really easy for the Accessibility Events viewer to implement
> what you're saying without having to change much at all (the number of lines to
> achieve that would be roughly the same as the quoted portion above), but it
> really complicates all the other viewers and contracts.
> 
> I have filed bug 606112.

Yes, it will complicate contracts and getting an object inside a viewer, but that's shouldn't be very awful, just few lines of code, no?

Comment 60

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #59)
> I might missing something but how event output is going to be passed to
> accessibleView?

event[" handlerOutput "] = eventHandlerOutput?

> > > I think it's better to allow subject be an object like
> > > subject = {
> > >   node: null,
> > >   accessible: null,
> > >   accevent: null,
> > >   acceventOutput: []
> > > };
> > > 
> > > and then viewer can choose the property it supports. How does it sound?
> > 
> > Mmmm, I don't think harcoding a multi-valued object with those keys is such a
> > good design.  It's really easy for the Accessibility Events viewer to implement
> > what you're saying without having to change much at all (the number of lines to
> > achieve that would be roughly the same as the quoted portion above), but it
> > really complicates all the other viewers and contracts.
> > 
> > I have filed bug 606112.
> 
> Yes, it will complicate contracts and getting an object inside a viewer, but
> that's shouldn't be very awful, just few lines of code, no?

No, and that's my point.  It's "just a few lines" for the accessibleEvents viewer, but it also requires adding lines in every other viewer and the panelset/panel bindings.  It requires touching and complicating lots of other parts of code just to allow the accessibleEvents viewer to take it easy.  If that were the only way to accomplish what you want, sure, that'd be fine, but
a) all that extra work in the other viewers doesn't benefit those viewers, and
b) it happens to be very straightforward to achieve the same thing while keeping the details contained within the areas of code it's relevant to.

Did you look at #3 in bug 606122?  It's very similar to what you're asking to do here, but it keeps things simple for the other areas of code that don't care about knowing how the Accessible Events viewer is implemented.

In the accessibleEvents viewer:

  this.pane.panelset.getPanel(1).params = {
    accessible: accessNode,
    accessibleEvent: event,
    accessibleEventHandlerOutput: eventHandlerOutput
  };

In the accessibleEvent viewer:

  var outputList = this.pane.params.accessibleEventHandlerOutput;

The viewer registry would need to be changed so you could (easily) get at the params object, though.

Comment 61

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #60)
> Did you look at #3 in bug 606122?

Argh.  Bug 606112.

Comment 62

[alexander :surkov (:asurkov)]

(In reply to comment #60)
> (In reply to comment #59)
> > I might missing something but how event output is going to be passed to
> > accessibleView?
> 
> event[" handlerOutput "] = eventHandlerOutput?

Aren't JS wrappers for XPCOM objects shared between content and chrome? So if content listen accessibles events (assume it has privelelges for this) then it has own JS wrapper that hasn't " handlerOutput" " property, right?

Why is this idea different from passing an object? Shouldn't I change DOM node view and others to let them work with accessible event instead of DOM node?

> Did you look at #3 in bug 606122?  It's very similar to what you're asking to
> do here, but it keeps things simple for the other areas of code that don't care
> about knowing how the Accessible Events viewer is implemented.
> 
> In the accessibleEvents viewer:
> 
>   this.pane.panelset.getPanel(1).params = {
>     accessible: accessNode,
>     accessibleEvent: event,
>     accessibleEventHandlerOutput: eventHandlerOutput
>   };
> 
> In the accessibleEvent viewer:
> 
>   var outputList = this.pane.params.accessibleEventHandlerOutput;
> 
> The viewer registry would need to be changed so you could (easily) get at the
> params object, though.

Sounds ok, perhaps I should move this way.

Comment 63

[alexander :surkov (:asurkov)]

(In reply to comment #60)

> In the accessibleEvents viewer:
> 
>   this.pane.panelset.getPanel(1).params = {
>     accessible: accessNode,
>     accessibleEvent: event,
>     accessibleEventHandlerOutput: eventHandlerOutput
>   };

this makes me bother a bit, what if somebody else will want to set up params, it would be safer to provide setParam/getParam API.

but I'm fine with params approach as it is.

Comment 64

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #62)
> if content listen accessibles events (assume it has privelelges for this)

Does it?  I didn't think it did.

(In reply to comment #63)
> this makes me bother a bit, what if somebody else will want to set up params,
> it would be safer to provide setParam/getParam API.

It shouldn't be a problem.  Both the document viewer and the object viewer will be accessibility viewers.  Nothing else will be using params at the time.  If, for example, other viewers are using it for data passing, they won't be open, and they'll reset it when they are open.  At that point, the Accessible Tree and Accessible Events viewer won't need for the params that they set to stick around.

Comment 65

[alexander :surkov (:asurkov)]

(In reply to comment #56)

> > + * The Original Code is mozilla.org code.
> 
> Not DOM Inspector?  Also, you should make sure you use the license boilerplate
> from http://www.mozilla.org/MPL/boilerplate-1.1/ for all the new files, instead
> of copying them from other files.  I see that at least one of them doesn't
> match the boilerplate from the URL; I didn't check the others yet.

ok

> > +function inDataTreeViewNode(aData)
> > +{
> > +  this.parent = null;
> > +  this.children = [];
> > +
> > +  this.level = 0;
> > +  this.data = aData;
> > +}
> > +
> > +function inDataTreeViewRow(aNode)
> > +{
> > +  this.node = aNode;
> > +  this.isOpen = false;
> > +}
> 
> Nit: maybe level should be an attribute of the row instead of the node?

the level is not presentation characteristic so it's fine to keep in node I think.

> > +          outputElm.JSView = output.view;
> 
> (What's this JSView about, anyway?)

I don't know :)

> > +    if (aCol.id == "welIsWatched" ||
> > +        aCol.id == "welIsHandlerEnabled" && !this.isContainer(aRowIndex)) {
> 
> Parenthesize these, please.  (The line-breaking is also a little weird.)

I added some (), though not sure you meant what I did.

Comment 66

[alexander :surkov (:asurkov)]

Attachment: patch12

Comment 67

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 485748 [details] [diff] [review]
patch12

>        <method name="setSubject">
>          <parameter name="aObject"/>
>          <body><![CDATA[
>            this.mSubject = aObject;
> -          this.mParams = null;

Nulling out the params is necessary.

You'll need to change the current params setter to something sort of like this:

// Don't let the params get nulled out before anyone gets to use them.
this.mParamsPending = true;
this.mParams = aVal

Then change the above removed line to something like:

// Check if someone wanted to pass some parameters in.
if (this.mParamsPending) {
  this.mParamsPending = false;
}
else {
  this.mParams = null;
}

Otherwise, when inspectDOMNode from hooks.js is used, if the user begins inspecting another document, then goes back to inspecting the document originally opened with inspectDOMNode (or a document which contains that node in a subtree), that node will get shown in the tree again.  Also, all documents that get inspected in that window and which don't have that node in their subtree will do unnecassary computation upfront trying to locate it, even though it's not there.  If this is what you were referring to in comment 63, sorry, I misunderstood what you were talking about.
 
> diff --git a/resources/content/jsutil/xul/inDataTreeView.js b/resources/content/jsutil/xul/inDataTreeView.js
> new file mode 100644

Any particular reason this is in jsutil/ instead of staying in accessibleEvents/?  It's only used there, and it's not very general purpose.  For example, it assumes there is only one 0-level row: 
> +inDataTreeView.prototype.hasNextSibling =
> +function inDataTreeView_hasNextSibling(aRowIdx, aAfterRowIdx)
> +{
> +  var row = this.rowAt(aRowIdx);
> +  if (!row || !row.node.parent) {
> +    return false;
> +  }

> +  var lastIdx = row.node.parent.children.length - 1;
> +  if (lastIdx < 0) {
> +    return false;
> +  }

This check isn't necessary.

> +  for (var i = this.rowCount - 1; i > aRowIdx; --i) {
> +    this.mRows[i + kidCount] = this.mRows[i];
> +  }

Nit: let instead of var in for loops.

> +  var removeCount = 0;
> +  var rowLevel = row.node.level;
> +  for (var i = aRowIdx + 1; i < this.rowCount; i++) {
> +    if (this.rowAt(i).node.level <= rowLevel) {
> +      break;
> +    }
> +    removeCount++;

Would

var removeCount = this.rowAt(i).node.children.length;

not work?

> +inDataTreeView.prototype.rowAt =
> +function inDataTreeView_rowAt(aRowIdx)
...
> +inDataTreeView.prototype.dataAt =
> +function inDataTreeView_dataAt(aRowIdx)

Nit: getRowAt and getDataAt?

> -    this.mSelection = this.mView.getDOMNode(idx);
> +    var object = this.mView.getObject(idx);
> +
> +    // Set subject for right panel view.
> +    var subject = object.node || object.accessnode;
> +    if (object.node) {
> +      subject[" accessible "] = object.accessnode;
> +    }

It would be really nice if the other viewers stopped doing this stuff, too (bug 606112).

> +    // Set parameters for right panel view.
> +    this.pane.panelset.getPanel(1).params = {
> +      accessibleEvent: object.event,
> +      accessibleEventHandlerOutput: object.eventHandlerOutput
> +    };

Need to check panelset.panelCount here, so you don't break object.xul.

> +  /**
> +   * Open hide handler editor.

"Open/hide..."

> +/**
> + * Return object used as a subject for the right panel. It could be either

"Return object to be used..."

> +  /**
> +   * Updates state of handler (enabled or disabled) and UI displaying the state.
> +   * @param  aValue [optional]
> +   *         New value of handler state column at the given row index, if
> +   *         undefined then data wasn't changed, needs to update UI
> +   * @param  aRowIdx [optional]
> +   *         The given row index, if missed then current row index is used

You don't need two spaces between @param and the parameter name/description.  You don't need any blank lines after the method description, either.  (See comment 56.)

> +function inAccTreeView(aAccessible, aHighlightList)
> +{
> +  this.__proto__ = new inDataTreeView();
> +
> +  // nsITreeView
> +  this.getRowProperties = function getRowProperties(aRowIdx, aProperties)
...
> +  this.getCellProperties = function getCellProperties(aRowIdx, aCol, aProperties)
...
> +  this.generateChildren = function generateChildren(aAccessible, aHighlightList,

Please change this to the more standard form.  (I.e.,

function inAccTreeView {...}
inAccTreeView.prototype = ...
inAccTreeView.prototype.getRowProperties = function ...
inAccTreeView.prototype.getCellProperties = function ...
inAccTreeView.prototype.generateChildren = function ...)

It doesn't look like the closures are needed here.

> +  <!ENTITY outputTree.descr "prints tree for the given accessible object highlighting accessibles from the given array">

Comma after "object".

Don't forget to update or remove the ru localization from the makefile.

Comment 68

[alexander :surkov (:asurkov)]

(In reply to Colby Russell :crussell from comment #67)

> > diff --git a/resources/content/jsutil/xul/inDataTreeView.js b/resources/content/jsutil/xul/inDataTreeView.js
> > new file mode 100644
> 
> Any particular reason this is in jsutil/ instead of staying in
> accessibleEvents/?  It's only used there, and it's not very general purpose.

I'd like to use in different accessible viewers

> For example, it assumes there is only one 0-level row: 
> > +inDataTreeView.prototype.hasNextSibling =

that can be fixed when we get a usecase

> > +  var removeCount = 0;
> > +  var rowLevel = row.node.level;
> > +  for (var i = aRowIdx + 1; i < this.rowCount; i++) {
> > +    if (this.rowAt(i).node.level <= rowLevel) {
> > +      break;
> > +    }
> > +    removeCount++;
> 
> Would
> 
> var removeCount = this.rowAt(i).node.children.length;
> 
> not work?

no, because of nested open items

Comment 69

[alexander :surkov (:asurkov)]

Attachment: patch 13

Comment 70

[alexander :surkov (:asurkov)]

(In reply to Colby Russell :crussell from comment #67)

> > +function inAccTreeView(aAccessible, aHighlightList)
> > +{
> > +  this.__proto__ = new inDataTreeView();
> > +
> > +  // nsITreeView
> > +  this.getRowProperties = function getRowProperties(aRowIdx, aProperties)
> ...
> > +  this.getCellProperties = function getCellProperties(aRowIdx, aCol, aProperties)
> ...
> > +  this.generateChildren = function generateChildren(aAccessible, aHighlightList,
> 
> Please change this to the more standard form.  (I.e.,
> 
> function inAccTreeView {...}
> inAccTreeView.prototype = ...
> inAccTreeView.prototype.getRowProperties = function ...
> inAccTreeView.prototype.getCellProperties = function ...
> inAccTreeView.prototype.generateChildren = function ...)

I can't do that because inAccTreeView.prototype = new inDataTreeView() is shared between all instances of inAccTreeView so that data members of inDataTreeView are shared too.

Comment 71

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 560938 [details] [diff] [review]
patch 13

Review of attachment 560938 [details] [diff] [review]:
-----------------------------------------------------------------

>    skin/modern/inspector/iconViewerList-dis.gif                          (resources/skin/modern/iconViewerList-dis.gif)
> +  skin/modern/inspector/viewers/accessibleEvent/accessibleEvent.css    (resources/skin/modern/viewers/accessibleEvent/accessibleEvent.css)
>    skin/modern/inspector/viewers/accessibleEvents/accessibleEvents.css   (resources/skin/modern/viewers/accessibleEvents/accessibleEvents.css)

Missed a space.

> +inDataTreeView.prototype.__defineGetter__("rowCount",
> +  function inDataTreeView_rowCount()
> +  {
> +    return this.mRows.length;
> +  });

Please put the close parenthesis on its own line at the same indentation depth as the defineGetter line.

> +
> +inDataTreeView.prototype.getCellText =
> +function inDataTreeView_getCellText(aRowIdx, aCol)
> +{

Please indent the function line here, as well as the others in the rest of the file.

> +  var removeCount = 0;
> +  var rowLevel = row.node.level;
> +  for (let i = aRowIdx + 1; i < this.rowCount; i++) {
> +    if (this.rowAt(i).node.level <= rowLevel) {

rowAt doesn't exist.  It was renamed to getRowAt.

> +      break;
> +    }
> +    removeCount++;

[It looks like you could just set removeCount once to |i - aRowIdx - 1| before the break, instead of incrementing every time.]

> +      for (var i = 0; i < outputList.length; i++) {

let here instead of var

> +        var output = outputList[i];
> +
> +        // Generate a tree.
> +        if (typeof output == "object" && "cols" in output && "view" in output) {
> +          var tree = document.createElement("tree");
…
> +        else {
> +          // Output text.
> +          var node = document.createElement("description");
> +          node.textContent = output;
> +          outputElm.appendChild(node);

[Any reason for generating and removing these programmatically instead of putting these in a deck in the XUL and toggling its selectedIndex?]

> +    try {
> +      var f = Function("event", "target", expr);
> +      var result = f(event, accessible);

You never do anything with result.

> +    }
> +    catch (ex) {
> +      for (var i in ex) {

let here
>    skin/modern/inspector/iconViewerList-dis.gif                          (resources/skin/modern/iconViewerList-dis.gif)
> +  skin/modern/inspector/viewers/accessibleEvent/accessibleEvent.css    (resources/skin/modern/viewers/accessibleEvent/accessibleEvent.css)
>    skin/modern/inspector/viewers/accessibleEvents/accessibleEvents.css   (resources/skin/modern/viewers/accessibleEvents/accessibleEvents.css)

Missed a space.

> +inDataTreeView.prototype.__defineGetter__("rowCount",
> +  function inDataTreeView_rowCount()
> +  {
> +    return this.mRows.length;
> +  });

Please remove the indentation before the last three lines, or put the close parenthesis on its own line unindented.

> +  this.mTree.rowCountChanged(aRowIdx + 1, this.rowCount - oldCount);

[You can change collapseNode to return removeCount, and do something similar for expandRow so that you don't have to recompute the difference here, but that's up to you.]

> +  this.mNodes.push(node);

What's the purpose of keeping this list if you always access a node through its row's |node| property?

> +
> +inDataTreeView.prototype.getCellText =
> +function inDataTreeView_getCellText(aRowIdx, aCol)
> +{

Please indent the function line here, as well as the others in the rest of the file.

> +  var removeCount = 0;
> +  var rowLevel = row.node.level;
> +  for (let i = aRowIdx + 1; i < this.rowCount; i++) {
> +    if (this.rowAt(i).node.level <= rowLevel) {

This will fail because rowAt doesn't exist.  It was renamed to getRowAt.

> +      break;
> +    }
> +    removeCount++;

[It looks like you could just set removeCount once to |i - aRowIdx - 1| before the break, instead of incrementing every time.]

> +      for (var i = 0; i < outputList.length; i++) {

let here instead of var

> +        var output = outputList[i];
> +
> +        // Generate a tree.
> +        if (typeof output == "object" && "cols" in output && "view" in output) {
> +          var tree = document.createElement("tree");
…
> +        else {
> +          // Output text.
> +          var node = document.createElement("description");
> +          node.textContent = output;
> +          outputElm.appendChild(node);

[Any reason for generating and removing these programmatically instead of specifying these in the XUL in a deck and toggling its selectedIndex?]

> +    try {
> +      var f = Function("event", "target", expr);
> +      var result = f(event, accessible);

result is never used.

> +    }
> +    catch (ex) {
> +      for (var i in ex) {

let here

Comment 72

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to alexander surkov from comment #70)
> (In reply to Colby Russell :crussell from comment #67)
> 
> > > +function inAccTreeView(aAccessible, aHighlightList)
> > > +{
> > > +  this.__proto__ = new inDataTreeView();
> > > +
> > > +  // nsITreeView
> > > +  this.getRowProperties = function getRowProperties(aRowIdx, aProperties)
> > ...
> > > +  this.getCellProperties = function getCellProperties(aRowIdx, aCol, aProperties)
> > ...
> > > +  this.generateChildren = function generateChildren(aAccessible, aHighlightList,
> > 
> > Please change this to the more standard form.  (I.e.,
> > 
> > function inAccTreeView {...}
> > inAccTreeView.prototype = ...
> > inAccTreeView.prototype.getRowProperties = function ...
> > inAccTreeView.prototype.getCellProperties = function ...
> > inAccTreeView.prototype.generateChildren = function ...)
> 
> I can't do that because inAccTreeView.prototype = new inDataTreeView() is
> shared between all instances of inAccTreeView so that data members of
> inDataTreeView are shared too.

Right.  You'll need to change the inAccTreeView constructor to either explicitly re-initialize mRows (and mNodes, if you don't remove it), or do

inDataTreeView.call(this);

Comment 73

[alexander :surkov (:asurkov)]

(In reply to Colby Russell :crussell from comment #71)

> > +        var output = outputList[i];
> > +
> > +        // Generate a tree.
> > +        if (typeof output == "object" && "cols" in output && "view" in output) {
> > +          var tree = document.createElement("tree");
> …
> > +        else {
> > +          // Output text.
> > +          var node = document.createElement("description");
> > +          node.textContent = output;
> > +          outputElm.appendChild(node);
> 
> [Any reason for generating and removing these programmatically instead of
> putting these in a deck in the XUL and toggling its selectedIndex?]

both of them may be shown the same time or even few instances of them

(In reply to Colby Russell :crussell from comment #72)

> > > function inAccTreeView {...}
> > > inAccTreeView.prototype = ...
> > > inAccTreeView.prototype.getRowProperties = function ...
> > > inAccTreeView.prototype.getCellProperties = function ...
> > > inAccTreeView.prototype.generateChildren = function ...)
> > 
> > I can't do that because inAccTreeView.prototype = new inDataTreeView() is
> > shared between all instances of inAccTreeView so that data members of
> > inDataTreeView are shared too.
> 
> Right.  You'll need to change the inAccTreeView constructor to either
> explicitly re-initialize mRows (and mNodes, if you don't remove it), or do
> 
> inDataTreeView.call(this);

isn't it more hacky then constructor style usage?

Comment 74

[alexander :surkov (:asurkov)]

Attachment: patch14

Comment 75

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 566118 [details] [diff] [review]
patch14

Review of attachment 566118 [details] [diff] [review]:
-----------------------------------------------------------------

> +  this.mNodes = [];

Again, what is the purpose of keeping this list if it's never used?

> +inDataTreeView.prototype.getCellText =
> +  function inDataTreeView_getCellText(aRowIdx, aCol)
> +  {
> +    var row = this.getRowAt(aRowIdx);
> +    return row ? row.node.data[aCol.id] : "";
> +  }

The whole function body doesn't need to be indented, just the function line.
Also, please put semi-colons at the end of these assignments.

> +    var checkRow = null;
> +    var i = aRowIdx - 1;
> +    do {
> +      checkRow = this.getRowAt(i);
> +      if (!checkRow) {
> +        return -1;
> +      }
> +
> +      if (checkRow.node == row.node.parent) {
> +        return i;
> +      }
> +
> +      --i;
> +    } while (checkRow);
> +
> +    return -1;
> +  }

The loop here can be made more concise, eliminating some redundancies and unreachable code:

  for (let i = aRowIdx - 1; i >= 0; --i) {
    let checkRow = this.getRowAt(i);
    if (checkRow.node == row.node.parent) {
      return i;
    }
  }

  return -1;

This does assume that for any i between 0 and (aRowIdx - 1), getRowAt(i) will be defined.  Is this assumption a bad one?

> +    var rowLevel = row.node.level;
> +    var idx = aRowIdx + 1;
> +    for (; idx < this.rowCount; idx++) {
> +      if (this.getRowAt(idx).node.level <= rowLevel) {
> +        break;
> +      }
> +    }
> +
> +    var removeCount = idx - aRowIdx - 1;
> +    this.mRows.splice(aRowIdx + 1, removeCount);

Why so much change from last time?  I liked the old one better, except with

 removeCount = idx - aRowIdx - 1
 break;

instead of |removeCount++| as the last part of the loop

> +    try {
> +      var f = Function("event", "target", expr);
> +      f(event, accessible);
> +    }
> +    catch (ex) {
> +      for (let i in ex) {
> +        output(ex[i]);

Wait, why is this happening instead of just output(ex)?

Comment 76

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to alexander surkov from comment #73)
> isn't it more hacky then constructor style usage?

Hackier than touching __proto__?  No, not at all.  Besides, inDataTreeView.call(this) *is* classic constructor style; it's like calling super...

Comment 77

[alexander :surkov (:asurkov)]

Attachment: patch15

Comment 78

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 567984 [details] [diff] [review]
patch15

Review of attachment 567984 [details] [diff] [review]:
-----------------------------------------------------------------

> +inDataTreeView.prototype.__defineGetter__("rowCount",
> +  function inDataTreeView_rowCount()
> +{
> +  return this.mRows.length;
> +}
> +);

No need to both unindent function body and have the close parenthesis on its own line.  Just pick one.  I.e., either:

inDataTreeView.prototype.__defineGetter__("rowCount",
  function inDataTreeView_rowCount()
  {
    return this.mRows.length;
  }
);

or

inDataTreeView.prototype.__defineGetter__("rowCount",
  function inDataTreeView_rowCount()
{
  return this.mRows.length;
});

Sorry if I have seemed confusing about this and my previous comments.

> +          for (var col in output.cols) {

> +  for (var i = 0; i < states.length; i++) {

> +    for (var i = 0; i < data.properties.length; i++) {

> +    for (var i = 0; i < data.properties.length; i++) {

let here

r=me with those changes.  Sorry it took so long.

Comment 79

[Colby Russell :crussell (no longer Mozilla-ing)]

By the way, meandering Bugzilla reader, who may be wondering about this:

(In reply to alexander surkov from comment #13)
> > I say thumbs up to some kind of contextual help, but I'm not sure I like the
> > use of a toolbarbutton or if this is the right approach. With my widget set, at
> > least, toolbarbuttons don't look like buttons until you're hovering over them,
> > so the floating question mark just looks like some misplaced text.
> 
> Probably it should be nice image I don't know. I didn't care a lot about
> nice UI look and I'm not sure I would like. I was focused on functionality .

It turns out you can use <button dlgtype="help"/>.  This gives you the help button you see in the Firefox preferences window ("Help" on GTK, and "?" on OS X).  According to what's on MDN, though, this is only supposed to work in dialogs.  (It worked anyway when I just tried it outside of one, though.)

Comment 80

[alexander :surkov (:asurkov)]

(In reply to Colby Russell :crussell from comment #79)
 
> It turns out you can use <button dlgtype="help"/>.  This gives you the help
> button you see in the Firefox preferences window ("Help" on GTK, and "?" on
> OS X).  According to what's on MDN, though, this is only supposed to work in
> dialogs.  (It worked anyway when I just tried it outside of one, though.)

it eats too much space on certain platforms, I think I like to keep light '?' toolbarbutton.

Comment 81

[alexander :surkov (:asurkov)]

landed:
http://hg.mozilla.org/dom-inspector/rev/71789d21c7bb
http://hg.mozilla.org/dom-inspector/rev/3042f92446fb - followup fixes for comment #78

Comment 82

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to alexander surkov from comment #80)
> (In reply to Colby Russell :crussell from comment #79)
>  
> > It turns out you can use <button dlgtype="help"/>.  This gives you the help
> > button you see in the Firefox preferences window ("Help" on GTK, and "?" on
> > OS X).  According to what's on MDN, though, this is only supposed to work in
> > dialogs.  (It worked anyway when I just tried it outside of one, though.)
> 
> it eats too much space on certain platforms, I think I like to keep light
> '?' toolbarbutton.

To clarify, I'm not asking you to include it (because it is too big and because it apparently only works outside of dialogs by accident).  Comment 79 is left for anyone who stumbles upon the bug and wonders whatever happened to the effort and who may be trying to figure out themselves how to get a Help button in XUL.