308564 - No accessibility events when data in a tree row changes

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

This is the general case for bug 291074.

We need to fire a name change event when data for a tree row changes -- the
focused one would be good enough.

Comment 1

[Aaron Leventhal]

Not high priority, but would be nice if there's a simple fix.

Comment 2

[Marco Zehe (:MarcoZ)]

Should be done for Thunderbird 3 if possible. Right now, if focusing to a Newsgroup, for example, the number of unread messages is not added/updated when the node's contents changes.

Comment 3

[alexander :surkov (:asurkov)]

I'll fix it once we get bug 368835.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: wip

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch

Marco, could you test this one?

Comment 6

[Aaron Leventhal]

Is a TreeViewChanged really a TreeInvalidated for the whole tree?

Comment 7

[Marco Zehe (:MarcoZ)]

(In reply to comment #5)
> Marco, could you test this one?

Works great! If I go onto a newsgroup and it updates, I nicely see the numbers of unread and total messages changing on the braille display now. So JAWS now receives the events properly, and the JAWS scripting language can deal with the NameChangeEvent properly for speech.

Good work!

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> Is a TreeViewChanged really a TreeInvalidated for the whole tree?
> 

technically it's a different things. TreeViewChanged when we set new view, TreeInvalidated when we repaint view. Though I do not fire any event when view is repainted entirely event nsITreeBoxObject::invalidate() is used (for example, in autocomple binding). I'm afraid it can be performance issue and probably it's not our case.

Comment 9

[Evan Yan]

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

r=me for accessible module.

nit:
>+    nsresult HandleTreeInvalidatedEvent(nsIDOMEvent *aEvent,
>+                                        nsIAccessible *aAccessible,
>+                                        nsIDOMNode *aTarget,
>+                                        const nsAString& aTargetName);

aTarget is not used, why not make the function prototype the same as HandleTreeRowCountChangedEvent() ?

Comment 10

[Evan Yan]

nsTreeBodyFrame::InvalidateXX() functions are called frequently. When just mouse hover over a message in Thunderbird, InvalidateRow() get called twice.

I'm leaving it to the layout module reviewer to decide whether that is the correct place to fire those events.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #9)

> aTarget is not used, why not make the function prototype the same as
> HandleTreeRowCountChangedEvent() ?
> 

It was used in earlier version of the patch. Eventually I forgot to remove it. Thanks for the catch

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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

>+  /**
>+   *
>+   */
>+  void treeViewInvalidated(in long aStartRow, in long aEndRow,
>+                           in long aStartCol, in long aEndCol);
>+};
Shouldn't you document this new method?

>+
>+[uuid(b71532f9-53b2-4647-a5b2-1c5f57e9aed6)]
>+interface nsPIAccessibleTreeItem : nsISupports
>+{
>+  /**
>+   * Get/set cached name.
>+   */
>+  attribute AString cachedName;
> };
I'm not sure about coding style in accessible/ but is it wise to add
an nsPIXXX interface to an public .idl?
Could you perhaps define that interface in nsXULTreeAccessible.h, since it is
used only in nsXULTreeAccessible.cpp, if I looked correctly. That way the obviously
internal interface would really stay as internal.

>+#ifdef ACCESSIBILITY
> void
> nsTreeBodyFrame::FireRowCountChangedEvent(PRInt32 aIndex, PRInt32 aCount)
FireRowCountChangedEvent is called only inside #ifdef ACCESSIBILITY, right?
>+  nsCOMPtr<nsIDOMEvent> event;
>+  domEventDoc->CreateEvent(NS_LITERAL_STRING("datacontainerevents"),
>+                           getter_AddRefs(event));
>+
>+  event->InitEvent(NS_LITERAL_STRING("TreeInvalidated"), PR_TRUE, PR_FALSE);
>+
>+  nsCOMPtr<nsIDOMDataContainerEvent> treeEvent(do_QueryInterface(event));
>+  if (!treeEvent)
>+    return;
You're missing OUT_OF_MEMORY check after CreateEvent. The easiest way is to just move
QI-to-nsIDOMDataContainerEvent to happen before event->InitEvent

>+  /**
>+   * Fires 'treeInvalidated' event asynchronously. The event supports
>+   * nsIDOMDataContainerEvent interface that is used to expose the information
>+   * structures described by method arguments.
>+   *
>+   * @param aStartRow  the start index of invalidated rows, -1 points columns
>+   *                   have been invalidated only
>+   * @param aEndRow    the end index of invalidated rows, non -1 value points
>+   *                   row range have been invalidated
>+   * @param aCol       the start index of invalidated columns, -1 points rows
>+   *                   have been invalidated
>+   * @param aEndCol    the end index of invalidated columns, non -1 value points
>+   *                   column range have been invalidated
>+   */
>+  void FireInvalidateEvent(PRInt32 aStartRow, PRInt32 aEndRow,
>+                           nsITreeColumn *aStartCol, nsITreeColumn *aEndCol);
>+#endif
For me, "non -1 value points...have been" doesn't sound good.
There isn't a parameter called aCol.
Comment talks about indexes, but third and fourth parameters aren't indexes but 
some objects.

Comment 13

[alexander :surkov (:asurkov)]

Attachment: patch2

fixed Evan, Olli comments

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #12)

> >+[uuid(b71532f9-53b2-4647-a5b2-1c5f57e9aed6)]
> >+interface nsPIAccessibleTreeItem : nsISupports
> >+{
> >+  /**
> >+   * Get/set cached name.
> >+   */
> >+  attribute AString cachedName;
> > };
> I'm not sure about coding style in accessible/ but is it wise to add
> an nsPIXXX interface to an public .idl?
> Could you perhaps define that interface in nsXULTreeAccessible.h, since it is
> used only in nsXULTreeAccessible.cpp, if I looked correctly. That way the
> obviously
> internal interface would really stay as internal.

This idl file is not public actually since it is used internally only. Also I would like to keep them as idl files. Possibly we should move some such interface to src folder. Olli what's worth to do to level up code redability?

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

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

>   nsCOMPtr<nsIDOMEvent> event;
>   domEventDoc->CreateEvent(NS_LITERAL_STRING("datacontainerevents"),
>                            getter_AddRefs(event));
>+  if (!event)
>+    return;
Don't add this, but just move the QI to nsIDOMDataContainerEvent and if-check here.

>+void
>+nsTreeBodyFrame::FireInvalidateEvent(PRInt32 aStartRowIdx, PRInt32 aEndRowIdx,
>+                                     nsITreeColumn *aStartCol,
>+                                     nsITreeColumn *aEndCol)
...
>+  nsCOMPtr<nsIDOMEvent> event;
>+  domEventDoc->CreateEvent(NS_LITERAL_STRING("datacontainerevents"),
>+                           getter_AddRefs(event));
>+  if (!event)
>+    return;
>+
>+  event->InitEvent(NS_LITERAL_STRING("TreeInvalidated"), PR_TRUE, PR_FALSE);
>+
>+  nsCOMPtr<nsIDOMDataContainerEvent> treeEvent(do_QueryInterface(event));
>+  if (!treeEvent)
>+    return;
Same here, no need to null check twice if you're anyway going to QI to nsIDOMDataContainerEvent.

Btw, next time you add new nsIDOMDataContainerEvent events, might be good to
add some helper method for event creation.

>+  /**
>+   * Fires 'treeInvalidated' event asynchronously. The event supports
>+   * nsIDOMDataContainerEvent interface that is used to expose the information
>+   * structures described by method arguments.
>+   *
>+   * @param aStartRow  the start index of invalidated rows, -1 points columns
>+   *                   have been invalidated only
>+   * @param aEndRow    the end index of invalidated rows, -1 points columns
>+   *                   have been invalidated only
>+   * @param aStartCol  the start invaidated column, -1 points rows have been
>+   *                   invalidated only
invaidated -> invalidated

>+   * @param aEndCol    the end invalidated column, -1 points rows have been
>+   *                   invalidated only
>+   */
>+  void FireInvalidateEvent(PRInt32 aStartRow, PRInt32 aEndRow,
>+                           nsITreeColumn *aStartCol, nsITreeColumn *aEndCol);
>+#endif

I still don't like "-1 points XXX have been invalidated only"
Perhaps "-1 means that only XXX have been invalidated" (if that is what you mean)?

aStartCol and aEndCol are objects so they can't get value -1.

Comment 16

[alexander :surkov (:asurkov)]

Olli, shoudl I put new patch before you'll continue the review?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

that would be great.

Comment 18

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

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

>+  /**
>+   * Fires 'treeInvalidated' event asynchronously. The event supports
>+   * nsIDOMDataContainerEvent interface that is used to expose the information
>+   * structures described by method arguments.
>+   *
>+   * @param aStartRow  the start index of invalidated rows, -1 means that
>+   *                   columns have been invalidated only
>+   * @param aEndRow    the end index of invalidated rows, -1 means that columns
>+   *                   have been invalidated only
>+   * @param aStartCol  the start invalidated column, nsnull means that only rows
>+   *                   have been invalidated
>+   * @param aEndCol    the end invalidated column, nsnull means that rows have
>+   *                   been invalidated only
>+   */
Use the same '...that only...' -style describing parameters

Comment 20

[alexander :surkov (:asurkov)]

Jonas, any plans for review? It would be nice to get this in Firefox3 since the bug fixes major issue when AT deal with xul trees (it's concerned thunderbird, firefox bookmarks). This bug is much similar  to the bug 368835 you gave sr+ already.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Is there any way to write tests for all this?

Looks good to me anyhow.

Comment 22

[alexander :surkov (:asurkov)]

We haven't a way for automated tests in ally module yet, I think bug 415730 should solve this.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ao it sounds like there are existing API tests, which presumably aren't run automatically. But that's a lot better than not having anything.

Comment 24

[Mike Schroepfer]

Jonas are the changes to nsTreeBodyFrame safe enough at this stage of the game?

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

They do look safe to me. All it does is fire events, and it holds off doing so until it's safe to execute script.

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Was there anything in particular you were worried about?

Comment 27

[Mike Schroepfer]

(In reply to comment #26)
> Was there anything in particular you were worried about?
> 

Mostly that the lack of unit test coverage here makes me unsure of what semantics might have accidentally changed. Is it possible to unit test the event changes?

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #27)
> (In reply to comment #26)
> > Was there anything in particular you were worried about?
> > 
> 
> Mostly that the lack of unit test coverage here makes me unsure of what
> semantics might have accidentally changed. Is it possible to unit test the
> event changes?
> 

Do you mean xpcshell test? IIRC it's not very comfortable to use it here because I need to have rendered xul:tree to get an accessible for it. It sounds mochitest will be fine for this but now some mochitests are failed with enabled accessibility.

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Any types of tests would be great, mochitest would be fine too which gives you a full browser. It should be possible to test if the events fire as they should since they are normal DOM events.

Though I think you need to turn them on, right? Which I suspect doesn't happen while running mochitest always.

Also, these are new events right? So there's not much worry in regressions in the events themselves? Are these events specced somewhere?

Comment 30

[alexander :surkov (:asurkov)]

(In reply to comment #29)
> Any types of tests would be great, mochitest would be fine too which gives you
> a full browser. It should be possible to test if the events fire as they should
> since they are normal DOM events.
> 
> Though I think you need to turn them on, right? Which I suspect doesn't happen
> while running mochitest always.

I can't handle these events until accessibility is enabled. But once accessibility is enabled (for example, when I get nsIAccessibleRetrieval service in mochitest) then some another mochitests fail.

> Also, these are new events right? So there's not much worry in regressions in
> the events themselves? Are these events specced somewhere?
> 

These are new events and there shoudn't be regression in events themselves. These events aren't documented partially because they are supposed to be used by accessibility internally.

Comment 31

[alexander :surkov (:asurkov)]

(In reply to comment #30)
> I can't handle these events until accessibility is enabled. But once
> accessibility is enabled (for example, when I get nsIAccessibleRetrieval
> service in mochitest) then some another mochitests fail.

I wrote mochitest for bug 368835 which is similar to this one but I was forced to disable it due to some tests fail. Therefore I don't know how can I add test for this bug.

Comment 32

[Mike Schroepfer]

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

hrm - I would like to figure out how to get test coverage on this.

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sounds bad that turning on accessibility makes us fail tests. Won't that mean that some sites might breat if you turn on accessibility?

Comment 34

[alexander :surkov (:asurkov)]

(In reply to comment #33)
> Sounds bad that turning on accessibility makes us fail tests. Won't that mean
> that some sites might breat if you turn on accessibility?
> 

it's possible of course, either web or firefox UI.

Comment 35

[alexander :surkov (:asurkov)]

checked in

Comment 36

[Marco Zehe (:MarcoZ)]

This fix does not work. The original patch, attachment 293296 [details] [diff] [review], did work as expected, however the final fix does not. Also, I am now no longer seeing automatic updates to ListItems such as when downloading a file in DM, but have to investigate whether that's actually a regression from this checkin.

Comment 37

[alexander :surkov (:asurkov)]

Attachment: patch__1

Marco, does it helps?

Comment 38

[alexander :surkov (:asurkov)]

(In reply to comment #37)
> Created an attachment (id=302596) [details]
> patch__1
> 
> Marco, does it helps?
> 

I tested this on bookmarks and I get an name changed event in Gecko layer when I change bookmark name.

Comment 39

[alexander :surkov (:asurkov)]

I put mochitest for this to the patch of bug 416553.

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 302596 [details] [diff] [review]
patch__1

> nsTreeBodyFrame::InvalidateRow(PRInt32 aIndex)
...
>+#ifdef ACCESSIBILITY
>+  nsIPresShell *presShell = PresContext()->PresShell();
>+  if (presShell->IsAccessibilityActive())
>+    FireInvalidateEvent(aIndex, aIndex, nsnull, nsnull);
>+#endif
So what happened to this? It was in the previous patch too.

>-    treeEvent->SetData(NS_LITERAL_STRING("column"), startColVariant);
>+    treeEvent->SetData(NS_LITERAL_STRING("startcolumn"), startColVariant);

>-    treeEvent->SetData(NS_LITERAL_STRING("columncount"), endColVariant);
>+    treeEvent->SetData(NS_LITERAL_STRING("endcolumn"), endColVariant);
I should have caught these when reviewing the previous patch :/ 

Can't really test accessibility code though.
There really should be automated tests for the accessibility events.

Comment 41

[Marco Zehe (:MarcoZ)]

(In reply to comment #38)
> > Marco, does it helps?
> I tested this on bookmarks and I get an name changed event in Gecko layer when
> I change bookmark name.

Yes, with that followup patch in, it works!

(In reply to comment #40)
> There really should be automated tests for the accessibility events.

There will be, soon! See bug 415730 and bug 416553.

Comment 42

[alexander :surkov (:asurkov)]

> So what happened to this? It was in the previous patch too.

Possibly it's incorrect merging.

> I should have caught these when reviewing the previous patch :/ 

me too, possibly I was drunk on smoked up :)

> Can't really test accessibility code though.
> There really should be automated tests for the accessibility events.

I included mochitest to bug 416553 for the part of this bug.

Comment 43

[alexander :surkov (:asurkov)]

Jonas, it's simple patch, I think rs+ would be enough.

Comment 44

[alexander :surkov (:asurkov)]

checked in

Comment 45

[Marco Zehe (:MarcoZ)]

Verified using version 3.0a1pre (2008021303). Great work!

Comment 46

[alexander :surkov (:asurkov)]

mochitest has been added in bug 418043.