Closed Bug 386161 Opened 15 years ago Closed 15 years ago

no focus event when arrowing down in xul tree table

Categories

(Core :: Disability Access APIs, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: evan.yan, Assigned: evan.yan)

References

Details

(Keywords: access)

Attachments

(1 file, 1 obsolete file)

when arrowing down in xul tree table, like thunderbird message list, firefox bookmark manager, there is no focus event for the focused row/cell.
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.
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.
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"
Attachment #270166 - Flags: review?(aaronleventhal)
Attachment #270166 - Flags: review?(aaronleventhal) → review?(surkov.alexander)
(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.           
(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 on attachment 270166 [details] [diff] [review]
use finalFocusAccessible instead of finalFocusNode

r- per comment #5
Attachment #270166 - Flags: review?(surkov.alexander) → review-
(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().
(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.
Attachment #270166 - Attachment is obsolete: true
Attachment #270588 - Flags: review?(surkov.alexander)
(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?
(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.
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?
(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.
(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?
(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"
(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.
(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?
(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 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.
Attachment #270588 - Flags: review?(surkov.alexander) → review+
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?
(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.
(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?
Sorry, I don't actually know of a project that disables XUL.
patch checked in, with added comment and #include nsIDOMXULMultSelectCntrlEl.h moved into #ifdef MOZ_XUL
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.