418043 - add mochitest for bug 308564

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Do not invalidate tree accessible if tree view is changed because while we recreating tree accessible we miss important events. But it's usual thing to work with tree immediately after tree view is changed.

Comment 1

[Aaron Leventhal]

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

Surkov mentioned that this is not really just adding a test. He has an explanation.

Comment 2

[alexander :surkov (:asurkov)]

I put some explanation into description of the bug. But I change the bug title. If you need any additional clarification about this I will do.

Comment 3

[Evan Yan]

In what kind of scenario, we will get "TreeViewChanged" event?

Comment 4

[Evan Yan]

I can only get "TreeViewChanged" event when switch to another folder in Thunderbird. Is the dom tree node still the same or a new one when get the event? If it's the same node, I think it's reasonable to not invalidate tree accessible.

Comment 5

[alexander :surkov (:asurkov)]

Example from mochitest:

gTreeBox.view = gView; // "TreeViewChanged" will be fired
++gView.mRowCount;
gTreeBox.rowCountChanged(0, 1); // We can't process this event correctly due to tree accessible invalidation

I don't know thunderbird implementation but I guess it's the same tree but another view because columns are the same and it doesn't make sense to create tree node. But if they will recreate tree then we fall into usual scenario and will invalidate it.

Comment 6

[Marco Zehe (:MarcoZ)]

Evan, the scenario that bug 308564 was created for was the change in a tree item when the number of unread messages gets updated in a folder (for example when synchronizing with a newsgroup).

Comment 7

[Evan Yan]

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

>+  nsCOMPtr<nsIAccessibleEvent> event =
>+    new nsAccEvent(nsIAccessibleEvent::EVENT_DOM_DESTROY,
>+                   accessible, PR_FALSE);
>+  paccessible->FireAccessibleEvent(event);

This will fire accessible event for every tree item, so that when switching folders in Thunderbird, hundreds of events will be fired. I would prefer the way we do now - just invalidate and fire event for the tree accessible.

Why will recreating tree accessible make us miss events?

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> (From update of attachment 303827 [details] [diff] [review])
> >+  nsCOMPtr<nsIAccessibleEvent> event =
> >+    new nsAccEvent(nsIAccessibleEvent::EVENT_DOM_DESTROY,
> >+                   accessible, PR_FALSE);
> >+  paccessible->FireAccessibleEvent(event);
> 
> This will fire accessible event for every tree item, so that when switching
> folders in Thunderbird, hundreds of events will be fired. I would prefer the
> way we do now - just invalidate and fire event for the tree accessible.

But we do this in any way in nsDocAccessible::FireShowHideEvents. And tree accessible shouldn't be recreated because nothing happens with it.

> Why will recreating tree accessible make us miss events?
> 

We send delayed event to the tree and after delay we recreate it therefore we catch events before we invalidate the tree. The tree has obsolete view and we can't process event correctly.

Comment 9

[Evan Yan]

(In reply to comment #8)
> But we do this in any way in nsDocAccessible::FireShowHideEvents.

Do we? I didn't see that.

> And tree accessible shouldn't be recreated because nothing happens with it.

Yes, I agree. However, hundreds of events seems worse to me.

> We send delayed event to the tree and after delay we recreate it therefore we
> catch events before we invalidate the tree. The tree has obsolete view and we
> can't process event correctly.
> 
If we use EVENT_DOM_DESTROY when handling "TreeViewChanged" evnet:

662     return accService->InvalidateSubtreeFor(eventShell, treeContent,
663                  nsIAccessibleEvent::EVENT_ASYNCH_SIGNIFICANT_CHANGE);

so that invalidate will take place with no delay. Then will the missing event problem get fixed?
"TreeViewChanged" event doesn't sound like a async event to me.

Comment 10

[Evan Yan]

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

clear review for now until the comment is answered/addressed.

Comment 11

[alexander :surkov (:asurkov)]

I don't think it helps because we always fire hide events by delays. I would agree hundreds of events isn't good but that's what's actually happen. It sounds like AI and we should do this when accessible has many removed children.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> I don't think it helps because we always fire hide events by delays.

Any idea, Evan?

Comment 13

[alexander :surkov (:asurkov)]

Could we do a trick to fire hide/show event on tree accessible but actually do not recreate it? Could we lie to screen readers?

Comment 14

[Evan Yan]

(In reply to comment #12)
> (In reply to comment #11)
> > I don't think it helps because we always fire hide events by delays.
> 
> Any idea, Evan?
> 
Let's make clear how the event is missing first. This is my understanding, please correct me if I'm wrong.

First, Some dom event comes, which makes us invalidate the tree accessible and fire accessible events by delays. Then some other dom event comes before the tree accessible actually being invalidated and the accessible event actually getting fired. Because we're still holding the invalid tree accessible, so that we failed handling the new coming event.

If that's right, I think it would fix the problem to use a sync event so that we'll invalidate the tree accessible with no delay. Then when the new event comes, tree accessible will be recreated.

(In reply to comment #13)
> Could we do a trick to fire hide/show event on tree accessible but actually do
> not recreate it? Could we lie to screen readers?
> 
Sounds fine to me.

Comment 15

[alexander :surkov (:asurkov)]

Attachment: mochitest patch only

It's strange but it doesn't fail for me anymore. Evan, could you try it?

Comment 16

[Evan Yan]

Comment on attachment 306829 [details] [diff] [review]
mochitest patch only

please change the bug subject correspondingly.

Comment 17

[alexander :surkov (:asurkov)]

checked in

/cvsroot/mozilla/accessible/tests/mochitest/test_bug368835.xul,v  <--  test_bug368835.xul
new revision: 1.5; previous revision: 1.4
done