551404 - allow to watch a11y events for whole application

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[alexander :surkov (:asurkov)]

Now events for the chosen document are handled. The ability to watch events for other documents the same time is required.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[[e-mail]]

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

This didn't work very well on my debug build when I turned on Watch all documents and clicked on an event in the list.

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch2

a bit better, ignore events for this panel to avoid endless events appending to the event list.

Comment 4

[Alex Vincent [:WeirdAl]]

Whoa, wait a minute here.  Is DOM Inspector really the right tool to do this in?

DOM-I has always had a single-document-at-a-time model (ok, one doc and its sub-documents through iframes, browser elements, etc.).  Watching all accessibility events for the entire app?  I think that's seriously wrong for this tool, expanding its scope far beyond what Inspector does.

It might be better if there was a separate extension to do this.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> Whoa, wait a minute here.  Is DOM Inspector really the right tool to do this
> in?
> 
> DOM-I has always had a single-document-at-a-time model (ok, one doc and its
> sub-documents through iframes, browser elements, etc.).  Watching all
> accessibility events for the entire app?  I think that's seriously wrong for
> this tool, expanding its scope far beyond what Inspector does.

I don't think this requirement must be applied to events panel. Technically events panel might be not attached to specific document.

> It might be better if there was a separate extension to do this.

Then separate extension would be a copy of DOMi with this ability :) Seriously I do not find any appeal having two extensions to watch events and inspect events targets. This should complicate a much the things. If I'm wrong and you keep in mind an idea how two extensions might improve user experience then please share it. If your point is "that's not how DOMi works" then probably it's time to rethink it.

Comment 6

[alexander :surkov (:asurkov)]

Thinking more I find Alex's comment reasonable. Since as Alex said the current paradigm is the user can investigate the current document and its subdocuments then it should be applicable to all viewers including accessible events viewer. 

I suggest to add an ability to investigate application accessible which contains all application documents so the user will be able to see events from all documents within an application (probably excluding this DOM Inspector to avoid a mess) and this goes with the paradigm.

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch3

Add an ability to inspect application accessible. I like this approach more. Thanks Alex for rising the issue.

Comment 8

[[e-mail]]

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

>+      Components.classes["@mozilla.org/accessibleRetrieval;1"]
>+      .getService(Components.interfaces.nsIAccessibleRetrieval);
Nit: line up the .s?

>           if (!linkedViewer ||
>-              !(object instanceof Components.interfaces.nsIDOMNode))
>+              !object instanceof Components.interfaces.nsIDOMNode)
This change looks wrong because !object has higher precedence than instanceof

>+      if (aObject instanceof nsIAccessible)
>+        accObject = aObject;
>+
>+      else {
Why the blank line before the else?

> inAccTreeView.prototype.getDOMNodeFor =
> function getDOMNodeFor(aAccessible)
> {
>   var accessNode = XPCU.QI(aAccessible, nsIAccessNode);
>   var DOMNode = accessNode.DOMNode;
>+  if (!DOMNode)
>+    return;
Need to return something here, presumably null?

I'm not actually sure I spotted everything...

Comment 9

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

Not sure I'm the best person to review a11y stuff. I probably won't be able to really get to this until next Friday at the earliest. Some stuff I noticed, though:

>+      if (elm)
>         cmd.setAttribute("disabled", "true");

Brace the touch ifs etc., please.

>+        } catch(e) {
>+          dump("Failed to get accessible object for node.");
>+        }

Catch on a separate line, please. And maybe Components.utils.reportError instead of dump?

>+  let DOMNode = this.getDOMNodeFor(node.accessible);
>+  return DOMNode ? DOMNode : node.accessible;

Though it's not wrong, why let instead of var? Also:

return DOMNode || node.accessible;

(In reply to comment #8)
> (From update of attachment 433361 [details] [diff] [review])
> >+      Components.classes["@mozilla.org/accessibleRetrieval;1"]
> >+      .getService(Components.interfaces.nsIAccessibleRetrieval);
> Nit: line up the .s?

For what it's worth, in my cleanup, I've just been doing a two line indent for a several reasons:

1. Lined up periods doesn't really make any sense, does it? What are we trying to imply with that form of indentation? That getService somehow has something to do with the Components object? (Aside: people seem to do this more often with Components than in other forms, even when they might indent the other forms differently. Maybe because anything to do with XPCOM is such voodoo and they're already aping it?)
2. It's more consistent with other indentation rules, partly because:
3. Two-space indent saves precious horizontal space.

On the other hand, XPCU.js is already there, so just go ahead and use XPCU.getService. Heh.

(In reply to comment #4)
> Whoa, wait a minute here.  Is DOM Inspector really the right tool to do this
> in?

Meh. DOM Inspector doesn't necessarily have the best workflows in other areas. I tend to think of it as "Mozilla Inspector" anyway. (À la "Mozilla Firefox", not necessarily as in "inspector of Mozilla", though it's that, too.) It's certainly outgrown the "DOM" of "DOM Inspector". Seems like such a feature wouldn't be out of place, and a user-toggleable option sounds sufficient.

Comment 10

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

Argh, that should say "two-space indent" instead of "two line indent".

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #9)

> >+        } catch(e) {
> >+          dump("Failed to get accessible object for node.");
> >+        }
> 
> Catch on a separate line, please. And maybe Components.utils.reportError
> instead of dump?

I'm not sure we want to dump into error console, iirc dump() adds warning message into firefox debug console.

> >+  let DOMNode = this.getDOMNodeFor(node.accessible);
> >+  return DOMNode ? DOMNode : node.accessible;
> 
> Though it's not wrong, why let instead of var?

iirc, Shawn asked for this

> > >+      Components.classes["@mozilla.org/accessibleRetrieval;1"]
> > >+      .getService(Components.interfaces.nsIAccessibleRetrieval);
> > Nit: line up the .s?

> On the other hand, XPCU.js is already there, so just go ahead and use
> XPCU.getService. Heh.

done.

Comment 12

[alexander :surkov (:asurkov)]

Attachment: patch4

Comment 13

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

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

I can't seem to capture any application events on fx3.7a4. I'll try it on a trunk build later today.

>+    <menuitem oncommand="inspector.setTargetApplicationAccessible();"

>+  setTargetApplicationAccessible: function setTargetApplicationAccessible()
>+  {
>+    var accService =
>+      XPCU.getService(kAccRetrievalCID, "nsIAccessibleRetrieval");
>+
>+    if (accService) {
>+      this.mDocPanel.subject = accService.getApplicationAccessible();
>+    }
>+  },

This isn't available pre-1.9.3. I'd rather see this hidden by default in the XUL, and include a check that makes it visible for those with the ability to use it, or vice versa.

Also, I don't feel like this belongs in the File menu. I think something like resources/content/viewers/dom/popupOverlay.xul would be better, but for the a11y viewers. For example, when you're using the DOM Nodes viewer, you click the dropmarker on the right side of the viewer's title bar, and toggle something like "Show Whitespace Nodes". Same thing here. Create resources/content/viewers/accessibleTree/popupOverlay.xul and add the appropriate overlays to get the item to show up in the Accessible Tree and Accessible Events viewers.

>       <rdf:Description ins:uid="accessibleObject"
>                        ins:panels="bxObjectPanel bxObjPanel"
>                        ins:description="Accessible Object">
>         <ins:filter><![CDATA[
>           if (!linkedViewer ||
>-              !(object instanceof Components.interfaces.nsIDOMNode))
>+              linkedViewer.uid != "accessibleEvents" &&
>+              linkedViewer.uid != "accessibleTree" &&
>+              (linkedViewer.uid != "dom" || !linkedViewer.getAccessibleNodes())) {
>             return false;
>+          }

I'm not sure why we're discriminating about the linked viewer anyway. If I'm using the DOM Nodes viewer in the left pane, shouldn't I be able to get to the Accessible Properties viewer in the right pane? This is probably for another bug, though.

>+        } catch(e) {

Separate lines for the close brace and the catch, please.

>+  // Ignore events having target not in subtree of currently inspected
>+  // accessible.
>+  let parentAccessible = accessible;
>+  while (parentAccessible != this.mAccessible &&
>+         (parentAccessible = parentAccessible.parent));

while (parentAccessible && parentAccessible != this.mAccessible) {
  parentAccessible = parentAcessible.parent;
}

Comment 14

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

(In reply to comment #11)
> I'm not sure we want to dump into error console, iirc dump() adds warning
> message into firefox debug console.

Well, how often do we ask people to check the error console versus firing up a shell and checking there?

> 
> > >+  let DOMNode = this.getDOMNodeFor(node.accessible);
> > >+  return DOMNode ? DOMNode : node.accessible;
> > 
> > Though it's not wrong, why let instead of var?
> 
> iirc, Shawn asked for this

I know he asked for it in loops, but I think that's just reassurance against accidentally using the same variable used by an outer loop.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #13)

> Also, I don't feel like this belongs in the File menu. I think something like
> resources/content/viewers/dom/popupOverlay.xul would be better, but for the
> a11y viewers. For example, when you're using the DOM Nodes viewer, you click
> the dropmarker on the right side of the viewer's title bar, and toggle
> something like "Show Whitespace Nodes". Same thing here. Create
> resources/content/viewers/accessibleTree/popupOverlay.xul and add the
> appropriate overlays to get the item to show up in the Accessible Tree and
> Accessible Events viewers.

File menu serves to choose a document to inspect. Application accessible is much similar to document (it's top level accessible containing all chrome document accessibles) and therefore I put it into File menu. Application accessible isn't related to currently inspected document therefore it doesn't look right to put its menu item next to "Show whitespace nodes".

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #13)

> >       <rdf:Description ins:uid="accessibleObject"
> >                        ins:panels="bxObjectPanel bxObjPanel"
> >                        ins:description="Accessible Object">
> >         <ins:filter><![CDATA[
> >           if (!linkedViewer ||
> >-              !(object instanceof Components.interfaces.nsIDOMNode))
> >+              linkedViewer.uid != "accessibleEvents" &&
> >+              linkedViewer.uid != "accessibleTree" &&
> >+              (linkedViewer.uid != "dom" || !linkedViewer.getAccessibleNodes())) {
> >             return false;
> >+          }
> 
> I'm not sure why we're discriminating about the linked viewer anyway. If I'm
> using the DOM Nodes viewer in the left pane, shouldn't I be able to get to the
> Accessible Properties viewer in the right pane? This is probably for another
> bug, though.

You should and you do. DOM nodes viewer has "dom" uid afaik.

(In reply to comment #14)
> (In reply to comment #11)
> > I'm not sure we want to dump into error console, iirc dump() adds warning
> > message into firefox debug console.
> 
> Well, how often do we ask people to check the error console versus firing up a
> shell and checking there?

Mostly never. And we write into console when error should happen never. It's similar to assertions I think: we add them to the code what shouldn't fail. Any way if you prefer to see error in console then I could remove try/catch block here.

> I know he asked for it in loops, but I think that's just reassurance against
> accidentally using the same variable used by an outer loop.

So you prefer to see var instead a let? Should I change this?

Comment 17

[alexander :surkov (:asurkov)]

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

please let me know if you need new patch to continue review.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #13)

> This isn't available pre-1.9.3. I'd rather see this hidden by default in the
> XUL, and include a check that makes it visible for those with the ability to
> use it, or vice versa.

fixed locally

> >+        } catch(e) {
> 
> Separate lines for the close brace and the catch, please.

fixed locally

> while (parentAccessible && parentAccessible != this.mAccessible) {
>   parentAccessible = parentAcessible.parent;
> }

fixed locally

Comment 19

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

>+  while (parentAccessible != this.mAccessible &&
>+         (parentAccessible = parentAccessible.parent));
>+  
>+  if (!parentAccessible) {
>     return;
>+  }

Get rid of the two spaces at the end of the blank line, please.

>+  setTargetApplicationAccessible: function setTargetApplicationAccessible()
>+  {
>+    var accService =
>+      XPCU.getService(kAccRetrievalCID, "nsIAccessibleRetrieval");
>+
>+    if (accService) {
>+      this.mDocPanel.subject = accService.getApplicationAccessible();
>+    }
>+  },

The location bar will still show the location of the previously inspected document. onEvent needs to be changed in inspector.js so that a subjectChange event will set the location bar's value to the empty string. It would be nice if the DOM Inspector's title bar were set to something useful as well.

(In reply to comment #16)
> (In reply to comment #13)
...
> > I'm not sure why we're discriminating about the linked viewer anyway. If I'm
> > using the DOM Nodes viewer in the left pane, shouldn't I be able to get to the
> > Accessible Properties viewer in the right pane? This is probably for another
> > bug, though.
> 
> You should and you do. DOM nodes viewer has "dom" uid afaik.

Ah, you're right! I still don't think we should be sniffing for the viewer, instead of just making sure the object in question has the properties we're interested in. ("Properties" in the literal sense of the word, not necessarily JavaScript). But that's for another bug.

> (In reply to comment #14)
> > (In reply to comment #11)
...
> > Well, how often do we ask people to check the error console versus firing up a
> > shell and checking there?
> 
> Mostly never. And we write into console when error should happen never. It's
> similar to assertions I think: we add them to the code what shouldn't fail. Any
> way if you prefer to see error in console then I could remove try/catch block
> here.

I think I would, but neil might request differently.

> So you prefer to see var instead a let? Should I change this?

Here, yes.

(In reply to comment #17)
> (From update of attachment 437483 [details] [diff] [review])
> please let me know if you need new patch to continue review.

(In reply to comment #18)
> (In reply to comment #13)
> 
> > This isn't available pre-1.9.3. I'd rather see this hidden by default in the
> > XUL, and include a check that makes it visible for those with the ability to
> > use it, or vice versa.
> 
> fixed locally

I'd like to see this before r+.

I also still get spurious errors and lack of functionality on trunk. I don't know if this is because of trunk, or if it's something this patch is doing (or not doing). If I open the DOM Inspector and immediately select the "Inspect Application Accessible" menuitem, then switch to the Accessible Events viewer, I get the ol' "Components is not defined" error in tree.xml. I can get it to work if I open the DOM Inspector on a document, switch to the Accessible Events viewer first, then select the "Inspect Application Accessible" menuitem, but I don't receive any events for windows that predate the viewer instantiation, except for the window related to the document I was originally inspecting.

For example, if I have a browser window and the Error Console open, then open the DOM Inspector on the document in the browser, switch to the Accessible Events viewer, and select the "Inspect Application Accessible" menuitem, I get events for the browser window, but not for the Error Console. Similarly, if I open the DOM Inspector on the Error Console, switch to the Accessible Events viewer, then select the "Inspect Application Accessible" menuitem, I get events for the Error Console, but not for the browser. In both cases, if I open subsequent windows of any type, I will receive events for those.

AccessibleEventsView.prototype.observe continues to observe events even after the viewer is unloaded, or a new document is inspected. (When the DOM Inspector is closed, this fills the Error Console with "XPCU is not defined" errors for every event observed over the lifetime of the application). I would say to do a removeObserver call in the viewer's destroy method, but since that would fix the problem when you inspect another document, you'll need to take care of this in the viewer's subject setter.

Also, there's a line (261) in accessibleEvents.js that reads:
>   this.mTree.rowCountChanged(0, 1);

AccessibleEventsView has no mTree field. It should say viewer.mTree... This wasn't introduced in this patch, but now that we can receive events for the entire application, it makes these errors much more visible. It would be nice if that could be fixed.

Comment 20

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

Oh! I forgot to mention.

+  // Ignore events on this DOM Inspector to avoid a mess.
+  if (accessnode.rootDocument == this.mDOMIRootDocumentAccessible) {
     return;
+  }

It would be neat-o if the filter you get from the "Choose events to watch" button allowed you to filter events based on the rootDocument, and this DOM Inspector's window was shown unchecked with the checkbox disabled. Also, some kind of smart way to automatically filter out events originating from *other* DOM Inspector windows inspecting whole-application events (though I'm not sure why anyone would be doing this.)

You don't need to do any of this to get review from me.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #19)
> >+  while (parentAccessible != this.mAccessible &&
> >+         (parentAccessible = parentAccessible.parent));
> >+  
> >+  if (!parentAccessible) {
> >     return;
> >+  }
> 
> Get rid of the two spaces at the end of the blank line, please.

I failed to find this. Probably this was fixed eventually.

> The location bar will still show the location of the previously inspected
> document. onEvent needs to be changed in inspector.js so that a subjectChange
> event will set the location bar's value to the empty string. It would be nice
> if the DOM Inspector's title bar were set to something useful as well.

fixed: location bar is empty, title is "Application Accessible".

> Ah, you're right! I still don't think we should be sniffing for the viewer,
> instead of just making sure the object in question has the properties we're
> interested in. ("Properties" in the literal sense of the word, not necessarily
> JavaScript). But that's for another bug.

I don't remember the reason but at that point I've found it reasonable.

> > Mostly never. And we write into console when error should happen never. It's
> > similar to assertions I think: we add them to the code what shouldn't fail. Any
> > way if you prefer to see error in console then I could remove try/catch block
> > here.
> 
> I think I would, but neil might request differently.

Ok, let's see whether Neil likes to change this as well. 

> 
> > So you prefer to see var instead a let? Should I change this?
> 
> Here, yes.

ok, done

> > > This isn't available pre-1.9.3. I'd rather see this hidden by default in the
> > > XUL, and include a check that makes it visible for those with the ability to
> > > use it, or vice versa.
> > 
> > fixed locally
> 
> I'd like to see this before r+.

sure, but actually I added a hack, I started to check the gecko version, I can't think nothing nicer. I don't want to create accessibility service when DOMi starts to prevent a11y from loading. 

> 
> I also still get spurious errors and lack of functionality on trunk. I don't
> know if this is because of trunk, or if it's something this patch is doing (or
> not doing). If I open the DOM Inspector and immediately select the "Inspect
> Application Accessible" menuitem, then switch to the Accessible Events viewer,
> I get the ol' "Components is not defined" error in tree.xml.

It's somehow related with a11y since a11y triggers accesibleType property which fails. Unfortunately no idea what can be wrong.

>
> For example, if I have a browser window and the Error Console open, then open
> the DOM Inspector on the document in the browser, switch to the Accessible
> Events viewer, and select the "Inspect Application Accessible" menuitem, I get
> events for the browser window, but not for the Error Console. Similarly, if I
> open the DOM Inspector on the Error Console, switch to the Accessible Events
> viewer, then select the "Inspect Application Accessible" menuitem, I get events
> for the Error Console, but not for the browser. In both cases, if I open
> subsequent windows of any type, I will receive events for those.

That's strange because I can see events in either way. (However I don't see some events from Error Console I would expect to see but I don't see them in either way, it makes me thing something wrong with Error Console). I always get first focus for Error Console and lot of events when I type into text field of Error Console.

> 
> AccessibleEventsView.prototype.observe continues to observe events even after
> the viewer is unloaded, or a new document is inspected. (When the DOM Inspector
> is closed, this fills the Error Console with "XPCU is not defined" errors for
> every event observed over the lifetime of the application). I would say to do a
> removeObserver call in the viewer's destroy method, but since that would fix
> the problem when you inspect another document, you'll need to take care of this
> in the viewer's subject setter.

I added check when subject is set whether AccessibleEventsView was created already and then I destroy it. I hope it helps.

> 
> Also, there's a line (261) in accessibleEvents.js that reads:
> >   this.mTree.rowCountChanged(0, 1);
> 
> AccessibleEventsView has no mTree field. It should say viewer.mTree... This
> wasn't introduced in this patch, but now that we can receive events for the
> entire application, it makes these errors much more visible. It would be nice
> if that could be fixed.

It should have mTree if everything is ok since it's a tree view and when tree view is attached to the tree then setTree() is called. It sounds this error is similar to removeObserver() issue described above. When I started to destroy previously created AccessibleEventsView on subject change then this error disappeared.

Comment 22

[alexander :surkov (:asurkov)]

Attachment: patch5

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #20)

> It would be neat-o if the filter you get from the "Choose events to watch"
> button allowed you to filter events based on the rootDocument, and this DOM
> Inspector's window was shown unchecked with the checkbox disabled.

It's probably ok to add an ability to turn on/off events capturing from subdocuments but it might be a big tree and on the another hand the user always can choose subdocument to watch for events from File menu. I agree it's probably nice to show in UI we don't listen events for this DOMi window but I don't clearly imagine the UI.

> Also, some
> kind of smart way to automatically filter out events originating from *other*
> DOM Inspector windows inspecting whole-application events (though I'm not sure
> why anyone would be doing this.)

This makes sense but I would say let's fix this when somebody needs this.

Comment 24

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

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

+              (linkedViewer.uid != "dom" || !linkedViewer.getAccessibleNodes())) {
...
+              (linkedViewer.uid != "dom" || !linkedViewer.getAccessibleNodes())) {
...
+              (linkedViewer.uid != "dom" || !linkedViewer.getAccessibleNodes())) {

Linebreak after || to fit in 80 columns. There are a few other touched lines in this patch that go over 80 columns, too.

> +  while (parentAccessible != this.mAccessible &&
> +         (parentAccessible = parentAccessible.parent));

This one, too. (See comment 13.) Note that there's a typo in comment 13. It should say "parentAccessible", not "parentAcessible", so these lines and the ones changed in this patch should use the correct identifier. Sorry.

> +          } else if (aEvent.subject instanceof nsIAccessibleApplication) {

> +      if (aObject instanceof nsIAccessible) {
> +        accObject = aObject;
> +      } else {

else on new line.

(in reply to comment #21)
> sure, but actually I added a hack, I started to check the gecko version, I
> can't think nothing nicer. I don't want to create accessibility service when
> DOMi starts to prevent a11y from loading. 

Not ideal, but I guess it will have to do.

> That's strange because I can see events in either way.
...
> I always get first focus for Error Console and lot of events when I type into text field of Error Console.

I should amend what I said in comment #19. It seems like it doesn't matter which windows predate which. What *does* seem to matter is whether the window predates the first use of the accessibility service in the application, with the exception of windows which were originally being inspected. All the things I said above hold, *except* where the DOM Inspector was already used at least once before to inspect the application accessible. In that case, I can get events for all windows, regardless of whether they're older than the DOM Inspector's.

> It should have mTree

Oh. This is part of the way inBaseTreeView implements things.

Also, since you're touching popupOverlay.xul, could you please go ahead and remove the sourcing of util.dtd there?

One more patch.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #24)

> I should amend what I said in comment #19. It seems like it doesn't matter
> which windows predate which. What *does* seem to matter is whether the window
> predates the first use of the accessibility service in the application, with
> the exception of windows which were originally being inspected. All the things
> I said above hold, *except* where the DOM Inspector was already used at least
> once before to inspect the application accessible. In that case, I can get
> events for all windows, regardless of whether they're older than the DOM
> Inspector's.

Could you give me steps to reproduce please?

Comment 26

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

1. Open application.
2. Open Error Console.
3. Open DOM Inspector.
4. Switch to Accessible Events viewer.
5. From the File menu, choose "Inspect Application Accessible".
6. Try to generate some events in the Error Console by clicking, typing etc., and note no events appear.
7. Try to generate some events in the browser, e.g., tooltip show on toolbarbutton hover, clicking for focus, etc., and note that events do appear.
8. Repeat step 6 and observe that still no events appear.
9. Close Error Console.
10. Open Error Console.
11. Repeat step 6 and observe that events do appear.
12. Close DOM Inspector.
13. Open DOM Inspector.
14. Switch to Accessible Events viewer.
15. From the File Menu, choose "Inspect Application Accessible".
16. Repeat step 6 and observe that events do appear.
17. Close Error Console.
18. Open Error Console.
19. Repeat step 6 and observe that events to appear.

Interchanging steps 2 and 3 has no effect.

You can replace all instances of "Error Console" above with "second DOM Inspector window", "Venkman", or "Page Info", and the same results occur.

You can also rearrange the steps to be 1, 3, 4, 2, 5, and events *do* appear for the Error Console the first time around.

All testing indicates to me that the anomaly is for windows which were opened before the first use of the accessibility service.

Comment 27

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

I should amend the the observation in my last comment to say "... for windows whose documents were loaded before the first use of the accessibility service." The observation is supported by doing steps 1–6 with a second DOM Inspector window, and continuing as follows:

7. From second DOM Inspector window (the one not inspecting accessible events), switch to the Style Sheets viewer. Observe that events do appear for this. (Viewers are loaded through XUL browsers.)
8. Attempt to generate some events in the second DOM Inspector window, but outside the Style Sheets viewer, e.g, on the menubar. Observe that events begin appearing here, too.

It's like a11y is now "aware" of events in this window, where it wasn't before for step 6, because a subdocument was loaded. You can also try this with Venkman and opening a script in the Source view. Similar results occur.

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #26)

> All testing indicates to me that the anomaly is for windows which were opened
> before the first use of the accessibility service.

it sounds like serious accessibility bug. I'll investigate. Thank you.

Comment 29

[alexander :surkov (:asurkov)]

(In reply to comment #28)
> (In reply to comment #26)
> 
> > All testing indicates to me that the anomaly is for windows which were opened
> > before the first use of the accessibility service.
> 
> it sounds like serious accessibility bug. I'll investigate. Thank you.

I filed bug 560239 for this. Can we go ahead without this fix?

Comment 30

[alexander :surkov (:asurkov)]

Attachment: patch6

Comment 31

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

Comment on attachment 439946 [details] [diff] [review]
patch6

Some touched lines that break 80 columns:
viewer-registry.rdf, line 234 (attachment 439936 [details] [diff] [review], line 359)
viewer-registry.rdf, line 246 (attachment 439936 [details] [diff] [review], line 372)
accessibleEvents.js, line 211 (attachment 439936 [details] [diff] [review], line 448)

r=crussell after these are fixed

Comment 32

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

Oh, wow. That should be.

(From update of attachment 439946 [details] [diff] [review])
Some touched lines that break 80 columns:
viewer-registry.rdf, line 234 (attachment 439946 [details] [diff] [review], line 359)
viewer-registry.rdf, line 246 (attachment 439946 [details] [diff] [review], line 372)
accessibleEvents.js, line 211 (attachment 439946 [details] [diff] [review], line 448)

r=crussell after these are fixed

Jeez.

Comment 33

[alexander :surkov (:asurkov)]

Attachment: patch7

Comment 34

[[e-mail]]

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

>+          else if (aEvent.subject instanceof nsIAccessibleApplication) {
I wonder whether it would be better to do this check first?

>           if (!linkedViewer ||
>-              !(object instanceof Components.interfaces.nsIDOMNode))
>+              linkedViewer.uid != "accessibleEvents" &&
>+              linkedViewer.uid != "accessibleTree" &&
>+              (linkedViewer.uid != "dom" ||
>+               !linkedViewer.getAccessibleNodes())) {
>             return false;
>+          }
> 
>-          if (linkedViewer.uid != "accessibleEvents" &&
>-              linkedViewer.uid != "accessibleTree" &&
>-              (linkedViewer.uid != "dom" || !linkedViewer.getAccessibleNodes()))
>+          if (object instanceof Components.interfaces.nsIAccessible) {
>+            return true;
>+          }
>+
>+          if (!(object instanceof Components.interfaces.nsIDOMNode)) {
>             return false;
>+          }
It's hard to follow the logic changes here. Is there any reason you didn't simply prefix a test for nsIAccessible to the existing code?

>+  this.mAccessible = aObject instanceof nsIAccessible ?
>+    aObject :
>+    this.mAccService.getAccessibleFor(aObject);
I noticed that sometimes you wrapped these ?:s and sometimes you didn't?

>+  var parentAccessible = accessible;
>+  while (parentAccessible != this.mAccessible &&
>+         (parentAccessible = parentAccessible.parent));
>+
>+  if (!parentAccessible) {
>+    return;
>+  }
...
>+  var parentAccessible = accessible;
>+  while (parentAccessible && parentAccessible != this.mAccessible) {
>+    parentAccessible = parentAccessible.parent;
>+  }
>+
>+  if (!parentAccessible) {
>     return;
>+  }
Inconsistent. I prefer the second version.

[Another option:
  var parentAccessible = accessible;
  while (parentAccessible != this.mAccessible) {
    parentAccessible = parentAccessible.parent;
    if (!parentAccessible) {
      return;
    }
  }
]

Comment 35

[alexander :surkov (:asurkov)]

(In reply to comment #34)
> (From update of attachment 440111 [details] [diff] [review])
> >+          else if (aEvent.subject instanceof nsIAccessibleApplication) {
> I wonder whether it would be better to do this check first?

nsIAccessibleApplication is more rare case so I put it as second condition.

> It's hard to follow the logic changes here. Is there any reason you didn't
> simply prefix a test for nsIAccessible to the existing code?

That 'if' becomes big and hard to understand, therefore I split it into several simple ifs.

> 
> >+  this.mAccessible = aObject instanceof nsIAccessible ?
> >+    aObject :
> >+    this.mAccService.getAccessibleFor(aObject);
> I noticed that sometimes you wrapped these ?:s and sometimes you didn't?

I think because of 80 characters restriction.

> Inconsistent. I prefer the second version.
> 
> [Another option:

done

Comment 36

[[e-mail]]

Sorry for the delay.

(In reply to comment #35)
>(In reply to comment #34)
>>It's hard to follow the logic changes here. Is there any reason you didn't
>>simply prefix a test for nsIAccessible to the existing code?
>That 'if' becomes big and hard to understand, therefore I split it into several
> simple ifs.
No, that's not what I meant; my point is that if the object is already an nsIAccessible then the linked viewer must already be an accessiblity-enabled viewer, so there's no need to do all of those checks in that case.

>>>+  this.mAccessible = aObject instanceof nsIAccessible ?
>>>+    aObject :
>>>+    this.mAccService.getAccessibleFor(aObject);
>>I noticed that sometimes you wrapped these ?:s and sometimes you didn't?
>I think because of 80 characters restriction.
But the two like this would have fitted on to two lines like the other two?

Comment 37

[alexander :surkov (:asurkov)]

Attachment: patch8

Comment 38

[alexander :surkov (:asurkov)]

(In reply to comment #36)
> Sorry for the delay.
> 
> (In reply to comment #35)
> >(In reply to comment #34)
> >>It's hard to follow the logic changes here. Is there any reason you didn't
> >>simply prefix a test for nsIAccessible to the existing code?
> >That 'if' becomes big and hard to understand, therefore I split it into several
> > simple ifs.
> No, that's not what I meant; my point is that if the object is already an
> nsIAccessible then the linked viewer must already be an accessiblity-enabled
> viewer, so there's no need to do all of those checks in that case.

I see. I moved nsIAccessible check to the top.

> >>>+  this.mAccessible = aObject instanceof nsIAccessible ?
> >>>+    aObject :
> >>>+    this.mAccService.getAccessibleFor(aObject);
> >>I noticed that sometimes you wrapped these ?:s and sometimes you didn't?
> >I think because of 80 characters restriction.
> But the two like this would have fitted on to two lines like the other two?

you're right. fixed

Comment 39

[[e-mail]]

Comment on attachment 445319 [details] [diff] [review]
patch8

>           if (!linkedViewer ||
>-              !(object instanceof Components.interfaces.nsIDOMNode))
>+              linkedViewer.uid != "accessibleEvents" &&
>+              linkedViewer.uid != "accessibleTree" &&
>+              (linkedViewer.uid != "dom" ||
>+               !linkedViewer.getAccessibleNodes())) {
>             return false;
>+          }
> 
>-          if (linkedViewer.uid != "accessibleEvents" &&
>-              linkedViewer.uid != "accessibleTree" &&
>-              (linkedViewer.uid != "dom" || !linkedViewer.getAccessibleNodes()))
>+          if (!(object instanceof Components.interfaces.nsIDOMNode)) {
>             return false;
>+          }
(3x) Do you still need these changes? (hg diff makes them look worse.)

>+  var parentAccessible = accessible;
>+  while (parentAccessible != this.mAccessible &&
>+         (parentAccessible = parentAccessible.parent));
>+
>+  if (!parentAccessible) {
>+    return;
>+  }
...
>+  var parentAccessible = accessible;
>+  while (parentAccessible != this.mAccessible) {
>+    parentAccessible = parentAccessible.parent;
>+    if (!parentAccessible) {
>+      return;
>+    }
>+  }
>+
>+  if (!parentAccessible) {
>     return;
>+  }
These still don't match, and the second one now has two returns...

Comment 40

[alexander :surkov (:asurkov)]

Attachment: patch9 [Checkin: comment 42, backed out: comment 61]

Comment 41

[alexander :surkov (:asurkov)]

(In reply to comment #39)

> (3x) Do you still need these changes? (hg diff makes them look worse.)

no, actually, thanks

> These still don't match, and the second one now has two returns...

sorry, fixed

Comment 42

[alexander :surkov (:asurkov)]

landed http://hg.mozilla.org/dom-inspector/rev/182422848dc7

Comment 43

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

(From #seamonkey)
> 17:51 <@NeilAway> crussell_afk: hmm, surkov's Inspect Accessible Application
>                   patch wasn't tested against a --disable-accessibility build :s

Comment 44

[alexander :surkov (:asurkov)]

(In reply to comment #43)
> (From #seamonkey)
> > 17:51 <@NeilAway> crussell_afk: hmm, surkov's Inspect Accessible Application
> >                   patch wasn't tested against a --disable-accessibility build :s

reopen because it wasn't tested or has bugs?

Comment 45

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

It has bugs.

(From #seamonkey)
> 15:56 <%stefanh> Error: invalid 'instanceof' operand 
>                  Components.interfaces.nsIAccessible
> 15:56 <%stefanh> Source File: chrome://inspector/content/ViewerRegistry.js
> 15:56 <%stefanh> Line: 112

We need to replace all of the lines like:
>           if (object instanceof Components.interfaces.nsIAccessible) {

with

  if ("nsIAccessible" in Components.interfaces &&
      object instanceof Components.interfaces.nsIAccessible) {

The two occurrences of:
>           return (object instanceof Ci.nsIDOMDocument ||
>                   object instanceof Ci.nsIAccessibleApplication) &&
>             "@mozilla.org/accessibleRetrieval;1" in Components.classes;

can be twiddled so the |"@mozilla.org/accessibleRetrieval;1" in Components.classes| is in the first part of the AND expression, and will short-circuit the |"@mozilla.org/accessibleRetrieval;1" in Components.classes;| if it fails.

Also, the XULAppInfo hack needs to have a check for no accessibility in addition to what it is now:
>     if (info.platformVersion < "1.9.3") {

(Aside: I hope XULRunner never reaches 1.9.10, because that check will fail.)

Comment 46

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

(In reply to comment #45)
> (Aside: I hope XULRunner never reaches 1.9.10, because that check will fail.)

Stupid comment. Presumably, we'll have dropped support for anything less than 1.9.3 long before we reach 1.9.10, so those lines will be gone.

Comment 47

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

(In reply to comment #45)
> can be twiddled so the |"@mozilla.org/accessibleRetrieval;1" in
> Components.classes| is in the first part of the AND expression, and will
> short-circuit the |"@mozilla.org/accessibleRetrieval;1" in Components.classes;|
> if it fails.

This should say that the
"@mozilla.org/accessibleRetrieval;1" in Components.classes
will short-circuit the
object instanceof Ci.nsIAccessibleApplication

Comment 48

[alexander :surkov (:asurkov)]

Attachment: fix patch

Comment 49

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

Comment on attachment 446962 [details] [diff] [review]
fix patch

(In reply to comment #45)
> Also, the XULAppInfo hack needs to have a check for no accessibility in
> addition to what it is now:
> >     if (info.platformVersion < "1.9.3") {

I take this part back. A disabled menuitem seems sufficient.

Comment 50

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

I noted in bug 553364 that I've been unable to get any events to appear since this patch landed. (This is in the old document-oriented accessible events viewer in 1.9.2—not while inspecting application accessible.) All my events are getting dropped here:

  // Ignore events on this DOM Inspector to avoid a mess.
  if (accessnode.rootDocument == this.mDOMIRootDocumentAccessible) {
    return;
  }

Comment 51

[[e-mail]]

That would presumably be because rootDocument is a new API for 1.9.3 ...

Comment 52

[Robert Kaiser]

I just realized that "application accessible" is a very abstract term that is hard to understand for our localizers, it would be good to describe what it is in an L10n note, so localizers get a clue how to translate this.

Comment 53

[Robert Kaiser]

BTW, some of the localization problem comes from using "accessible" as a noun, which can't easily be done in many languages. The localizer needs to know what that means, i.e. what it is that is accessible there.

Comment 54

[alexander :surkov (:asurkov)]

I don't think accessible (when used as a noun) should be translated as for example DOM term isn't translated I think. In general accessible means an element of accessible tree. Accessible tree is a tree that contains information about web page content (or any document) to AT (assistive technologies) like screen readers.

Application accessible is top level accessible, unique for entire application, contains accessibles for all windows as children, for example, if you open Firefox window and DOMInspector window, then application accessible has two children.

Comment 55

[Tony Mechelynck [:tonymec]]

In reply to comment #54:
I think whether to translate the noun "accessible" or not should be a decision of the localizers for each language. In French, considering that «accessible» is actually a French adjective that the English language borrowed, I suppose «un accessible» would be OK for the noun. In Russian, assuming “доступный, -ая, -ое„ for the adjective, I suppose “доступное„ would be OK for the noun. Seeing “accessible„ in Latin script in the middle of a sentence in Cyrillic would look like something had been erroneously left untranslated.

Comment 56

[alexander :surkov (:asurkov)]

(In reply to comment #55)
> In reply to comment #54:
> I think whether to translate the noun "accessible" or not should be a decision
> of the localizers for each language.

Sure, localizers always take a decision :)

> In Russian, assuming “доступный, -ая,
> -ое„ for the adjective, I suppose “доступное„ would be OK for the noun. Seeing
> “accessible„ in Latin script in the middle of a sentence in Cyrillic would look
> like something had been erroneously left untranslated.

I'm not sure “доступное„ is good analogy in Russian since it can mean available and accessible both and perhaps available is more closer, at least I'm sure if you ask anybody what “доступное„ means then nobody says it's about blind people or people with disabilities. In English if it's not a primary meaning of accessibility word but not the last at least. I think Russian language should loanwords 'accessible' and 'accessibility' words like it happened with computer, notebook and many words last time.

Comment 57

[Tony Mechelynck [:tonymec]]

“аксесибль„ then? ;-)

Comment 58

[alexander :surkov (:asurkov)]

(In reply to comment #57)
> “аксесибль„ then? ;-)

without 'ь' in the end, but honestly I think that would be my preference. Perhaps 'аксесибл объект' I don't know.

Comment 59

[alexander :surkov (:asurkov)]

(In reply to comment #58)
> (In reply to comment #57)
> > “аксесибль„ then? ;-)
> 
> without 'ь' in the end, but honestly I think that would be my preference.
> Perhaps 'аксесибл объект' I don't know.

though google returns 10000 results for 'доступность accessibility' keywords but around 1000 results for 'аксессибилити' keyword. It sounds people tend to give new meaning to existing word than borrow a foreign word.

Comment 60

[Robert Kaiser]

Ouch, it looks like I've missed the new DOMi release with German L10n just because I waited for getting a resolution on this issue. :(

Comment 61

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

Comment on attachment 445331 [details] [diff] [review]
patch9 [Checkin: comment 42, backed out: comment 61]

Oh yeah, I should have noted this, probably: the original attachment 445331 [details] [diff] [review] got backed out (http://hg.mozilla.org/dom-inspector/rev/38fccf86d9bb) for bug 565436 for the time being, since the events viewer was broken for non-trunk (as described in comments 50 and 51; further comments in bug 565436, comment 2). We should get this back in, but let's fix those issues with API usage before we resurrect this.

Recap:
Reland the functionality added by attachment 445331 [details] [diff] [review], but including:
- a fallback in the events viewer that will keep it working for current, non-trunk releases.
- the changes to viewer-registry.rdf in attachment 446962 [details] [diff] [review].

Comment 62

[Robert Kaiser]

Thanks for noting the backout - that's what I missed and what confused me. Everything's alright actually with German on the 2.0.6 release. :)

Comment 63

[alexander :surkov (:asurkov)]

Attachment: patch10

sorry, for being inactive, the patch works on 1.9.2 and 2.0

Comment 64

[alexander :surkov (:asurkov)]

Attachment: patch11

make faster event processing when application accessible is inspected in events viewer for Gecko 2.0

Comment 65

[[e-mail]]

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

>+        var accessible = accService.getAccessibleFor(document);
>+        do {
>+          if (!accessible.parent) {
>+            this.mDocPanel.subject = accessible;
>+            break;
>+          }
>+          accessible = accessible.parent;
>+        } while (accessible);
I think this can be simplified as follows:
var accessible = accService.getAccessibleFor(document);
while (accessible.parent)
  accessible = accessible.parent;
this.mDocPanel.subject = accessible;

>+  if ("rootDocument" in acc) {
>+    this.mDOMIRootDocumentAccessible = acc.rootDocument;
>+
>+    // We can skip accessible tree traversal for perf on Gecko 2.0 if the
>+    // inspected accessible is application accessible.
>+    this.canSkipTreeTraversal =
>+      (this.mAccessible == this.mDOMIRootDocumentAccessible.parent);
>+  }
>+  else {
>+    // Gecko 1.9.2 compatibility.
>+    while (acc) {
>+      var parent = acc.parent;
>+      if (parent)
>+        this.mDOMIRootDocumentAccessible = acc;
>+
>+      acc = parent;
>+    }
>+  }
>+
>+  this.mApplicationAccessible = this.mDOMIRootDocumentAccessible.parent;
Possibly looks slightly less confusing written this way:
if ("rootDocument" in acc) {
  this.mDOMIRootDocumentAccessible = acc.rootDocument;
  this.mApplicationAccessible = this.mDOMIRootDocumentAccessible.parent;

  // We can skip accessible tree traversal for perf on Gecko 2.0 if the
  // inspected accessible is application accessible.
  this.canSkipTreeTraversal =
    (this.mAccessible == this.mApplicationAccessible);
}
else {
  while (acc.parent) {
    this.mDOMIRootDocumentAccessible = acc;
    acc = acc.parent;
  }
  this.mApplicationAccessible = acc;
}

>+  var DOMNode = this.getDOMNodeFor(node.accessible);
>+  return DOMNode || node.accessible;
Nit: doesn't need a temporary, just use
return this.getDOMNodeFor(node.accessible) || node.accessible;

Comment 66

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

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

>+          else if (aEvent.subject instanceof nsIAccessibleApplication ||
>+                   aEvent.subject instanceof nsIAccessible &&
>+                   !aEvent.subject.parent) {
>+            // Update title for application accessible in compatible way for
>+            // Gecko 2.0 and 1.9.2.

Need to check for support of nsIAccessibleApplication here, too.

>+          <!-- Allow for DOM document and application accessible (compatible
>+            with Gecko 2.0 and 1.9.2). -->
...
>+          <!-- Allow for DOM document and application accessible (compatible
>+            with Gecko 2.0 and 1.9.2). -->

This is JS inside CDATA, not XML:

Warning: useless expression
Source File: chrome://inspector/content/ViewerRegistry.js
Line: 118

Comment 67

[[e-mail]]

With the filter syntax errors fixed it hung trying to select events to view :-(

Comment 68

[alexander :surkov (:asurkov)]

Attachment: patch12

addressed Neil's and Colby's comments.

Comment 69

[alexander :surkov (:asurkov)]

Attachment: patch13

patch12 was from other bug

Comment 70

[[e-mail]]

Comment on attachment 482449 [details] [diff] [review]
patch13

[Choose events to view is still hanging for me, but presumably unrelated.]

>+          if ("nsIAccessibleApplication" in Ci) {
>+            return object instanceof Ci.nsIAccessibleApplication;
>+          }
>+
>+          return object instanceof Ci.nsIAccessible;
Should you check for && !object.parent here too?

Comment 71

[alexander :surkov (:asurkov)]

(In reply to comment #70)
> Comment on attachment 482449 [details] [diff] [review]
> patch13
> 
> [Choose events to view is still hanging for me, but presumably unrelated.]

I can't reproduce. How can we dealt with this?

> >+          if ("nsIAccessibleApplication" in Ci) {
> >+            return object instanceof Ci.nsIAccessibleApplication;
> >+          }
> >+
> >+          return object instanceof Ci.nsIAccessible;
> Should you check for && !object.parent here too?

right, I'll fix it locally if you don't need a patch for this. Do you?

Comment 72

[[e-mail]]

Comment on attachment 482449 [details] [diff] [review]
patch13

r=me with the extra parent checks in both places.

Comment 73

[alexander :surkov (:asurkov)]

landed - http://hg.mozilla.org/dom-inspector/rev/af5badfecfca with Neil comment addressed