388203 - catch accessibility events in DOMi

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[alexander :surkov (:asurkov)]

Discussion is on http://groups.google.com/group/mozilla.dev.accessibility/browse_thread/thread/53df58d29cbf251c

Possibly we can have for this two views (for each panel).

View in the right panel will catch all events. The target of event (accessible object and probably related DOM node) can be inspected by left panel views.

View in the left panel will catch only those events that are targeted by inspected accessible. The target of accessible event can be expected by proposed tabable interface (bug 388201).

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

It's a simplest version. More complex way we can implement in following up bugs.

Comment 2

[Shawn Wilsher :sdwilsh]

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

Sorry for the delay on this...

>+      <rdf:Description ins:uid="accessibleEvents"
>+                       ins:panels="bxDocPanel"
>+                       ins:description="Accessible Events">
>+        <ins:filter>
>+        <![CDATA[
>+          return object instanceof Components.interfaces.nsIDOMDocument;
>+          ]]>
>+        </ins:filter>
nit: <![CDATA[ immediately after <ins:filter> and ]]> before </ins:filter> as the rest of the file please.

>+//////////// global variables /////////////////////
nit: like this please http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/inspector/resources/content/viewers/domNode/domNodeDialog.js&rev=1.1#38 (same for everywhere else in the file please)

>+    //this.onItemSelected();
o_O

>+  ////////////////////////////////////////////////////////////////////////////
>+  //// stuff
Could we get a slightly better comment please?

>+  this.mAccService = Components.classes["@mozilla.org/accessibleRetrieval;1"].
>+                     getService(nsIAccessibleRetrieval);
So, you are mixing to styles here, and I'd like you to choose one.  Either declare Cc and Ci up top, then you can have the period after ], or have the period on the next line and line it up with the period after Components.

>+  this.mObserverService = Components.classes["@mozilla.org/observer-service;1"].
>+                          getService(nsIObserverService);
ditto

Comment 3

[Shawn Wilsher :sdwilsh]

forgot - Can we see some screenshots of the UI please?

Comment 4

[Alex Vincent [:WeirdAl]]

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

>Index: accessible/public/nsIAccessibleRetrieval.idl
>+  /**
>+    * Returns type of accessible event as a string.
>+    *
>+    * @param aEventType - the accessible event type constant.
>+    */
>+  AString getStringEventType(in unsigned long aEventType);

Line up the asterisks with the first one, please (like the rest of this file).

>Index: accessible/src/base/nsAccessibilityService.cpp
>+NS_IMETHODIMP
>+nsAccessibilityService::GetStringEventType(PRUint32 aEventType,
>+                                           nsAString& aString)
>+{

Perhaps add a one-line comment naming the interface this is implementing?  (Not required, just advisory.  I hate having to figure out which interface a method is for.)

>Index: accessible/src/base/nsAccessibilityService.h

>+class nsAccessibilityService : public nsIAccessibilityService, 
>+                               public nsIObserver,
>+                               public nsIWebProgressListener,
>+                               public nsSupportsWeakReference
>+{

...

>-class nsAccessibilityService : public nsIAccessibilityService, 
>-                               public nsIObserver,
>-                               public nsIWebProgressListener,
>-                               public nsSupportsWeakReference
>-{

Did anything change in this class declaration, other than its placement in the document?  If so, maybe now's a good time to add some JavaDoc.  If not, why was it moved, messing up CVS blame?

>Index: extensions/inspector/jar.mn
>+  content/inspector/viewers/accessibleEvents/accessibleEvents.js      (resources/content/viewers/accessibleEvents/accessibleEvents.js)
>+  content/inspector/viewers/accessibleEvents/accessibleEvents.xul     (resources/content/viewers/accessibleEvents/accessibleEvents.xul)

...

>Index: extensions/inspector/resources/content/viewers/accessibleEvents/accessibleEvents.JS

The file names don't match (.js versus .JS).  This would be bad for case-sensitive operating systems.

>+  ////////////////////////////////////////////////////////////////////////////
>+  //// stuff

Great description here :)

>+AccessibleEventsView.prototype.observe =
>+function observe(aSubject, aTopic, aData)
>+{
>+    time: date.getHours() + ":" + date.getMinutes() + ":" + date.getSeconds() +
>+          ":" + date.getMilliseconds()

May I suggest date.toLocaleTimeString() perhaps?  (It doesn't record milliseconds, though.)

>Index: extensions/inspector/resources/content/viewers/accessibleEvents/accessibleEvents.xul
>+  <!ENTITY % dtd3 SYSTEM "chrome://inspector/locale/viewers/accessibleEvents.dtd"> %dtd3;

Where's the jar.mn entry for this?

>Index: extensions/inspector/resources/locale/en-US/viewers/accessibleEvents.dtd
>+<!-- ***** BEGIN LICENSE BLOCK *****
>+#if 0
...
>+#endif
>+   - ***** END LICENSE BLOCK ***** -->

This leaves the "BEGIN LICENSE BLOCK", "END LICENSE BLOCK" lines in the post-processed file.  If you're going to remove the license block, please remove these lines too.

r- for the missing jar.mn entry for accessibleEvents.dtd.  The rest of this looks good, but neither sdwilsh nor I have tested this patch on our boxes.

Comment 5

[Alex Vincent [:WeirdAl]]

cc'ing Pike; there's been some dispute over whether the exclude the "BEGIN LICENSE BLOCK", "END LICENSE BLOCK" lines.  sdwilsh says Pike knows best - care to weigh in?

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 7

[alexander :surkov (:asurkov)]

Attachment: screenshot

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #2)

> >+  ////////////////////////////////////////////////////////////////////////////
> >+  //// stuff
> Could we get a slightly better comment please?

I called it 'utils'. I don't know how much is better :).

> So, you are mixing to styles here, and I'd like you to choose one.  Either
> declare Cc and Ci up top, then you can have the period after ], or have the
> period on the next line and line it up with the period after Components.

I used XPCU in new patch. Ok?

Comment 9

[Axel Hecht]

The preprocessor macros should go inside the xml comments. There's no good reason to make the source files malformed xml.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #4)

> Did anything change in this class declaration, other than its placement in the
> document?  If so, maybe now's a good time to add some JavaDoc.  If not, why was
> it moved, messing up CVS blame?

Initialy there wasn't any changes there. I moved the class definition on top of file because nobody will look at those array. Now I added some javadoc styled comments and remove dead method declarations.

> >+  ////////////////////////////////////////////////////////////////////////////
> >+  //// stuff
> 
> Great description here :)

is utils better? :)

> May I suggest date.toLocaleTimeString() perhaps?  (It doesn't record
> milliseconds, though.)

ok, changed.

> >Index: extensions/inspector/resources/content/viewers/accessibleEvents/accessibleEvents.xul
> >+  <!ENTITY % dtd3 SYSTEM "chrome://inspector/locale/viewers/accessibleEvents.dtd"> %dtd3;
> 
> Where's the jar.mn entry for this?

I forgot this. Thank you for the catch.

Comment 11

[Alex Vincent [:WeirdAl]]

Comment on attachment 273221 [details] [diff] [review]
patch2

>Index: accessible/public/nsIAccessibleRetrieval.idl
>+  /**
>+   * Returns type of accessible event as a string.
>+   *
>+   * @param aEventType - the accessible event type constant.
>+   */
>+  AString getStringEventType(in unsigned long aEventType);

Nit - no @return line.  (You could change the word "Returns" to "Get the", if you wanted.)

>Index: accessible/src/base/nsAccessibilityService.h
>+  /**
>+   * Return presentation shell for the given node.
>+   *
>+   * @param aNode - the given DOM node.
>+   */
>+  static nsresult GetShellFromNode(nsIDOMNode *aNode,
>+                                   nsIWeakReference **weakShell);
>+
>+  /**
>+   * Return accessibility service (static instance of this class).
>+   */
>+  static nsresult GetAccessibilityService(nsIAccessibilityService** aResult);
>+
>+private:
>+  /**
>+   * Return presentation shell, DOM node for the given frame.
>+   *
>+   * @param aFrame - the given frame
>+   * @param aRelatFrame [out] - the given frame casted to nsIFrame

Typo - this doesn't match the argument name.

>+   * @param aShell - presentation shell for DOM node associated with the
>+   *                 given frame
>+   * @param aContent - DOM node associated with the given frame
>+   */
>+  nsresult GetInfo(nsISupports *aFrame, nsIFrame **aRealFrame,
>+                   nsIWeakReference **aShell,
>+                   nsIDOMNode **aContent);

Your JavaDoc is inconsistent here.  The first names one argument (of two, the second being the real retval), the second doesn't name any arguments (where the first is the retval) and the third names four arguments (the fourth being the retval).  I'm confused.  :)  Also, [out] for aRealFrame, but not for aContent or aShell?

In a situation like this, I think it's okay to say |@return aContent| instead of |@param aContent|.  But I'll defer to other reviewers on that.  Three @return lines for one function might raise an eyebrow, even if it's totally accurate and the intent.

>+  /**
>+   * Initialize an accessible and cache it. The method should be called for
>+   * every created accessible.
>+   *
>+   * @param aAccessibleIn - accessible to initialize.
>+   */
>+  nsresult InitAccessible(nsIAccessible *aAccessibleIn, nsIAccessible **aAccessibleOut);

This calls for initialization checks (NS_ERROR_NOT_INITIALIZED, NS_ERROR_ALREADY_INITIALIZED as return codes) throughout the implementation... but I'd say that's a different bug/patch.  One issue per bug and all that.

>+  /**
>+   * Return accessible object for elements implementing nsIAccessibleProvider
>+   * interface.
>+   *
>+   * @param aNode - DOMNode that accessible is returned for.
>+   */

"DOM Node", please.  Also, I have to admit, "DOMNode that accessible is returned for." doesn't make a whole lot of sense to me.  It needs better wording, but I don't know what to put here.

>+  "active decendent change",                 // EVENT_ACTIVE_DECENDENT_CHANGED

Bug 106386.

>+  "defaction change",                        // EVENT_DEFACTION_CHANGE

???  Can someone tell me what this means, and if it's an actual word?

>+  "hypertext nlinks changed",                // EVENT_HYPERTEXT_NLINKS_CHANGED

Ditto (but I bet this is correct, somehow).

>Index: extensions/inspector/resources/content/viewers/accessibleEvents/accessibleEvents.xul
>+  <tree id="olAccessibleEvents" class="plain" flex="1"
>+        onselect="viewer.onItemSelected()">

Nit:  Other elements in this file give each attribute its own line.  Your call if you want to do it here.

r=ajvincent with these nits addressed.

Comment 12

[alexander :surkov (:asurkov)]

Attachment: patch3

with alex's nits fixed

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #11)

> Your JavaDoc is inconsistent here.  The first names one argument (of two, the
> second being the real retval), the second doesn't name any arguments (where the
> first is the retval) and the third names four arguments (the fourth being the
> retval).  I'm confused.  :)  Also, [out] for aRealFrame, but not for aContent
> or aShell?
> 
> In a situation like this, I think it's okay to say |@return aContent| instead
> of |@param aContent|.  But I'll defer to other reviewers on that.  Three
> @return lines for one function might raise an eyebrow, even if it's totally
> accurate and the intent.

I added [out] for rest arguments.

> >+  /**
> >+   * Initialize an accessible and cache it. The method should be called for
> >+   * every created accessible.
> >+   *
> >+   * @param aAccessibleIn - accessible to initialize.
> >+   */
> >+  nsresult InitAccessible(nsIAccessible *aAccessibleIn, nsIAccessible **aAccessibleOut);

> This calls for initialization checks (NS_ERROR_NOT_INITIALIZED,
> NS_ERROR_ALREADY_INITIALIZED as return codes) throughout the implementation...
> but I'd say that's a different bug/patch.  One issue per bug and all that.

I'll file following up bug for this.

> >+  /**
> >+   * Return accessible object for elements implementing nsIAccessibleProvider
> >+   * interface.
> >+   *
> >+   * @param aNode - DOMNode that accessible is returned for.
> >+   */
> 
> "DOM Node", please.  Also, I have to admit, "DOMNode that accessible is
> returned for." doesn't make a whole lot of sense to me.  It needs better
> wording, but I don't know what to put here.

DOM node can have associated accessible objects exposed for AT. The method returns such accessible object.

> >+  "active decendent change",                 // EVENT_ACTIVE_DECENDENT_CHANGED
> 
> Bug 106386.

it goes from IA2, I'll write a letter them to get rid it.

> >+  "defaction change",                        // EVENT_DEFACTION_CHANGE
> 
> ???  Can someone tell me what this means, and if it's an actual word?

it's a default action :). I fixed this in proposed method.

> >+  "hypertext nlinks changed",                // EVENT_HYPERTEXT_NLINKS_CHANGED
> 
> Ditto (but I bet this is correct, somehow).
> 

fixed too.

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch

patch3 is wrong patch. sorry.

Comment 15

[[e-mail]]

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

>+AccessibleEventsView.prototype.getCellText =
>+function getCellText(aRow, aCol)
>+{
>+  if (aCol.id == "olcEventType")
>+    return this.mEvents[aRow].type;
>+  else if (aCol.id == "olcEventTime")
>+    return this.mEvents[aRow].time;
>+  else if (aCol.id == "olcEventTargetNodeName")
>+    return this.mEvents[aRow].nodename;
>+  return "";
>+}
You don't need else after return. At least you managed not to put
else
  return "";

Comment 16

[Evan Yan]

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

r=me for change to mozilla/accessible

Comment 17

[alexander :surkov (:asurkov)]

Attachment: patch5

with Neil's comment, updated to trunk

Comment 18

[alexander :surkov (:asurkov)]

Damon, usually you grant an approval for accessibility bugs. Here I need to get an approval for accessibility part of my patch only because another part of patch (UI features) don't need an approval (per bug 386818 comment #40).

Comment 19

[alexander :surkov (:asurkov)]

checked in