386161 - no focus event when arrowing down in xul tree table

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Evan Yan]

when arrowing down in xul tree table, like thunderbird message list, firefox bookmark manager, there is no focus event for the focused row/cell.

Comment 1

[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 when arrowing down in xul tree table, we're keep firing focus event for the same accessible, then these events are prevented by atk.

Comment 2

[Evan Yan]

Actually it's a regression which took place between the nightly builds of 2007-06-17 and 2007-06-18. I believe it's by bug 383482.

Comment 3

[Evan Yan]

Attachment: use finalFocusAccessible instead of finalFocusNode

I'm a little confused by "ToolkitEvent" and "AccessibleEvent".

FireDelayedToolkitEvent() just create a accEvent, then calls FireToolkitEvent(), isn't it?

And we have FireToolkitEvent() and FireAccessibleEvent(). I'm not clear about the difference of the meanings of "Toolkit" and "Accessible"

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> Created an attachment (id=270166) [details]
> use finalFocusAccessible instead of finalFocusNode
> 
> I'm a little confused by "ToolkitEvent" and "AccessibleEvent".

Toolkit event will go away eventually.

> FireDelayedToolkitEvent() just create a accEvent, then calls
> FireToolkitEvent(), isn't it?

yes, after timeout.

> 
> And we have FireToolkitEvent() and FireAccessibleEvent(). I'm not clear about
> the difference of the meanings of "Toolkit" and "Accessible"
> 

Later we will have FireAccessibleEvent() only. There are bugs for this.           

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> Created an attachment (id=270166) [details]
> use finalFocusAccessible instead of finalFocusNode

It's not right I guess. Either we shouldn't use delayed events here if it's possible or we should have the way to create delayed events for your case because delayed events shouldn't be initialized by accessible object since we have not guarantee that accessible object will be live after timeout.

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 270166 [details] [diff] [review]
use finalFocusAccessible instead of finalFocusNode

r- per comment #5

Comment 7

[Evan Yan]

(In reply to comment #5)
> It's not right I guess. Either we shouldn't use delayed events here if it's
> possible

please refer to bug 383482 and http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/accessible/src/base&command=DIFF_FRAMESET&file=nsRootAccessible.cpp&rev1=1.216&rev2=1.217&root=/cvsroot
that's where we change it from FireToolkitEvent() to FireDelayedToolkitEvent()

> or we should have the way to create delayed events for your case
> because delayed events shouldn't be initialized by accessible object since we
> have not guarantee that accessible object will be live after timeout.
> 
The timer is set in nsDocAccessible::FireDelayedAccessibleEvent(), but not nsDocAccessible::FireDelayedToolkitEvent().

1183 nsDocAccessible::FireDelayedAccessibleEvent(nsIAccessibleEvent *aEvent,
1184                                            PRBool aAllowDupes)
1185 {
1186   PRBool isTimerStarted = PR_TRUE;
1187   PRInt32 numQueuedEvents = mEventsToFire.Count();
1188   if (!mFireEventTimer) {
1189     // Do not yet have a timer going for firing another event.
1190     mFireEventTimer = do_CreateInstance("@mozilla.org/timer;1");
1191     NS_ENSURE_TRUE(mFireEventTimer, NS_ERROR_OUT_OF_MEMORY);
1192   }

So I think it's ok to call FireDelayedAccessibleEvent instead of FireDelayedToolkitEvent().

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> (In reply to comment #5)
> > It's not right I guess. Either we shouldn't use delayed events here if it's
> > possible
> 
> please refer to bug 383482 and
> http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/accessible/src/base&command=DIFF_FRAMESET&file=nsRootAccessible.cpp&rev1=1.216&rev2=1.217&root=/cvsroot
> that's where we change it from FireToolkitEvent() to FireDelayedToolkitEvent()
> 
> > or we should have the way to create delayed events for your case
> > because delayed events shouldn't be initialized by accessible object since we
> > have not guarantee that accessible object will be live after timeout.
> > 
> The timer is set in nsDocAccessible::FireDelayedAccessibleEvent(), but not
> nsDocAccessible::FireDelayedToolkitEvent().

No, FireDelayedToolkitEvent calls FireDelayedAccessibleEvent. Actually there is no difference between FireDelayedToolkitEvent and FireDelayedAccessibleEvent.

> 
> 1183 nsDocAccessible::FireDelayedAccessibleEvent(nsIAccessibleEvent *aEvent,
> 1184                                            PRBool aAllowDupes)
> 1185 {
> 1186   PRBool isTimerStarted = PR_TRUE;
> 1187   PRInt32 numQueuedEvents = mEventsToFire.Count();
> 1188   if (!mFireEventTimer) {
> 1189     // Do not yet have a timer going for firing another event.
> 1190     mFireEventTimer = do_CreateInstance("@mozilla.org/timer;1");
> 1191     NS_ENSURE_TRUE(mFireEventTimer, NS_ERROR_OUT_OF_MEMORY);
> 1192   }
> 
> So I think it's ok to call FireDelayedAccessibleEvent instead of
> FireDelayedToolkitEvent().
> 

Yes. But it shouldn't be initialized by accessible. We should have new constructor for events objects that will allow to create accessible in your case when it's not enough to have DOM node only.

Comment 9

[Evan Yan]

Attachment: return selected treeitem in nsAccEvent::GetAccessibleByNode()

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> Created an attachment (id=270588) [details]
> return selected treeitem in nsAccEvent::GetAccessibleByNode()
> 

It really looks like a hack. Are you sure we do not need delayed accessible events for xul:tree itself?

Most of accessible objects has unique rule how to get it. It's enough to have DOMNode. Can we generalize the rule for more complicated cases?

Comment 11

[Evan Yan]

(In reply to comment #10)
> It really looks like a hack. Are you sure we do not need delayed accessible
> events for xul:tree itself?
> 
I think when we get a DOM event, we always search for the selected treeitem accessible to replace tree accessible, like we do in nsRootAccessible::HandleEventWithTarget().

I agree it's a little hack, but I don't have a better idea.

Comment 12

[alexander :surkov (:asurkov)]

Ok, if you will file bug for better idea and get a link in your patch at this bug then it whould be helpful. Also please add a comment that if someone wants to fire delayed events for the tree then he should fix that bug ;)

do you really need #ifdef MOZ_XUL? Is it possible to compile firefox/thunderbird/some other application without XUL?

Will your patch recreate an accessible for current treeitem if it was destroyed?

Comment 13

[Evan Yan]

(In reply to comment #12)
> Ok, if you will file bug for better idea and get a link in your patch at this
> bug then it whould be helpful. Also please add a comment that if someone wants
> to fire delayed events for the tree then he should fix that bug ;)
> 
bug 386821 filed.
do you need a new patch with the added comment? or I can do it before check in.

> do you really need #ifdef MOZ_XUL? Is it possible to compile
> firefox/thunderbird/some other application without XUL?
> 
The code is for xul tree table. I think we usually put xul specific code into #ifdef MOZ_XUL so that we won't break apps without xul.

> Will your patch recreate an accessible for current treeitem if it was
> destroyed?
> 
I think it won't.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)

> > Will your patch recreate an accessible for current treeitem if it was
> > destroyed?
> > 
> I think it won't.
> 

But it should. Right?

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #13)

> > do you really need #ifdef MOZ_XUL? Is it possible to compile
> > firefox/thunderbird/some other application without XUL?
> > 
> The code is for xul tree table. I think we usually put xul specific code into
> #ifdef MOZ_XUL so that we won't break apps without xul.

Iirc we don't follow this rule always. But if you like ifdefs then it's ok. But why next line isn't wrapped by ifdefs?

#include "nsIDOMXULMultSelectCntrlEl.h"

Comment 16

[Evan Yan]

(In reply to comment #14)
> But it should. Right?
> 
Why it should?
Actually I think xul treeitem accessible isn't shutdown-ed individually. They only get shutdown-ed all together when their tree accessible get shutdown-ed and the cache was destroyed.

(In reply to comment #15)
> Iirc we don't follow this rule always. But if you like ifdefs then it's ok. But
> why next line isn't wrapped by ifdefs?
> 
> #include "nsIDOMXULMultSelectCntrlEl.h"
> 
I don't know, just nsRootAccessible.cpp does that.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> (In reply to comment #14)
> > But it should. Right?
> > 
> Why it should?

Because if it is destroyed then you will send wrong event.

> Actually I think xul treeitem accessible isn't shutdown-ed individually. They
> only get shutdown-ed all together when their tree accessible get shutdown-ed
> and the cache was destroyed.

Ok, if tree was destroeyd then you recreate it and accessible for tree will recache its children. Right? Also I assume tree should keep its children accessible up to dated? If so then your code looks correct.

> (In reply to comment #15)
> > Iirc we don't follow this rule always. But if you like ifdefs then it's ok. But
> > why next line isn't wrapped by ifdefs?
> > 
> > #include "nsIDOMXULMultSelectCntrlEl.h"
> > 
> I don't know, just nsRootAccessible.cpp does that.
> 

I just guess without XUL there won't be such interface. Also I can't imagine myself how it is without XUL :)

Aaron, can you clarify the position about MOZ_XUL usage?

Comment 18

[Evan Yan]

(In reply to comment #17)
> Ok, if tree was destroeyd then you recreate it and accessible for tree will
> recache its children. Right?

Yes, tree accessible will cache the treeitem accessible which was accessed.

> Also I assume tree should keep its children
> accessible up to dated? If so then your code looks correct.
> 
It should, while it has some bug now. see bug 368835, which is blocked by this one.

Comment 19

[alexander :surkov (:asurkov)]

Comment on attachment 270588 [details] [diff] [review]
return selected treeitem in nsAccEvent::GetAccessibleByNode()

so r=me with added comment and clear position of #ifdef MOZ_XUL usage.

Comment 20

[alexander :surkov (:asurkov)]

Neil, do you know when to use #ifdef MOZ_XUL (is it possible it is undefiened) and is it nsIDOMXULMultSelectCntrlEl defined if MOZ_XUL is not defined?

Comment 21

[[e-mail]]

(In reply to comment #20)
>Neil, do you know when to use #ifdef MOZ_XUL (is it possible it is undefined)
>and is it nsIDOMXULMultSelectCntrlEl defined if MOZ_XUL is not defined?
You have to disable it, either using --disable-xul or --with-embedding-profile=minimal in your mozconfig.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to comment #21)
> (In reply to comment #20)
> >Neil, do you know when to use #ifdef MOZ_XUL (is it possible it is undefined)
> >and is it nsIDOMXULMultSelectCntrlEl defined if MOZ_XUL is not defined?
> You have to disable it, either using --disable-xul or
> --with-embedding-profile=minimal in your mozconfig.
> 

So, nsIDOMXULMultSelectCntrlEl is not defined if I use --disable-xul or
> --with-embedding-profile=minimal. Right?

Btw, what can I build without XUL?

Comment 23

[[e-mail]]

Sorry, I don't actually know of a project that disables XUL.

Comment 24

[Evan Yan]

patch checked in, with added comment and #include nsIDOMXULMultSelectCntrlEl.h moved into #ifdef MOZ_XUL