Aaron Leventhal Reporter
	Description • 19 years ago

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.

	Aaron Leventhal Reporter
	Comment 1 • 17 years ago

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

	Marco Zehe (:MarcoZ)
	Comment 2 • 17 years ago

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.

	alexander :surkov (:asurkov) Assignee
	Comment 3 • 17 years ago

I'll fix it once we get bug 368835.

	alexander :surkov (:asurkov) Assignee
	Comment 5 • 17 years ago

Marco, could you test this one?

	Aaron Leventhal Reporter
	Comment 6 • 17 years ago

Is a TreeViewChanged really a TreeInvalidated for the whole tree?

	Marco Zehe (:MarcoZ)
	Comment 7 • 17 years ago

(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!

	alexander :surkov (:asurkov) Assignee
	Comment 8 • 17 years ago

(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.

	Evan Yan
	Comment 9 • 17 years ago

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() ?

	Evan Yan
	Comment 10 • 17 years ago

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.

	alexander :surkov (:asurkov) Assignee
	Comment 11 • 17 years ago

(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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 12 • 17 years ago

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.

	alexander :surkov (:asurkov) Assignee
	Comment 13 • 17 years ago

fixed Evan, Olli comments

	alexander :surkov (:asurkov) Assignee
	Comment 14 • 17 years ago

(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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 15 • 17 years ago

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.

	alexander :surkov (:asurkov) Assignee
	Comment 16 • 17 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 17 • 17 years ago

that would be great.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 19 • 17 years ago

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

	alexander :surkov (:asurkov) Assignee
	Comment 20 • 17 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 21 • 17 years ago

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

Is there any way to write tests for all this?

Looks good to me anyhow.

	alexander :surkov (:asurkov) Assignee
	Comment 22 • 17 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 23 • 17 years ago

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.

	Mike Schroepfer
	Comment 24 • 17 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 25 • 17 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 26 • 17 years ago

Was there anything in particular you were worried about?

	Mike Schroepfer
	Comment 27 • 17 years ago

(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?

	alexander :surkov (:asurkov) Assignee
	Comment 28 • 17 years ago

(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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 29 • 17 years ago

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?

	alexander :surkov (:asurkov) Assignee
	Comment 30 • 17 years ago

(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.

	alexander :surkov (:asurkov) Assignee
	Comment 31 • 17 years ago

(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.

	Mike Schroepfer
	Comment 32 • 17 years ago

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

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 33 • 17 years ago

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

	alexander :surkov (:asurkov) Assignee
	Comment 34 • 17 years ago

(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.

	alexander :surkov (:asurkov) Assignee
	Comment 35 • 17 years ago

checked in

	Marco Zehe (:MarcoZ)
	Comment 36 • 17 years ago

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.

	alexander :surkov (:asurkov) Assignee
	Comment 37 • 17 years ago

Marco, does it helps?

	alexander :surkov (:asurkov) Assignee
	Comment 38 • 17 years ago

(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.

	alexander :surkov (:asurkov) Assignee
	Comment 39 • 17 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 40 • 17 years ago

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.

	Marco Zehe (:MarcoZ)
	Comment 41 • 17 years ago

(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.

	alexander :surkov (:asurkov) Assignee
	Comment 42 • 17 years ago

> 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.

	alexander :surkov (:asurkov) Assignee
	Comment 43 • 17 years ago

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

	alexander :surkov (:asurkov) Assignee
	Comment 44 • 17 years ago

checked in

	Marco Zehe (:MarcoZ)
	Comment 45 • 17 years ago

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

	alexander :surkov (:asurkov) Assignee
	Comment 46 • 17 years ago

mochitest has been added in bug 418043.