368835 - No focus events from xul tree table when a row is deleted

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Lynn Monsanto]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.1) Gecko/20060601 Firefox/2.0.0.1 (Ubuntu-edgy)
Build Identifier: version 3 alpha 1 (20070130)

Now table focus changed events are fired by Thunderbird when a message is deleted from the message header table.  This means that assistive technologies cannot report which message was deleted, and which message table cell now has focus. This is a critical bug because blind users do not know when a message is deleted. This can cause users to loose mail messages.

Reproducible: Always

Steps to Reproduce:
1. Enable accessibility on the desktop.
2. Start at-poke and enable event logging.
3. Delete a message from the message header table. You will see that events are received from the new document frame, but not from the header table.
Actual Results:  
See above.

Expected Results:  
The message header table should fire focus changed events.

Comment 1

[Evan Yan]

confirmed.
hit "Delete" key to delete a message, no focus event on the new focused message.
if delete message by popup menu or toolbar delete button, new focus event received.

Comment 2

[Mike Pedersen]

Hi Evan, thanks for confirming this.  The thing that makes this a really nasty problem is that it could cause a user to delete messages that they didn't intend to because he or she wouldn't realize that a new message had gained focus before pressing delete again.  

Comment 3

[Mike Cowperthwaite]

Lynn, please stop marking access bugs as critical.  They're not crashing, they're not hanging, they're not causing data loss.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Evan, have you dug into this since your last comment?  Since most AT do key off of focus events, Mike is of course correct that this is a high impact nasty bug. I'm not familiar with our event system (yet) but am considering digging into this one.  Let me know...

Comment 5

[David :Bienvenu]

Maybe I'm confusing focus events - for TB, focus is really pane based, and deleting a message doesn't change the pane focus. Does hitting the down arrow change the focus that you're talking about? Is the issue for the table that the same row index is selected as was selected before?

Seems like a selection change event is what's interesting...

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

In GNOME, AT can expect focus events at the widget level. You are correct I think, that in Mozilla land we are talking about selections. These get converted to focus events in the accessible module AFAIK.

So basically we need to make sure in the end, an ATK focus event happens.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Update: I think I've narrowed this down to SetFocusThreadPaneIfNotOnMessagePane (in mail3PaneWindowCommands.js)... I need to do more testing. I should have a patch tomorrow at latest.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #5)
David Bienvenu, hitting the down arrow does cause a focus event (confirmed via at-poke on Linux). I'm not sure why ending up on a new row via a delete operation is not also causing a focus event.

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Weird, looks like a focus event is firing all the way to atk_focus_tracker_notify.  Not sure why at-poke is reporting. Perhaps this is catching (in atk_focus_tracker_notify from libatk.so):
  if (object == previous_focus_object)
    return;

Yet when I delete the message via the popup menu I get a focus event for the message that was deleted, but not the newly post-delete selected message. I think this is because the popup gets focus, then the message-to-delete gets focus, then it gets deleted, then... no newly selected message focus.

Message deletion in tbird is asynchronous so this could be a tricky timing issue.
(Note to self: don't promise patches in writing -- comment 7)

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

This appears to be a core bug.  I have confirmed the exact same behaviour in the FF2.0 bookmarks organizer. There might be something odd happening when a selection happens on a row that ends up with the same index as the previous row that was selected. We probably need to track that logic down and fix (if indeed that is where the problem lies).

(Thanks David Bienvenu for the suggestion to test this on Firefox bookmarks!)

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

FWIW David Bienvenu originally considered the "same row" issue in comment #5 (FWIW).

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

I think I've narrowed this down to nsXULTreeAccessible::GetCachedTreeitemAccessible. I think it is erroneously returning the "deleted" row.

We are hashing on aRow * kMaxTreeColumns + columnIndex. After deleting a row, the next row to be selected shuffles up and now has the same hash key. So when we try to grab the accessible from the cache, we get the same "deleted" accessible.

So I'm thinking we need to update the cache when a row is deleted (if we indeed are not already).

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Making note of bug 201922 for help in understanding the history and issues around the tree cache.

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: proposed fix

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 255139 [details] [diff] [review]
proposed fix

I've confirmed this patch fixes the bug (via at-poke).

Comment 16

[Aaron Leventhal]

Comment on attachment 255139 [details] [diff] [review]
proposed fix

Change to more generic name like TreeItemsChanged. 
Make sure CPU doesn't go crazy after each delete. We probably shouldn't rebuild all the tree's accessibles after each delete
* Deal with other changes besides delete

Also I can mostly only review mozilla/accessible changes.

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch in progress

Thanks Aaron. I'll look into optimizations and then seek appropriate review. And thanks reminding me of your similar work from another bug!

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 255143 [details] [diff] [review]
patch in progress

This patch addresses Aaron's concerns. Optimization of accessible tree cache updating, if possible will happen later on a separate bug issue.

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 255143 [details] [diff] [review]
patch in progress

cancelling review while discussing performance on irc

Comment 20

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

Just to remind that nsFrame::FireDOMEvent dispatches event asynchronously. Perhaps event dispatching could be batched so that
only one event is created when nsTreeBodyFrame::RowCountChanded is called several times in a row.

Comment 21

[David Bolter [:davidb] (NeedInfo me for attention)]

Smaug, that's right thanks. This makes timing large numbers of row additions/deletions less useful (but I tried it anyway to confirm).

Running top (on linux), and performing a 1000 row addition test, I do see a longer CPU spike with my patch than without it. Loading my xul test file into Thunderbird and running it performs the row additions in about 1.5 seconds. The CPU spike lasts about 10 seconds.

When dealing with many events one thing I've had some success with in the past is to create an internal queue of events which gets processed in an onidle handler. But I think your idea of batching is interesting, could you expand on that?

Would it involve a timer? Perhaps RowCountChanged starts a timer, and the next time it gets called it restarts the timer, and only if/when the timer counts down then the event fires? This is a bit yucky but might work.

If we batch based on say every n RowCountChanged events, then we won't be alerted of important changes that happen infrequently.

Comment 22

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

I think you could create nsPLDOMEvent (or something similar)
(like here http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrame.cpp#1642)
and keep a reference to it. And if there is the event pending, changing
rowcount shouldn't cause new events.
The event should of course somehow notify the caller that it has
been dispatched.

Comment 23

[David Bolter [:davidb] (NeedInfo me for attention)]

Interesting.

Since I can't change the nsPLDOMEvent::Run method to directly alert the caller I guess we are dealing with the idea of perhaps making the caller a listener for the event? So we (nsTreeBodyFrame) fire a RowCountChanged, then go into an unwilling-to-fire-RowCountChanged mode, then when we next receive a RowCountChanged event (which we probably fired) we go back into willing-to-fire-RowCountChanged events mode.  

Worth a try or flawed understanding of nsPLDOMEvents?

Comment 24

[Joanmarie Diggs]

ping :-)

Comment 26

[Evan Yan]

All treeitem accessibles use the dom node of the tree as its mDOMNode. When focus to a row, we are actually fire focus event against the tree accessible. 

So not only when deleting a message, but also when arrowing down to another message, focus event is always prevented by atk_focus_tracker_notify (refer to comment #9).

Update treeitem accessible cache is another problem.

Actually we didn't update treeitem accessible cache all the time, like change sorting column, changing column order. We didn't get "TreeViewChanged" event at all. (do we really should rebuild the accessible tree when the _view_ changed while the _model_ is the same?)

Comment 27

[Evan Yan]

(In reply to comment #26)
> when arrowing down to another
> message, focus event is always prevented by atk_focus_tracker_notify (refer to
> comment #9).

that's a regression, bug 386161 filed for this.

Comment 28

[Evan Yan]

Two problems are mentioned in comment #26. The first problem has been fixed in bug 386161. For the second problem, treeitem accessible cache issue, I want to make it more clear here.

We're supposed to update treeitem accessible cache when we got "TreeViewChanged" event. However, as my testing, we didn't get any "TreeViewChanged" event when changing tree view, like sorting message, moving column, etc. That's to say, we didn't update cache when tree view changed.

But that works. After tree view changed, treeitems are at new positions. Though we exposed the old treeitm accessibles, those treeitem accessibles will return the infomation of the updated treeitem at the position.

Say, we have a xul tree table and its accessible tree looks like below.

      xul tree table            accessible tree
           A                           1
           B                           2
           C                           3

At first, 1.getName() returns A, 2.getName() returns B. Then tree view changed. The xul tree table and its accessible tree will looks like below.

      xul tree table(changed view)       accessible tree(still the same)
           C                                    1
           B                                    2
           A                                    3

Now, 1.getName() returns C. The old treeitem accessible bound to the new DOM treeitem. So actually we don't need to update the treeitem accessible cache when tree view changed.

For this bug, the issue of deleting a row, of course it will work if we update the entire tree cache. But we have performance concern about that. Another alternative is, don't need to update the cache, just work around the atk code of preventing focus event to the same object.

Actually, when selecting a row or deleting a row in xul tree table, there is "select" dom event received in nsRootAccessible::HandleEventWithTarget(), but we didn't fire SELECTION_CHANGED atk event for that. If we do that, I think it probably can solve the problem.

Comment 29

[David Bolter [:davidb] (NeedInfo me for attention)]

Evan, sorry I missed your recent comments and thanks for the information about the tree cache implementation... this sounds promising.

Regarding comment #26, the first problem... it is only fixed for arrowing correct?  Do you think if we change (pseudocode) in atk_focus_tracker_notify

- if (object == previous_focus_object)
+ if ( !aTree && (object == previous_focus_object))
    return;

It should solve it?  (See also comment #9)

(Note I'm off until Tuesday)

Comment 30

[Evan Yan]

(In reply to comment #29)
> Regarding comment #26, the first problem... it is only fixed for arrowing
> correct?

Yes.

> Do you think if we change (pseudocode) in atk_focus_tracker_notify
> 
> - if (object == previous_focus_object)
> + if ( !aTree && (object == previous_focus_object))
>     return;
> 
> It should solve it?  (See also comment #9)
>
I think so, but I haven't try it.
I believe there is a reason for atk to do so, and not sure whether we can hack this in atk side.

Comment 31

[Marco Zehe (:MarcoZ)]

*ping* :-)

Comment 32

[alexander :surkov (:asurkov)]

(In reply to comment #28)

> We're supposed to update treeitem accessible cache when we got
> "TreeViewChanged" event. However, as my testing, we didn't get any
> "TreeViewChanged" event when changing tree view, like sorting message, moving
> column, etc. That's to say, we didn't update cache when tree view changed.
>

TreeViewChanged is fired when new tree view is setted up. We need an event when the current tree view is changed (not replaced by new one). It's better to have events to know what exactly has been changed to do not update the accessible entirely. I think we should file a bug to request new events.

It looks it's important feature. For example, we should file show/hide events when tree item is collapsed or expanded.

Comment 33

[David Bolter [:davidb] (NeedInfo me for attention)]

There are some really good comments here. To bring this bug back into focus... I think we just need to circumvent the atk_focus_tracker_notify same object check for cases where focus has changed after a tree mutation. I'll try to make time to check this in the next 24hrs.

Comment 34

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #32)
> (In reply to comment #28)
> 
> > We're supposed to update treeitem accessible cache when we got
> > "TreeViewChanged" event. However, as my testing, we didn't get any
> > "TreeViewChanged" event when changing tree view, like sorting message, moving
> > column, etc. That's to say, we didn't update cache when tree view changed.
> >
> 
> TreeViewChanged is fired when new tree view is setted up. We need an event when
> the current tree view is changed (not replaced by new one). It's better to have
> events to know what exactly has been changed to do not update the accessible
> entirely. I think we should file a bug to request new events.
> 

If I understand correctly, TreeViewChanged is meant for when the whole view has changed or just started? So we need an event to capture more minor changes?

> It looks it's important feature. For example, we should file show/hide events
> when tree item is collapsed or expanded.
> 

I agree that we need to make sure we provide events for when the view has been updated. Shall we track this in a new bug?

Comment 35

[alexander :surkov (:asurkov)]

(In reply to comment #34)

> If I understand correctly, TreeViewChanged is meant for when the whole view has
> changed or just started? So we need an event to capture more minor changes?

Right. It's one problem we cache treeview and therefore we should listen this event (http://lxr.mozilla.org/mozilla/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#517.)

> I agree that we need to make sure we provide events for when the view has been
> updated. Shall we track this in a new bug?
> 

I guess yes.

Actually we have not problem if we do not update cache because our accessible for treeitems objects are wrappers for methods of nsITreeView. The one bad thing we could get is our cache is bigger than total number of treeitems. We need some modification events to fire show/hide events as I said before and to fix bugs like this one.

Comment 36

[alexander :surkov (:asurkov)]

(In reply to comment #35)

> Actually we have not problem if we do not update cache because our accessible
> for treeitems objects are wrappers for methods of nsITreeView. The one bad
> thing we could get is our cache is bigger than total number of treeitems.

Though it seems contradict with Evan's comment #28.

Comment 37

[Evan Yan]

(In reply to comment #36)
> (In reply to comment #35)
> 
> > Actually we have not problem if we do not update cache because our accessible
> > for treeitems objects are wrappers for methods of nsITreeView. The one bad
> > thing we could get is our cache is bigger than total number of treeitems.
> 
> Though it seems contradict with Evan's comment #28.

I guess not. As I commented in c#28 as below

(In reply to comment #28)
> Now, 1.getName() returns C. The old treeitem accessible bound to the new DOM
> treeitem. So actually we don't need to update the treeitem accessible cache
> when tree view changed.
> [...]
> Another alternative is, don't need to update the cache, just work around the atk code
> of preventing focus event to the same object.

Comment 38

[alexander :surkov (:asurkov)]

(In reply to comment #35)

> > I agree that we need to make sure we provide events for when the view has been
> > updated. Shall we track this in a new bug?
> > 
> 
> I guess yes.

I didn't notice here there was discussion about event firing. So we can continue a discussion here.

I think all we need actually is to fire an event when row count changed like you discussed above. Though we should fire one event for one RowCountChanged() method because if we will fire for every treeitem on toggle operation then it may be performance issue. But then how we can pass additional info with an event? Should we use DOM events here or some another notification approach?

Olli, what do you think?

Comment 39

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

(In reply to comment #38)
> But then how we can pass additional info with an
> event? Should we use DOM events here or some another notification approach?

If accessibility is anyway using DOM events for notifications, I think it is ok 
here too. If you need to add some additional data to events, just create new
event type (implemented like smartcardevents for example) and dispatch
those events. When dispatching events make sure it is happening in a "safe time". Dispatching events may kill the related nsFrame object.
(FireDOMEvent is a safe method since it is asynchronous, but that method is
ofc only for common events, without any extra data.)

Comment 40

[David Bolter [:davidb] (NeedInfo me for attention)]

Note the second patch introduced a new event name but RowCountChanged sounds good to me too.

The main thing for this bug is to make sure we don't have to update the tree accessible cache for each tree mutation, and to make sure that atk_focus_tracker_notify fires when we need it to.

Comment 41

[alexander :surkov (:asurkov)]

(In reply to comment #40)

> The main thing for this bug is to make sure we don't have to update the tree
> accessible cache for each tree mutation,

Why and then when we should and should we?

> and to make sure that
> atk_focus_tracker_notify fires when we need it to.
> 

btw, can you give me some extract of previous discussion. Why do we need focus event? Why are show/hide events unsuitable here? Do we need focus event for this case at all, for example, does xul:listbox fires focus event in similar situation?

Comment 42

[alexander :surkov (:asurkov)]

(In reply to comment #39)
> (In reply to comment #38)
> > But then how we can pass additional info with an
> > event? Should we use DOM events here or some another notification approach?
> 
> If accessibility is anyway using DOM events for notifications, I think it is ok 
> here too. If you need to add some additional data to events, just create new
> event type (implemented like smartcardevents for example) and dispatch
> those events.

Ok. Where is better place to keep new event class? content/events I guess, right? At least there are defined another events and there I can use nsDOMEvent ot simplify new event class implementation.

 When dispatching events make sure it is happening in a "safe
> time". Dispatching events may kill the related nsFrame object.
> (FireDOMEvent is a safe method since it is asynchronous, but that method is
> ofc only for common events, without any extra data.)
> 

I think I can steal idea from nsPLDOMEvent implementation.

Comment 43

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #41)
> (In reply to comment #40)
> 
> > The main thing for this bug is to make sure we don't have to update the tree
> > accessible cache for each tree mutation,
> 
> Why and then when we should and should we?
> 
> > and to make sure that
> > atk_focus_tracker_notify fires when we need it to.
> > 
> 
> btw, can you give me some extract of previous discussion. Why do we need focus
> event? Why are show/hide events unsuitable here? Do we need focus event for
> this case at all, for example, does xul:listbox fires focus event in similar
> situation?
> 


Hi Surkov. It is awesome to have you attention on this bug!

A lot of assistive technology look for focus events because (as you know) it is very important to report to the end user what has just received focus. In the case of Thunderbird, if a user has focus on a message header (from the email header list) and presses delete, the focus is moved to the next message. This needs to be reported to the user and unfortunately is not (at least on linux).

See comment #29 for a possible solution (I'm embarassed I haven't gotten around to try this).

I had also thought perhaps the tree cache was out of sync but I haven't examined the implementation lately.

Surkov please feel free to take this bug if you are going to post a solution :)

Comment 44

[alexander :surkov (:asurkov)]

(In reply to comment #43)

> A lot of assistive technology look for focus events because (as you know) it is
> very important to report to the end user what has just received focus. In the
> case of Thunderbird, if a user has focus on a message header (from the email
> header list) and presses delete, the focus is moved to the next message. This
> needs to be reported to the user and unfortunately is not (at least on linux).

So who is focusable: tree, treeitem or treecell? Who should get focus if there is not treeitems in tree after last treeitem was removed?

Comment 45

[Evan Yan]

(In reply to comment #44)
> So who is focusable: tree, treeitem or treecell? Who should get focus if there
> is not treeitems in tree after last treeitem was removed?
> 
When there is selected treeitem, we alway fire focus event against the selected treeitem instead of tree itself. 
treeitem and treecell actually the same thing I think.

Comment 46

[alexander :surkov (:asurkov)]

Btw, I see focus event when treeitem is deleted. At least, in cross platform code. Evan, can you check this on linux?

Comment 47

[Evan Yan]

Surkov, yes, we have fired focus event in cross platform code. but it was prevented by atk_focus_tracker_notify(). see comment #9

Comment 48

[alexander :surkov (:asurkov)]

(In reply to comment #47)
> Surkov, yes, we have fired focus event in cross platform code. but it was
> prevented by atk_focus_tracker_notify(). see comment #9
> 

It seems I get it. It doesn't fire focus event because we do not invalidate accessible cache for tree?

Comment 49

[alexander :surkov (:asurkov)]

Olli, where is better place to keep idl/cpp/h files for new event (comment #42)?
1) idl - layout/src/xul/src/tree/public
   cpp/h - layout/xul/src/tree/src

2) idl - ?
   cpp/h - content/events/src

Comment 50

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

(In reply to comment #49)

Certainly not under layout. Either in content/events/ (if the event type is
somewhat generic or standardized somewhere, or possibly somewhere under accessible/ )

Comment 51

[Evan Yan]

(In reply to comment #48)
> It seems I get it. It doesn't fire focus event because we do not invalidate
> accessible cache for tree?
> 
Yes. On Linux, after deleting a row, we fire focus event against the same AtkObject, then it was prevented by atk.

Comment 52

[alexander :surkov (:asurkov)]

Attachment: patch in progress

Olli, is patch's approach correct?

Comment 53

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

I don't really like to add accessibility-only-event-type (nsDOMTreeRowChangedEvent) to content/events. Maybe some more generic
eventtype? nsIDOMDataContainerEvent or something which could then have
.data field which could be QIed to something... Or put nsDOMTreeRowChangedEvent
class somewhere under accessible/
You probably want to add something to nsDOMClassInfo.cpp, at least if you 
create the generic event type.

Btw, doesn't make sense to have nsIDOMTreeEvent.idl where you define 
nsIDOMTreeRowChangedEvent

Otherwise the approach looks reasonable.

Comment 54

[alexander :surkov (:asurkov)]

Attachment: patch in progress

Olli, can you overlook the patch?

Comment 55

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

Comment on attachment 279741 [details] [diff] [review]
patch in progress

>   if (eventType.EqualsLiteral("TreeViewChanged")) { // Always asynch, always from user input
>-    NS_ENSURE_TRUE(localName.EqualsLiteral("tree"), NS_OK);
>+    NS_ASSERTION(localName.EqualsLiteral("tree"),
>+                 "TreeViewChanged event is fired for not tree element");
>+

Does accessibility handle only trusted events? If not, then content pages can easily fire
any events on any DOM node, and IMO NS_WARNING might be better than NS_ASSERTION.
Same thing also elsewhere.

>+  nsCOMPtr<nsIDOMEvent> domEvent(mEvent);
>+  if (!event) {
Should this be !domEvent

DataEvent seems quite useful event type, something similar could have been used to implement
few other event types we have now.
What I'm a bit worried is how to handle cases when trusted event is dispatched on content nodes and
then some content event listener changes the data before possible chrome event listeners read the values.
Especially what happens if variant is actually some object (nsISupports). What at least should be done
is to document, that to safely read values, chrome should use capture phase event listeners.
But maybe setting/changing data should be disabled while event is being dispatched or perhaps
data setting should be allowed only between initEvent and .dispatch calls and otherwise all data should
be read only. Or would there be some better API, something where data is set as a parameter to initDataEvent...not sure.

Btw, you may have to hook up dataevent to cycle collector, so that for example event.setData("foo", event) doesn't leak.

Comment 56

[alexander :surkov (:asurkov)]

(In reply to comment #55)
> (From update of attachment 279741 [details] [diff] [review])
> >   if (eventType.EqualsLiteral("TreeViewChanged")) { // Always asynch, always from user input
> >-    NS_ENSURE_TRUE(localName.EqualsLiteral("tree"), NS_OK);
> >+    NS_ASSERTION(localName.EqualsLiteral("tree"),
> >+                 "TreeViewChanged event is fired for not tree element");
> >+
> 
> Does accessibility handle only trusted events? If not, then content pages can
> easily fire
> any events on any DOM node, and IMO NS_WARNING might be better than
> NS_ASSERTION.
> Same thing also elsewhere.

You're right. Anyone can fire this event. Therefore I guess it's better not to take into account it at all, no warning, no assertion.
> 
> DataEvent seems quite useful event type, something similar could have been used
> to implement
> few other event types we have now.
> What I'm a bit worried is how to handle cases when trusted event is dispatched
> on content nodes and
> then some content event listener changes the data before possible chrome event
> listeners read the values.
> Especially what happens if variant is actually some object (nsISupports). What
> at least should be done
> is to document, that to safely read values, chrome should use capture phase
> event listeners.
> But maybe setting/changing data should be disabled while event is being
> dispatched or perhaps
> data setting should be allowed only between initEvent and .dispatch calls and
> otherwise all data should
> be read only. Or would there be some better API, something where data is set as
> a parameter to initDataEvent...not sure.

Looks reasonable to deny a data changing duiring event dispatching. Though state of nsISupports objects can be modified in any way. But probably it's ok.

> Btw, you may have to hook up dataevent to cycle collector, so that for example
> event.setData("foo", event) doesn't leak.
> 

Can you be more detailed please? I'm not familar with this.

Comment 57

[alexander :surkov (:asurkov)]

Attachment: not tested patch

Comment 58

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 59

[David Bolter [:davidb] (NeedInfo me for attention)]

Surkov, has your latest patch been tested?

Comment 60

[alexander :surkov (:asurkov)]

(In reply to comment #59)
> Surkov, has your latest patch been tested?
> 

I debugged it only. Probably cache invalidate error can exists and we don't fire focus event still. Please make sure patch fixes problem because now I have not linux to check this.

Comment 61

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

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


>+// nsIAccessible nsIAccessibleTreeCache::
>+//    GetCachedTreeitemAccessible(in long aRow, nsITreeColumn* aColumn)

Didn't really review accessible/, but the indentation here looks strange.
And not so sure how useful the comment is.

>+// void nsIAccessibleTreeCache::
>+//    invalidateCache(in PRInt32 aRow, in PRInt32 aCount)

Same here.

>+#ifdef MOZ_ACCESSIBILITY_ATK
>+    PRInt32 colsCount = 0;
>+    cols->GetCount(&colsCount);
>+    for (PRInt32 j = 0; j < colsCount; j++) {
>+      mAccessNodeCache->Remove((void*)(i * kMaxTreeColumns + j));

Use C++ casting.

>+    }
>+#else
>+    nsCOMPtr<nsITreeColumn> col;
>+    cols->GetKeyColumn(getter_AddRefs(col));
>+    PRInt32 colIndex = 0;
>+    col->GetIndex(&colIndex);
>+    mAccessNodeCache->Remove((void*)(i * kMaxTreeColumns + colIndex));
Use C++ casting.

>+++ content/events/public/nsPLDOMEvent.h	11 Sep 2007 04:59:26 -0000
...

>   nsCOMPtr<nsIDOMNode> mEventNode;
>+
>+  nsCOMPtr<nsIDOMEvent> mEvent;
>   nsString mEventType;

Nit, no need for that extra newline before mEvent.
Could you align all the members

>+NS_INTERFACE_MAP_BEGIN(nsDOMDataEvent)
>+  NS_INTERFACE_MAP_ENTRY(nsIDOMDataEvent)
>+  NS_INTERFACE_MAP_ENTRY_CONTENT_CLASSINFO(DataEvent)
>+NS_INTERFACE_MAP_END_INHERITING(nsDOMEvent)
>+
>+NS_IMPL_ADDREF_INHERITED(nsDOMDataEvent, nsDOMEvent)
>+NS_IMPL_RELEASE_INHERITED(nsDOMDataEvent, nsDOMEvent)

Make nsDOMDataEvent to be a cycle collector participant, so
that unlink/traverse goes through the entries in the mData. Otherwise
leaks are possible. nsDOMEvent does already traverse/unlink .target, .originalTarget etc. There are some event type specific things in 
nsDOMEvent's Unlink/Traverse, but I think it is better to handle
nsDOMDataEvent.mData in nsDOMDataEvent.cpp

>+NS_IMETHODIMP
>+nsDOMDataEvent::SetData(const nsAString& aKey, nsIVariant *aData)
>+{
>+  NS_ENSURE_ARG(aData);
>+
>+  // Make sure this event isn't already being dispatched.
>+  NS_ENSURE_TRUE(!NS_IS_EVENT_IN_DISPATCH(mEvent), NS_ERROR_INVALID_ARG);

I think I'd prefer
NS_ENSURE_STATE(!((NS_IS_EVENT_IN_DISPATCH(mEvent) ||
                 (mEvent->flags & NS_EVENT_FLAG_STOP_DISPATCH_IMMEDIATELY)));

>+
>+  mData.Put(aKey, aData);

Before using .Put or .Get you need to make sure that mData is properly initialized. (It may not be if OOM happened when initializing it.)

>+  /**
>+   * Set the data for the given key.

Add some comment here that data must be set after initializing the event
but before dispatching it.

Comment 62

[Scott Haeger]

I have tested with Accerciser and I am getting both a focus: and object:state-changed:focus event on the newly highlighted row (actually a table cell) when a message is deleted.

Comment 63

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Denying based on Olli's comments. Have confirmed with Scott that this is a viable solution. I think with the changes Olli suggest we should be looking good.

Comment 64

[Scott Haeger]

Should I be getting some sort of remove event when deleting a message.  I would expect an object:children-changed:remove event.

Comment 65

[Marco Zehe (:MarcoZ)]

In addition, using Orca, I found that when deleting a message:
* Orca would not speak the newly highlighted message automatically.
* The more messages I delete, the more bottom-most messages would not be announced when arrowing onto them. So if you initially have a folder with 10 messages, then delete messages 5 and 6, the new messages 7 and 8 will not be announced, but the new message positions 5 and 6 will be announced correctly with the newly moved-up messages that were previously messages 7 and 8.

Comment 66

[alexander :surkov (:asurkov)]

(In reply to comment #64)
> Should I be getting some sort of remove event when deleting a message.  I would
> expect an object:children-changed:remove event.
> 

There is one feature. If we would fire delete/add events then we will fire them for every collapse/extend actions too. It's specificity of xul trees.

Comment 67

[alexander :surkov (:asurkov)]

(In reply to comment #65)
> In addition, using Orca, I found that when deleting a message:
> * Orca would not speak the newly highlighted message automatically.
> * The more messages I delete, the more bottom-most messages would not be
> announced when arrowing onto them. So if you initially have a folder with 10
> messages, then delete messages 5 and 6, the new messages 7 and 8 will not be
> announced, but the new message positions 5 and 6 will be announced correctly
> with the newly moved-up messages that were previously messages 7 and 8.
> 

I suppose it's because Orca cached our accessible objects that becomes out of date because we do not fire add/remove events.

Comment 68

[alexander :surkov (:asurkov)]

Marco, when you do not remove messages but collapse them then does orca read them successfully?

Comment 69

[Marco Zehe (:MarcoZ)]

(In reply to comment #68)
> Marco, when you do not remove messages but collapse them then does orca read
> them successfully?

Yes, and this morning, I also can delete messages and still have the full accurate list of messages read when arrowing. The glitch I was observing yesterday consistently seems to have to do with an intermittent instability I see in Thunderbird under Linux, which always results in a crash of Thunderbird at shutdown. This morning, I have not seen this crash when exiting Thunderbird, and didn't see any glitches beforehand.
So until this general intermittent instability is resolved, let's just say that Orca does not automatically speak the newly focused item, which is an Orca bug,not a Thunderbird one once this is checked in.

Comment 70

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

(In reply to comment #56)
> 
> Can you be more detailed please? I'm not familar with this.
> 

You may want to look at how nsIDOM3Node::get/setUserData is implemented and
how that data is traversed and unlinked in nsNodeUtils. That code uses propertytable, but handling nsInterfaceHashtable<nsStringHashKey, nsIVariant>
is probably even easier.

Comment 71

[Evan Yan]

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

>+    for (PRInt32 j = 0; j < colsCount; j++) {
>+      mAccessNodeCache->Remove((void*)(i * kMaxTreeColumns + j));
>+    }

The accessibles of deleted row are removed, but accessibles in lower rows are not shifted up. I think that caused the problem in comment #65.

Comment 72

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 73

[alexander :surkov (:asurkov)]

(In reply to comment #71)
> (From update of attachment 280430 [details] [diff] [review])
> >+    for (PRInt32 j = 0; j < colsCount; j++) {
> >+      mAccessNodeCache->Remove((void*)(i * kMaxTreeColumns + j));
> >+    }
> 
> The accessibles of deleted row are removed, but accessibles in lower rows are
> not shifted up. I think that caused the problem in comment #65.
> 

I remove lower rows too until row count.

Comment 74

[alexander :surkov (:asurkov)]

(In reply to comment #61)
> (From update of attachment 280430 [details] [diff] [review])
> 
> >+// nsIAccessible nsIAccessibleTreeCache::
> >+//    GetCachedTreeitemAccessible(in long aRow, nsITreeColumn* aColumn)
> 
> Didn't really review accessible/, but the indentation here looks strange.
> And not so sure how useful the comment is.

I remove excess space if you about it.

Weirdal suggested to write IDL declaration to see interface the method belongs to.

Comment 75

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

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

>+nsDOMDataEvent::
>+  nsDOMDataEvent(nsPresContext *aPresContext, nsEvent *aEvent) :
>+  nsDOMEvent(aPresContext, aEvent)
>+{
Maybe this way:
nsDOMDataEvent::nsDOMDataEvent(nsPresContext* aPresContext, nsEvent* aEvent)
  : nsDOMEvent(aPresContext, aEvent)

>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsDOMDataEvent, nsDOMEvent)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMARRAY(mData)
Well, this seems to do the right thing, but since mData isn't an nsCOMArray,
better to tmp->mData.Clear();

>+nsDOMDataEvent::GetData(const nsAString& aKey, nsIVariant **aData)
...
>+  mData.Get(aKey, aData);
You didn't add the .mData.IsInitialized() check here.

>+nsDOMDataEvent::SetData(const nsAString& aKey, nsIVariant *aData)
...
>+  NS_ENSURE_STATE(!(NS_IS_EVENT_IN_DISPATCH(mEvent) ||
>+                 (mEvent->flags & NS_EVENT_FLAG_STOP_DISPATCH_IMMEDIATELY)));
Align the expressions: add two spaces before '(mEvent->'

>+  mData.Put(aKey, aData);
You didn't add the .mData.IsInitialized() check here.

>+  /**
>+   * Set the data for the given key.
>+   *
>+   * @param  key   the data key
>+   * @param  data  the data
>+   * @return       NS_ERROR_UNEXPECTED if the method is called during event
>+   *               dispatch otherwise NS_OK
>+   */
>+  void setData(in DOMString key, in nsIVariant data);

That is a void method, so it doesn't return a value, but it does:
@throws NS_ERROR_UNEXPECTED if the method is called during event dispatch.

> NS_IMETHODIMP nsTreeBodyFrame::RowCountChanged(PRInt32 aIndex, PRInt32 aCount)
> {
>   if (aCount == 0 || !mView)
>     return NS_OK; // Nothing to do.
> 
>+  // Fire an event before tree items are removed.
>+  if (aCount < 0)
>+    FireRowCountChangedEvent(aIndex, aCount);
>+
...
>+
>+    // Fire an event after tree items are inserted.
>+    if (aCount > 0)
>+      FireRowCountChangedEvent(aIndex, aCount);
...
>@@ -1842,16 +1855,20 @@ NS_IMETHODIMP nsTreeBodyFrame::RowCountC
...
>  if (FullScrollbarsUpdate(needsInvalidation)) {
>     MarkDirtyIfSelect();
>   }
>+
>+  // Fire an event after tree items are inserted.
>+  if (aCount > 0)
>+    FireRowCountChangedEvent(aIndex, aCount);
>   return NS_OK;
> }
Why do we need three places for firing the event?
The event is anyway asynchronous, so firing it right after
   if (aCount == 0 || !mView)
     return NS_OK; // Nothing to do.
or
  if (mUpdateBatchNest)
    return NS_OK;
might be work, no?
And btw, doing anything after FullScrollbarsUpdate() must be
handled with great care. Read the comment about that method in
nsTreeBodyFrame.h

Almost r+, but I could still read the final patch.
I didn't read the accessible/ part of the patch properly.

Comment 76

[David Bolter [:davidb] (NeedInfo me for attention)]

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

I'm thinking it would be good to have one of the Sun guys look at the cache code . I'm volunteering Evan. I will keep an eye on this bug and tell you if I notice something.

Comment 77

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 78

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

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


>+  /**
>+   * Set the data for the given key.
>+   *
>+   * @param  key   the data key
>+   * @param  data  the data
>+   * @throws       NS_ERROR_UNEXPECTED if the method is called during event
>+   *               dispatch otherwise NS_OK

This doesn't "throw" NS_OK, so remove "otherwise NS_OK"

>@@ -1794,16 +1799,18 @@ NS_IMETHODIMP nsTreeBodyFrame::RowCountC
>   nsCOMPtr<nsITreeSelection> sel;
>   mView->GetSelection(getter_AddRefs(sel));
>   if (sel)
>     sel->AdjustSelection(aIndex, aCount);
> 
>   if (mUpdateBatchNest)
>     return NS_OK;
> 
>+  FireRowCountChangedEvent(aIndex, aCount);
>+
>     // Just update the scrollbar and return.
>     if (FullScrollbarsUpdate(PR_FALSE)) {
>       MarkDirtyIfSelect();
>     }
>+

No extra newlines, please.

>   if (FullScrollbarsUpdate(needsInvalidation)) {
>     MarkDirtyIfSelect();
>   }
>+

Same here.

r=me, but I want to see mochitests for DOMDataEvent, and wouldn't be bad to
have tests for <xul:tree> either, to make sure that events are really dispatched.
Not sure how to test memory leaks automatically, yet. IIRC that possibility is
coming soon.

I can't test whether this all works well with accessibility code, that testing
must be done by someone else.

Comment 79

[Evan Yan]

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

>+  /**
>+   * Invalidates the number of cached treeitem accessibles.
>+   *
>+   * @param aRow    the given row index
>+   * @param aCount  the number of treeitem accessibles to ivalidate
>+   */
it would be nice to have a description about aCount's negative and positive value

nit: misspell of invalidate.

>+  rowCount -= aCount;
>+
>+  for (PRInt32 i = aRow; i < rowCount; ++i) {

My concern here is performance.
Imagine that I have 1000 messages in my Inbox, then I deleted only the 5th message, then almost the whole cache will be invalidated.

Actually I think we can just 

   // invalidate current row to walk around atk_focus_tracker_notify
   for (PRInt32 i = 0; i < colsCount; i++) {
     mAccessNodeCache->Remove((void*)(aRow * kMaxTreeColumns + i));
   }
   // invalidate out of size cache.
   for (PRInt32 i = rowCount; i < rowCount - aCount; i++) {
      // invalidate
   }

Comment 80

[alexander :surkov (:asurkov)]

Attachment: patch4

Olli, can you look at tests?

Comment 81

[alexander :surkov (:asurkov)]

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

Evan, I fixed your comments. Can you check focus event is fired with changes :)?

Comment 82

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

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

>+++ content/events/test/test_bug368835.html	14 Sep 2007 17:20:57 -0000
...
>+      function dataEventHandler(aEvent)
>+      {
>+        var value = "";
>+        var isPassed = true;
>+        try {
>+          value = aEvent.getData("data1");
>+          isPassed = true;
>+        } catch (e) {
>+          alert(e);
Remove alert()


>+        ok(isPassed, "getData shoudn't fail.");
shouldn't 

>+
>+        ok(isPassed, "setData should fail duiring event dispatching.");
during

>+    function CheckEvents()
>+    {
>+      ok(gTreeViewChanged, "TreeViewChanged event should be fired.")

should have been fired.

>+      ok(gTreeRowCountChanged, "TreeRowCountChanged event should be fired.");

should have been fired.

Comment 83

[Evan Yan]

Thunderbird crash at start up after applying this patch.

 fef2e8d7 _lwp_kill (1, b) + 7
 feee6a52 raise    (b) + 22
 fe824d26 void nsProfileLock::FatalSignalHandler(int) (b, 0, 80461a0) + 166
 fef2dc8f __sighndlr (b, 0, 80461a0, fe824bc0) + f
 fef22bdb call_user_handler (b, 0, 80461a0) + 2b8
 fef22d82 sigacthandler (b, 0, 80461a0) + c2
 --- called from signal handler with signal 11 (SIGSEGV) ---
 f7b1bab8 nsCOMPtr<nsIDOMEvent>::nsCOMPtr #Nvariant 1(const nsCOMPtr<nsIDOMEvent>&) (8046500, 8a69d58) + 58
 f7b1b02c unsigned nsPLDOMEvent::Run() (8a69d48) + 6c
 feb96381 unsigned nsThread::ProcessNextEvent(int,int*) (80b1f08, 1, 8046610) + 321
 feae1e7f int NS_ProcessNextEvent_P(nsIThread*,int) (80b1f08, 1) + 7f
 f93d01b6 unsigned nsBaseAppShell::Run() (816a0e8) + 86
 f91f2d4a unsigned nsAppStartup::Run() (81b74b8) + 9a
 fe80e4df XRE_main (1, 8046ecc, 80659c8) + 464f
 08051d4d main     (1, 8046ecc, 8046ed4) + 17d
 08051ada _start   (1, 8047060, 0, 8047080, 80470c4, 80470df) + 7a

Comment 84

[alexander :surkov (:asurkov)]

Attachment: patch5

patch is updated to trunk

Evan, I run thunderbird on windows, it works fine. Did you do a full rebuild of your thunderbird?

Comment 85

[alexander :surkov (:asurkov)]

Attachment: patch6

Comment 86

[Evan Yan]

we're very close.

if I delete one single row, focus event is fired (great!), but if select multiple rows then delete them, no focus event. That's because |currentRow| is the last selected row before deleting, which not surely be the focused row after deleting. We need to invalidate all the deleted rows.

Comment 87

[alexander :surkov (:asurkov)]

Attachment: patch7

another approach: pass previous selection in event and remove those accessibles that was selected.

Comment 88

[alexander :surkov (:asurkov)]

Attachment: patch8

Comment 89

[alexander :surkov (:asurkov)]

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

new patch is coming

Comment 90

[alexander :surkov (:asurkov)]

Attachment: patch9

Comment 91

[Evan Yan]

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

It doesn't compile.

>+  nsresult rv = cols->GetCount(&colsCount);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRInt32 newRowCount = 0;
>+  nsresult rv = mTreeView->GetRowCount(&newRowCount);

redefine of nsresult.

>+  if (currIdxVariant) {
>+    PRInt32 currentIndex = -1;
>+    rv = indexVariant->GetAsInt32(&currentIndex);

s/indexVariant/currIdxVariant

>+    if (currentIndex != -1) {
>+      for (PRInt32 j = 0; j < colsCount; j++) {
>+        void *key = reinterpret_cast<void*>
>+          (currentIndex * kMaxTreeColumns + j);

kMaxTreeColumns is out of scope, need to put it into nsXULTreeAccessible.h

>+  rv = selRangeStartVariant->GetAsArray(nsIDataType::VTYPE_INT32,
>+                                        nsnull, &selRangeCount,
>+                                        startVoidArray);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = selRangeEndVariant->GetAsArray(nsIDataType::VTYPE_INT32,
>+                                      nsnull, &selRangeCount,
>+                                      endVoidArray);

incorrect call to GetAsArray.
the first argument is a out parameter, should be a point;
the third argument needs PRUint32*;
the forth argument needs void**, which should point to a pre-allocated array.

>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  NS_ENSURE_STATE(startVoidArray && endVoidArray);
>+
>+  PRInt32 *startArray = static_cast<PRInt32*>startVoidArray;
>+  PRInt32 *endArray = static_cast<PRInt32*>endVoidArray;
>+
static_cat<PRInt32*> needs following "()"

And do we really needs the |selection| stuffs? I thought about adding the below code based on your patch6 should fix the problem.

  for (PRInt32 i = aRow; i < aRow - aCount; i++) {
    for (PRInt32 j = 0; j < colsCount; j++) {
      void *key = reinterpret_cast<void*>(i * kMaxTreeColumns + j);
      mAccessNodeCache->Remove(key);
    }
  }

Then I debugged it to test the idea and got some new information.

When selecting multiple rows then delete, RowCountChanged event is not fired. That's because in layout code, multiple rows are actually deleted one by one, and in nsTreeBodyFrame::RowCountChanged() |mUpdateBatchNest| is true.

  if (mUpdateBatchNest)
    return NS_OK;

Then I moved FireRowCountChangedEvent() to the front of the above code. After that, xul tree cache can be invalidated successfully.

However, when deleting multiple rows, there is no DOMMenuItemActive event received from layout after deleting finished. So still no focus event fired after deleting multiple rows.
I haven't figured out why DOMMenuItemActive event is missing.

Comment 92

[alexander :surkov (:asurkov)]

> When selecting multiple rows then delete, RowCountChanged event is not fired.
> That's because in layout code, multiple rows are actually deleted one by one,
> and in nsTreeBodyFrame::RowCountChanged() |mUpdateBatchNest| is true.
> 
>   if (mUpdateBatchNest)
>     return NS_OK;
>
> Then I moved FireRowCountChangedEvent() to the front of the above code. After
> that, xul tree cache can be invalidated successfully.

Evan, I have a feel you tested wrong patch. And you pointed some errors of previous patch too.

Comment 93

[alexander :surkov (:asurkov)]

> However, when deleting multiple rows, there is no DOMMenuItemActive event
> received from layout after deleting finished. So still no focus event fired
> after deleting multiple rows.
> I haven't figured out why DOMMenuItemActive event is missing.

Possibly save this for another patch?

Comment 94

[alexander :surkov (:asurkov)]

Attachment: patch10

Evan, I tried to fix your comments exceptin DOMMenuActivate event.

Comment 95

[alexander :surkov (:asurkov)]

> And do we really needs the |selection| stuffs? I thought about adding the below
> code based on your patch6 should fix the problem.
> 
>   for (PRInt32 i = aRow; i < aRow - aCount; i++) {
>     for (PRInt32 j = 0; j < colsCount; j++) {
>       void *key = reinterpret_cast<void*>(i * kMaxTreeColumns + j);
>       mAccessNodeCache->Remove(key);
>     }
>   }

There is one thing stoping from this. We get rowCountChanged event on every expand/collapse action. I think these actions are more often than dealing with selection, therefore it should be a bit quickly.

Comment 96

[alexander :surkov (:asurkov)]

Attachment: patch11

Comment 97

[Evan Yan]

(In reply to comment #92)
> Evan, I have a feel you tested wrong patch. And you pointed some errors of
> previous patch too.
> 
My testing is based on your patch6 with adding invalidating from aRow to (aRow - aCount). Sorry I didn't notice you've corrected the invoke place of FireRowCountChangedEvent() in later patch.

Comment 98

[Evan Yan]

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

>+  PRUint16 type;
>+  nsIID iid;
>+  void *startVoidArray = nsnull, *endVoidArray = nsnull;
>+  rv = selRangeStartVariant->GetAsArray(&type, &iid, &selRangeCount,
>+                                        &startVoidArray);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = selRangeEndVariant->GetAsArray(&type, &iid, &selRangeCount,
>+                                      &endVoidArray);

startVoidArray and endVoidArray need to allocate memory in advance.
And please check whether we can replace |type| and |iid| with nsnull since we don't care their value.

>+  PRInt32 *startArray = static_cast<PRInt32*>startVoidArray;
>+  PRInt32 *endArray = static_cast<PRInt32*>endVoidArray;

"()" is needed, i.e. static_cast<PRInt32*>(startVoidArray);

I'll test the patch tomorrow when I at office.

Comment 99

[alexander :surkov (:asurkov)]

(In reply to comment #98)

> startVoidArray and endVoidArray need to allocate memory in advance.

Why? I think nsVariant should care about this because we can't know array size before we call getAsArray method (an example of usage: http://lxr.mozilla.org/mozilla/source/extensions/webservices/soap/src/nsDefaultSOAPEncoder.cpp#1178_

> And please check whether we can replace |type| and |iid| with nsnull since we
> don't care their value.

Unfortunately we have not.

> 
> >+  PRInt32 *startArray = static_cast<PRInt32*>startVoidArray;
> >+  PRInt32 *endArray = static_cast<PRInt32*>endVoidArray;
> 
> "()" is needed, i.e. static_cast<PRInt32*>(startVoidArray);
> 
> I'll test the patch tomorrow when I at office.
> 

Evan, do you need new patch with () fixed?

Comment 100

[Evan Yan]

(In reply to comment #99)
> Why? I think nsVariant should care about this because we can't know array size
> before we call getAsArray method (an example of usage:
> http://lxr.mozilla.org/mozilla/source/extensions/webservices/soap/src/nsDefaultSOAPEncoder.cpp#1178_
> 
ok, you're right.

> Evan, do you need new patch with () fixed?
> 
no, I don't.

Comment 101

[Evan Yan]

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

>+  rv = selRangeStartVariant->GetAsArray(&type, &iid, &selRangeCount,
>+                                        &startVoidArray);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = selRangeEndVariant->GetAsArray(&type, &iid, &selRangeCount,
>+                                      &endVoidArray);
the third argument should be (unsigned *).

with above and static_cast thing fixed, r=me.

Comment 102

[Evan Yan]

Thanks for fixing this bug. It's really a long lasted and high severity one.

Comment 103

[alexander :surkov (:asurkov)]

Attachment: patch12

Olli, can you look again at the patch because I changed code for treeRowCountChanged event? Btw, do you see a better way to test async treeViewChanged and treeRowCountChanged events than via setTimeout()?

Comment 104

[alexander :surkov (:asurkov)]

(In reply to comment #101)

> the third argument should be (unsigned *).

I fixed this by introducing stub PRInt32 variable :) I'll fix this a later.

Comment 105

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

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

>+   * @throws       NS_ERROR_UNEXPECTED if the method is called during event
>+   *               dispatch otherwise NS_OK

You have still "otherwise NS_OK". Remove it.

>+    if (len) {
>+      PRInt32 *rangeStartArray = static_cast<PRInt32*>(
>+        nsMemory::Alloc(len * sizeof(PRInt32)));
>+      if (!c)
>+        return;
>+
>+      PRInt32 *rangeEndArray = static_cast<PRInt32*>(
>+        nsMemory::Alloc(len * sizeof(PRInt32)));
>+      if (!rangeEndArray)
>+        return;

Here you leak rangeStartArray.

>+
>+      for (PRInt32 selIdx = 0, idx = 0; selIdx < selCount; ++selIdx) {
>+        PRInt32 min = 0, max = 0;
>+        rv = sel->GetRangeAt(selIdx, &min, &max);
>+        if (NS_FAILED(rv))
>+          return;

Here you leak both rangeStartArray and rangeEndArray.

>+
>+      // 'selRangeStartBounds' data - array to store min row indexes of selected
>+      // rows ranges.
>+      nsCOMPtr<nsIWritableVariant> selRangeStartVariant(
>+        do_CreateInstance("@mozilla.org/variant;1"));
>+      if (!selRangeStartVariant)
>+        return;

As well here.

>+      selRangeStartVariant->SetAsArray(nsIDataType::VTYPE_INT32, nsnull,
>+                                       len, rangeStartArray);
...
>+      selRangeEndVariant->SetAsArray(nsIDataType::VTYPE_INT32, nsnull,
>+                                     len, rangeEndArray);

Who owns the arrays? If I read the code correctly variant makes a copy of the
arrays, so you should delete the arrays somewhere, otherwise you leak.

>+    function CheckEvents()

You could call CheckEvents right after the events have fired and cancel the timeout. That would make the test to run faster. And in that case you could
use a longer timeout, maybe 1000ms.

Comment 106

[alexander :surkov (:asurkov)]

Attachment: patch13

Olli's comments

Comment 107

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

Sorry, I must have missed the review request. I'll try to read the patch tomorrow.

Comment 108

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

We're firing "TreeRowCountChanged" events even if accessibility isn't compiled
or if it is not enabled. How often does the event get dispatched and does it
affect performance significantly?
(Same question to all accessibility related events, actually.)

Comment 109

[alexander :surkov (:asurkov)]

(In reply to comment #108)
> We're firing "TreeRowCountChanged" events even if accessibility isn't compiled
> or if it is not enabled. How often does the event get dispatched and does it
> affect performance significantly?
> (Same question to all accessibility related events, actually.)
> 

Afaik trees are used in firefox/thunderbird so that treeRowChanged is called either on user input or when couple of mails is received. These can't make a performance problem. Therefore I guess there wasn't raised similar question in bug 246236 where 'TreeViewChanged" has been introduced.

I can include an event sending in ifdef/endif if you like. And sure I can do some tests to show in percents how slower we started to be.

Comment 110

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

(In reply to comment #109)
> Afaik trees are used in firefox/thunderbird so that treeRowChanged is called
> either on user input or when couple of mails is received. These can't make a
> performance problem.
"couple". Is the event dispatched for each new message. What if I 
subscribe to a new newsgroup which has thousands of messages?
Could you please test how often event gets dispatched.

> Therefore I guess there wasn't raised similar question in
> bug 246236 where 'TreeViewChanged" has been introduced.
> 
TreeView isn't changed nearly as often as rowcount.

Comment 111

[alexander :surkov (:asurkov)]

Attachment: perf test

it calls rowCountChanged 1000 times, average performance regression is 70ms, before: 450, after: 520.

When you import news then they do not call rowCountChanged for every news item.

Comment 112

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

(In reply to comment #111)
> it calls rowCountChanged 1000 times, average performance regression is 70ms,
> before: 450, after: 520.
That is quite huge perf regression, although the test is artificial. But I don't 
want to slow down some extensions which may do something strange.

Could you perhaps batch the changes? The event is anyway asynchronous.
So while there is an event to be dispatched, and row count changes, you just add
some new data to the event. Or does that get too complex?

Comment 113

[alexander :surkov (:asurkov)]

(In reply to comment #112)
> (In reply to comment #111)
> > it calls rowCountChanged 1000 times, average performance regression is 70ms,
> > before: 450, after: 520.
> That is quite huge perf regression, although the test is artificial. But I
> don't 
> want to slow down some extensions which may do something strange.
> 
> Could you perhaps batch the changes? The event is anyway asynchronous.
> So while there is an event to be dispatched, and row count changes, you just
> add
> some new data to the event. Or does that get too complex?
> 

I'm not sure we'll win some performance because then I need trick algorithm to merge event data if I got you right.

Comment 114

[alexander :surkov (:asurkov)]

Olli, what's in the end?

Comment 115

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

The pref regression would be bad. Are there any good ways to solve that?
Is it slow if accessible/ code recreates the data structure referring to
xultree if there are multiple row counts changed. Or something like that

Comment 116

[alexander :surkov (:asurkov)]

(In reply to comment #115)
> The pref regression would be bad. Are there any good ways to solve that?
> Is it slow if accessible/ code recreates the data structure referring to
> xultree if there are multiple row counts changed. Or something like that
> 

Do you mean will I win if I would create event object and variants at once and then reuse them? But variants count depends on current tree state, for example, I do not create variants if there is no selection and it means I should remove them. I'm not sure creating/removing should be faster.

Comment 117

[alexander :surkov (:asurkov)]

Olli, will it be fine if I won't fire events if the build is without ally support or if ally wasn't enabled by AT?

Comment 118

[Aaron Leventhal]

Olli, I agree with Surkov, that sounds reasonable to me. What do you think?

> Olli, will it be fine if I won't fire events if the build is without ally
> support or if ally wasn't enabled by AT?

Basically it means that if no AT is being used then there won't be any perf difference.

Comment 119

[Joanmarie Diggs]

Alexander, if it wouldn't be too much to ask, would you please post a new patch merged to trunk?  I tried to manually patch with today's trunk but still got build failures.

Thanks!!

Comment 120

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

If you do that, please make it dispatch events only when a11y is activated.
(Sorry, I'm still wondering if there was a better and cleaner way to fix the bug.
 But if no better solution is found, I can review the new patch.)

Comment 121

[alexander :surkov (:asurkov)]

Attachment: patch14

updated to trunk
new event is fired if ally is activated only

Comment 122

[alexander :surkov (:asurkov)]

Attachment: patch15

correct patch

Comment 123

[Marco Zehe (:MarcoZ)]

Surkov, when I try to compile Thunderbird with your latest patch using Ubuntu Gutsy, I get the following build errors:
/home/marco/mozilla/accessible/src/atk/nsXULTreeAccessibleWrap.cpp:532: error: no matching function for call to ‘nsDerivedSafe<nsIVariant>::GetAsArray(PRUint16*, nsIID*, PRInt32*, void**)’
../../../dist/include/xpcom/nsIVariant.h:224: note: candidates are: virtual nsresult nsIVariant::GetAsArray(PRUint16*, nsIID*, PRUint32*, void**) <near match>
/home/marco/mozilla/accessible/src/atk/nsXULTreeAccessibleWrap.cpp:535: error: no matching function for call to ‘nsDerivedSafe<nsIVariant>::GetAsArray(PRUint16*, nsIID*, PRInt32*, void**)’
../../../dist/include/xpcom/nsIVariant.h:224: note: candidates are: virtual nsresult nsIVariant::GetAsArray(PRUint16*, nsIID*, PRUint32*, void**) <near match>

Comment 124

[alexander :surkov (:asurkov)]

Attachment: patch16

with Marco's catch

Comment 125

[Marco Zehe (:MarcoZ)]

Surkov, I've been running with this patch for half an hour now, and am noticing one problem:
If I delete the top-most message, the accessible infor for the second message gets duplicated in the new first message. As a result, only the new top-most message is actually corresponding to the actually displayed message. The others are off by one now.
1. Before deleting the top most message:
subject A
subject B
subject C
subject D
2. Delete message titled "subject A". Result:
subject B
subject B
subject C
subject D

If I delete a message somewhere in the middle, or at the bottom, everything is OK. It is only if I delete the top-most message that things get out of sync.

Almost there!

Comment 126

[alexander :surkov (:asurkov)]

Marco, what software do you use? It soudns like your client doesn't update its cache because it shows 4 items after first item has been removed? Probably we should notify it somehow?

Comment 127

[Marco Zehe (:MarcoZ)]

(In reply to comment #126)
> Marco, what software do you use? It soudns like your client doesn't update its
> cache because it shows 4 items after first item has been removed? Probably we
> should notify it somehow?

I'm using Orca, and I don't think that's the problem. In every other case, meaning the 2nd or any other than the 1st item being removed, it updates correctly. So the mechanism is working, it just falls over somehow for the very first entry.

Comment 128

[Marco Zehe (:MarcoZ)]

OK, I repeated the test, and here's what happens:
1. Before delete:
subject A
subject B
subject C
subject D
2. After delete:
subject B
subject B
subject C

So, the number of items is correct, but the accessible info for subject B, which is now shifted upwards, is doubled, and the info for subject D is lost completely.

Comment 129

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

Alex, you're fixing the bugs in the patch, right?

Comment 130

[alexander :surkov (:asurkov)]

(In reply to comment #129)
> Alex, you're fixing the bugs in the patch, right?
> 

I'm trying to figure out what's going wrong but I still think you can start to review the patch because 1) the patch fixes what's the bug summary about, 2) I suppose possible changes might be in ally module only 3) we need to get this approved and our changes for this are going down I guess

Comment 131

[alexander :surkov (:asurkov)]

Marco, is your catch regression of the patch or not?

Comment 132

[Marco Zehe (:MarcoZ)]

(In reply to comment #131)
> Marco, is your catch regression of the patch or not?

Honestly, I can't say. Without the patch, the accessibles are not updated at all. To get an update, one has to switch away from the folder and back to it. If I do it with the patch, that also updates the accessible, then. But it definitely shows now that the events are getting fired.

Comment 133

[alexander :surkov (:asurkov)]

(In reply to comment #132)
> (In reply to comment #131)
> > Marco, is your catch regression of the patch or not?
> 
> Honestly, I can't say. Without the patch, the accessibles are not updated at
> all. To get an update, one has to switch away from the folder and back to it.
> If I do it with the patch, that also updates the accessible, then. But it
> definitely shows now that the events are getting fired.
> 

Ah, see. Can you test please patch 3, there shouldn't be such problem, right?

Comment 134

[Marco Zehe (:MarcoZ)]

(In reply to comment #133)
> Ah, see. Can you test please patch 3, there shouldn't be such problem, right?

That patch no longer applies cleanly and causes compilation errors I don't know how to fix. Sorry!

Comment 135

[alexander :surkov (:asurkov)]

Attachment: patch 17

fires hide events for cached tree items, Marco, can you looks, does it help?

Comment 136

[Marco Zehe (:MarcoZ)]

(In reply to comment #134)
> (In reply to comment #133)
> > Ah, see. Can you test please patch 3, there shouldn't be such problem, right?
> That patch no longer applies cleanly and causes compilation errors I don't know
> how to fix. Sorry!

OK, got it fixed and could compile with patch3. And that patch correctly updates the tree when deleting the first entry. So everything is still in sync then.

I will now try patch 17.

Comment 137

[Marco Zehe (:MarcoZ)]

(In reply to comment #135)
> fires hide events for cached tree items, Marco, can you looks, does it help?

Surkov, this one has a different problem:
1. If I delete the very first item, all the other items get updated completely.

2. However, if I delete any item x from the top but the first, the subject and author columns of the last x-1 items get the subject and author from the xth item from the bottom. Example:

You start with this:

s1 a1 t1
s2 a2 t2
s3 a3 t3
s4 a4 t4
s5 a5 t5
s6 a6 t6
s7 a7 t7
s8 a8 t8
s9 a9 t9
s10 a10 t10

Delete item 2. Result should be:

s1 a1 t1
s2 a2 t2
s3 a3 t3
s4 a4 t4
s5 a5 t5
s6 a6 t6
s7 a7 t7
s8 a8 t8
s9 a9 t9

Actual result is:

s1 a1 t1
s2 a2 t2
s3 a3 t3
s4 a4 t4
s5 a5 t5
s6 a6 t6
s7 a7 t7
s8 a8 t8
s8 a8 t9

or, delete item 3 from the original list. Actual result is:

s1 a1 t1
s2 a2 t2
s3 a3 t3
s4 a4 t4
s5 a5 t5
s6 a6 t6
s7 a7 t7
s7 a7 t8
s7 a7 t9

Is this cunderstandable?

Comment 138

[alexander :surkov (:asurkov)]

Really I haven't idea :). Marco, can you say what accessibles you get hide events for?

Comment 139

[Marco Zehe (:MarcoZ)]

(In reply to comment #138)
> Really I haven't idea :). Marco, can you say what accessibles you get hide
> events for?

Sorry, can't tell. Accerciser is currently in a non-useable state on Ubuntu Gutsy if Orca is also running, and since I am blind, I need Orca to know what I'm doing. So there is no way for me to record events right now I'm afraid.

Comment 140

[alexander :surkov (:asurkov)]

Comment on attachment 287098 [details] [diff] [review]
patch 17

Evan, can you look does the patch fixes original problem and what's going on with orca?

Comment 141

[Evan Yan]

Comment on attachment 287098 [details] [diff] [review]
patch 17

I didn't reproduce Marco's result.
Marco, which columns are you using? And which column is sorted?

There is focus event after I delete a message, but orca still didn't read the next message after deleting. I don't know why.

Another regression I found is after sorting by another column, the cache is in a mess. Orca reads wrong message other than what's focused.

Comment 142

[Marco Zehe (:MarcoZ)]

(In reply to comment #141)
> (From update of attachment 287098 [details] [diff] [review])
> I didn't reproduce Marco's result.
> Marco, which columns are you using? And which column is sorted?

It's both default (after a fresh profile is created or Thunderbird is freshly installed). Sorted by date oldest to newest, and Subject is forst column. The only thing I did was turn off the Preview Pane.

Comment 143

[Evan Yan]

I see another behavior. After deleting a message, including the first one, Orca always reads the former focused message. It can only be reproduced when Orca had ever read those message once.

For example, with Orca running, pressing DOWN to focus messages one by one to let Orca read them. Then delete a message, the next message get focused, but Orca reads nothing. Press DOWN again to move focus to a next message, the message which just lost focus get read by Orca. But in Accerciser I can see focus event is against the right message. If restart Orca, Orca will read the right message again.

(In reply to comment #141)
> Another regression I found is after sorting by another column, the cache is in
> a mess. Orca reads wrong message other than what's focused.
> 
And this is not regression. This issue still exists without the patch, only reproduced with Orca.

I'm not sure whether the above issues is Orca's bug or Firefox's bug.

Comment 144

[Evan Yan]

Comment on attachment 287098 [details] [diff] [review]
patch 17

canceling review other than minus until we figure out whether it's Orca's bug or firefox's bug.

Comment 145

[Marco Zehe (:MarcoZ)]

Evan, can you see in Accerciser if, after you delete a message, Thunderbird shows the right message info for what is visually displayed? If that's the case, and the visual info and what Accerciser show, match, then it's now an Orca bug. If there are discrepancies, it means that the patch still doesn't do the right thing.
This should be checked by someone sighted, because under Linux, even flat review in Orca depends on the accessible info, so I will never get an accurate visual display myself.

Comment 146

[Evan Yan]

(In reply to comment #145)
> Evan, can you see in Accerciser if, after you delete a message, Thunderbird
> shows the right message info for what is visually displayed?

Yes, Accerciser logged the correct focus event.

Comment 147

[Marco Zehe (:MarcoZ)]

(In reply to comment #146)
> (In reply to comment #145)
> > Evan, can you see in Accerciser if, after you delete a message, Thunderbird
> > shows the right message info for what is visually displayed?
> Yes, Accerciser logged the correct focus event.

Also, do all the column information of all the displayed messages match? That was the problem: Orca does notice on my system that one of the messages has gone away, but the bottom-most messages then get a screwed-up column information in the Subject and From columns.

Comment 148

[Evan Yan]

(In reply to comment #147)
> Also, do all the column information of all the displayed messages match? 
> 
The column information for a message are not screwed-up, they belong to a single message, but a wrong message (my comment #143)

Comment 149

[Marco Zehe (:MarcoZ)]

Now that Accerciser is fixed for me, I'll give this another look with Thunderbird Trunk with patch 17.

Comment 150

[Marco Zehe (:MarcoZ)]

(In reply to comment #143)
> I see another behavior. After deleting a message, including the first one, Orca
> always reads the former focused message. It can only be reproduced when Orca
> had ever read those message once.

Using patch 17 with latest Orca trunk, I still see Orca not speaking the newly focused message. But if I delete a message, then DownArrow, it reads the correct new focused message. This is Orca Trunk with all the pyatspi changes already in.

> > Another regression I found is after sorting by another column, the cache is in
> > a mess. Orca reads wrong message other than what's focused.
> And this is not regression. This issue still exists without the patch, only
> reproduced with Orca.

I no longer see this behaviour with Orca Trunk. If I go to View/Sort By, then select "From", the tree gets updated properly. Also when then changing back to Sort by Date, the tree gets updated too.

> I'm not sure whether the above issues is Orca's bug or Firefox's bug.
 
I think this is a bug in Orca before pyatspi migration.

Also, I no longer see the mismatch between columns when the xth message from the top has been deleted.

I hardly dare say it, but this patch looks good to me!!! Evan, do you want to review this patch?

Comment 151

[Evan Yan]

(In reply to comment #150)
> Evan, do you want to review this patch?
> 
I'll look at the patch on next Monday.

Surkov,
could you please outline the differences between patch17 and the former patch that I ever reviewed? It will help me a lot on reviewing the latest patch. Thanks!

Comment 152

[alexander :surkov (:asurkov)]

Attachment: patch18

Evan, I removed selection info from new event and started to fire events when tree item is removed. Actually you need to read carefully nsXULTreeAccessible::InvalidateCache only.

Comment 153

[Evan Yan]

Surkov, in comment #95, you justified to use selection, what's the reason you removed selection in your latest patch?

Comment 154

[alexander :surkov (:asurkov)]

(In reply to comment #153)
> Surkov, in comment #95, you justified to use selection, what's the reason you
> removed selection in your latest patch?
> 

Here I invalidate those treeitems that was removed only and send remove events for them. I do not remember now how we have gone to idea to operate with selection (probably it was orca specific or the fact usually selected items are removed). Now I was aimed by idea to repeat what we do for any other accessibles.

Comment 155

[Evan Yan]

In your comment #95, you said

"We get rowCountChanged event on every expand/collapse action. I think these actions are more often than dealing with selection, therefore it should be a bit quickly."

Is the situation changed?

Comment 156

[alexander :surkov (:asurkov)]

(In reply to comment #155)
> In your comment #95, you said
> 
> "We get rowCountChanged event on every expand/collapse action. I think these
> actions are more often than dealing with selection, therefore it should be a
> bit quickly."
> 
> Is the situation changed?
> 

No I think. Just selection won't help if I we try to make behaviour of treeitems similar with behaviour of another accessibles. Operations with selection was a try to make Orca work, I mean it was specific way, now it's more general approach. I'm not sure it should be really performance problem because I do not know an example where may of treeitems are expanded or collapsed.

Comment 157

[Evan Yan]

Comment on attachment 289120 [details] [diff] [review]
patch18

well, I think I'm ok with removal selection.

Comment 158

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

Comment on attachment 289120 [details] [diff] [review]
patch18

>+// void nsIAccessibleTreeCache::
>+//   invalidateCache(in PRInt32 aRow, in PRInt32 aCount)
>+NS_IMETHODIMP
>+nsXULTreeAccessible::InvalidateCache(PRInt32 aRow, PRInt32 aCount)
>+{
>+  // Do not invalidate the cache if rows have been inserted.
>+  if (aCount > 0)
>+    return NS_OK;
>+
>+  nsCOMPtr<nsITreeColumns> cols;
>+  nsresult rv = mTree->GetColumns(getter_AddRefs(cols));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+#ifdef MOZ_ACCESSIBILITY_ATK
>+  PRInt32 colsCount = 0;
>+  rv = cols->GetCount(&colsCount);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+#else
>+  nsCOMPtr<nsITreeColumn> col;
>+  rv = cols->GetKeyColumn(getter_AddRefs(col));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRInt32 colIdx = 0;
>+  rv = col->GetIndex(&colIdx);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+#endif
I'm not really review accessible/ part of the patch, but perhaps add some
comment here and also elsewhere why MOZ_ACCESSIBILITY_ATK is handled in a different way... And this part is tested with ATK and without, right?

>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsDOMDataEvent, nsDOMEvent)
>+  tmp->mData.Clear();
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>+
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsDOMDataEvent, nsDOMEvent)
>+  tmp->mData.EnumerateRead(TraverseEntry, &cb);
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END

I think before using tmp->mData  you need to check that mData is initialized.

>+nsDOMDataEvent::SetData(const nsAString& aKey, nsIVariant *aData)
...
>+  mData.Put(aKey, aData);
>+  return NS_OK;

Return NS_ERROR_OUT_OF_MEMORY is .Put fails.

So this patch may affect accessible performance (using for example the 'perf test'), but
if that is ok to accessible/ reviewers, then ok to me too.

r=me

Comment 159

[alexander :surkov (:asurkov)]

(In reply to comment #158)
> I'm not really review accessible/ part of the patch, but perhaps add some
> comment here and also elsewhere why MOZ_ACCESSIBILITY_ATK is handled in a
> different way...

Not sure it's really worth to do because there are similar things already where atk table implementation is handled in another way. It's known thing. Probably after fx3 we will get rid the difference if it won't affect on windows AT.

> And this part is tested with ATK and without, right?

Originally there wasn't a problem on windows side, though patch doesn't affect there.

Comment 160

[Aaron Leventhal]

What about Smaug's other comments?

Comment 161

[alexander :surkov (:asurkov)]

(In reply to comment #160)
> What about Smaug's other comments?
> 

I would happy to fix them with another comments, of course if Jonas is fine with this.

Comment 162

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

Comment on attachment 289120 [details] [diff] [review]
patch18

>Index: dom/public/nsDOMClassInfoID.h
>@@ -348,17 +348,20 @@ enum nsDOMClassInfoID {
> 
>   // Canvas
>   eDOMClassInfo_HTMLCanvasElement_id,
> #ifdef MOZ_ENABLE_CANVAS
>   eDOMClassInfo_CanvasRenderingContext2D_id,
>   eDOMClassInfo_CanvasGradient_id,
>   eDOMClassInfo_CanvasPattern_id,
> #endif
>-  
>+
>+  // Data Events
>+  eDOMClassInfo_DataEvent_id,

iirc you should add stuff to the end of nsDOMClassInfoID, not in the middle. This is because the enum values should be considered frozen.

>+nsTreeBodyFrame::FireRowCountChangedEvent(PRInt32 aIndex, PRInt32 aCount)
>+{
>+  nsCOMPtr<nsIContent> content(GetBaseElement());
>+  nsCOMPtr<nsIDOMNode> node(do_QueryInterface(content));
>+  if (!node)
>+    return;
>+
>+  nsCOMPtr<nsIDOMDocument> domDoc;
>+  node->GetOwnerDocument(getter_AddRefs(domDoc));
>+  nsCOMPtr<nsIDOMDocumentEvent> domEventDoc(do_QueryInterface(domDoc));
>+  if (!domEventDoc)
>+    return;

A better way to do this is to do:

nsIContent* content = GetBaseElement();
if (!content)  // not sure if you need this nullcheck?
  return;

nsCOMPtr<nsIDOMNode> node = do_QI(content);  // no need for nullcheck here
nsCOMPtr<nsIDOMDocumentEvent> domEventDoc(do_QueryInterface(content->GetOwnerDoc()));
if (!domEventDoc)
  return;

>+  nsRefPtr<nsPLDOMEvent> plevent = new nsPLDOMEvent(node, event);
>+  if (!plevent)
>+    return;
>+
>+  plevent->PostDOMEvent();

How does this compile? nsPLDOMEvent.h says that the second argument should be a string.


sr=me with those things fixed or explained.

Comment 163

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

Ah, i see, you added the ability to provide an event in this patch, cool.

Also, "DataEvent" is a bit generic name? I'm sort of thinking that we'd probably want to bring it up with w3c before we expose this to content.

Smaug, where else did you want to use an event like this?

Comment 164

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

ah, the event name is indeed perhaps a bit too generic.
DataContainerEvent?

That kind of event type could have been/be used in many places.
Right now we have all kinds of gecko specific events, which just
contain some additional getters for some data:
XULCommandEvent, nsDOMPopupBlockedEvent, nsDOMPageTransitionEvent...

Comment 165

[alexander :surkov (:asurkov)]

(In reply to comment #162)

> >+  nsRefPtr<nsPLDOMEvent> plevent = new nsPLDOMEvent(node, event);
> >+  if (!plevent)
> >+    return;
> >+
> >+  plevent->PostDOMEvent();
> 
> How does this compile? nsPLDOMEvent.h says that the second argument should be a
> string.
> 

I added constructor for this case: nsPLDOMEvent(nsIDOMNode *aEventNode, nsIDOMEvent *aEvent)

Comment 166

[alexander :surkov (:asurkov)]

(In reply to comment #158)

> >+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsDOMDataEvent, nsDOMEvent)
> >+  tmp->mData.Clear();
> >+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> >+
> >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsDOMDataEvent, nsDOMEvent)
> >+  tmp->mData.EnumerateRead(TraverseEntry, &cb);
> >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> 
> I think before using tmp->mData  you need to check that mData is initialized.
> 

Why? And how is it possible, if mData.Init() return false?

Comment 167

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

The object is still collectable even if mData.Init fails, so cyclecollector may 
unlink or traverse the object.

Comment 168

[alexander :surkov (:asurkov)]

(In reply to comment #162)

> A better way to do this is to do:
> 
> nsIContent* content = GetBaseElement();
> if (!content)  // not sure if you need this nullcheck?
>   return;

I think I need because they check this in the code.

Comment 169

[alexander :surkov (:asurkov)]

(In reply to comment #164)
> ah, the event name is indeed perhaps a bit too generic.
> DataContainerEvent?

Ok, should I change the name?

Comment 170

[alexander :surkov (:asurkov)]

(In reply to comment #163)
 
> Also, "DataEvent" is a bit generic name? I'm sort of thinking that we'd
> probably want to bring it up with w3c before we expose this to content.

Sorry what does "expose this to content" mean? Does it mean to do not use this event more or don't let this patch to go? Who can bring this up to w3c?

Comment 171

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

By content I mean webpages. I.e. are we firing this event such that normal webpages will see it? Are we expecting them to use it?

Comment 172

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

Better to change the name to be less generic, so that we don't have to bring it
up to w3c. That would slow things down (the patch for this bug has been in
development quite some time ).
Flash seem to have dataevent
http://livedocs.adobe.com/labs/air/1/aslr/flash/events/DataEvent.html

DataContainerEvent name is also used, but not in DOM context
http://arch.web.cern.ch/arch/JViews55/doc/refman/ilog/views/appframe/docview/project/DataContainerEvent.html

Comment 173

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

Yeah, i think changing the name is the way to go.

Smaug: you seemed to have plans for using this event in the future for other things. Do you have any suggestions for names that would work with that but still not be too generic?

Comment 174

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

I could say my plan is to try to prevent adding more simple event types.
DataContainerEvent is good enough name, I think.
DataEvent would be better, but because Flash uses it, and even in DOM context,
better not to use same name.

Comment 175

[alexander :surkov (:asurkov)]

(In reply to comment #171)
> By content I mean webpages. I.e. are we firing this event such that normal
> webpages will see it? Are we expecting them to use it?
> 

Yes, webpages see it. I used it to add tests. Should we hide it for webpages and how I can do it?

Comment 176

[alexander :surkov (:asurkov)]

(In reply to comment #174)
> I could say my plan is to try to prevent adding more simple event types.
> DataContainerEvent is good enough name, I think.
> DataEvent would be better, but because Flash uses it, and even in DOM context,
> better not to use same name.
> 

I'll put new patch with DataContainerEvent name. I hope we won't change event name too often, there is no fun to change the code :).

Comment 177

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

I don't know enough about the event to know if it's a good idea to expose to
content or not, I trust smaug to make the call there.

My point was, if we do expose this to content, we need to be more careful with
what name we use. First of all since we won't be able to change the name in the
future. Second, since we don't want to collide, name-wise, with other events
out there.

Comment 178

[alexander :surkov (:asurkov)]

Attachment: patch19

with Olli's, Jonas comments

Comment 179

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 291660 [details] [diff] [review]
patch19

a=drivers for when trunk opens after Fx3 Beta 2 freeze

Comment 180

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

Did this cause mochitest failures?

Comment 181

[alexander :surkov (:asurkov)]

checked in, I disabled layout/xul/test that is cause of enabled accessibility leading to mochitest failures. Need to file a bug to fix tests to work with enabled accessibility.

Comment 182

[Alex Vincent [:WeirdAl]]

I'd like to request MDC documentation of the DataContainerEvent.  It looks interesting.

Comment 183

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381