378468 - Correct ATK children_changed events and handling of EVENT_SHOW/HIDE/REORDER

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

The console is getting extreme noise from this method.

We need to remove the console messages when there is not a true error, or fix the errors if they're real.

Comment 1

[Ginn Chen]

Aaron, can you specify your use case and your platform?
I didn't see this with last week's build on Ubuntu 7.04.

Comment 2

[Aaron Leventhal]

I'm on Ubuntu 7.04, running with today's Mozilla build.

Just tab around so that the location bar and search box are in your tab order.

Comment 3

[Ginn Chen]

Attachment: patch

I couldn't reproduce your problem.
I got a warning here when tabbing into/out of search box.
This patch solved it.

Comment 4

[Aaron Leventhal]

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

Would this a little more obvious?
If (IsText(tmpAccesible))) {
return NS_OK;
}

What kind of event are we trying to fire on text?

Comment 5

[Ginn Chen]

EVENT_HIDE
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsDocAccessible.cpp#1387

Comment 6

[Aaron Leventhal]

The EVENT_SHOW or EVENT_HIDE. They should always come along with an EVENT_REORDER.  We should be firing children_changed:add or children_changed:remove events. 

All of these are handled in nsDocAccessibleWrap right now, and I don't believe we're firing the right ATK events for them.

The EVENT_SHOW or EVENT_HIDE should not be firing state change for VISIBLE/SHOWING because we actually remove accessibles or create accessibles for them. Perhaps we should not be handling them at all, and do it all through EVENT_REORDER.

Also we need to do the similar refactoring that Surkov has been doing for the other events.

Comment 7

[Aaron Leventhal]

Ginn, I'm changing the bug to deal with the deeper issue that this assertion is showing us.

Comment 8

[Aaron Leventhal]

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

Minusing -- it looks like the EVENT_SHOW/HIDE and REORDER code needs to be brought into line.

Where there is a SHOW/HIDE event we should e firing the children_changed event on the parent (where the REORDER event will come). We shouldn't fire a state change event for that, because the accessible object actually goes away or is created.

Comment 9

[Aaron Leventhal]

Please see bug 378175

Comment 10

[alexander :surkov (:asurkov)]

taking while ginn on vacation and I'm about here

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch

fix SHOW/HIDE events processing

Comment 12

[alexander :surkov (:asurkov)]

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

new patch is comming up

Comment 13

[alexander :surkov (:asurkov)]

Attachment: patch2

patch isn't checked on windows

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 15

[Aaron Leventhal]

Nit: NS_ENSURE_ARG_POINTER instead of NS_ENSURE_ARG

I think we were going to get rid of nsIAccessibleEvent::EVENT_REORDER and change it to nsIAccessibleEvent::EVENT_OTHER_CHANGE or something like that. That way the nsIAccessibleEvent events can always be on the node that's changing, and MSAA or ATK can map it to the correct set of events for them.

Comment 16

[Aaron Leventhal]

PRBool        add;    // true for add, false for delete

I'd like 3 types: add/remove/change

Comment 17

[Aaron Leventhal]

+    case nsIAccessibleEvent::EVENT_REORDER:
+        MAI_LOG_DEBUG(("\n\nReceived: EVENT_REORDER\n"));
+        g_signal_emit_by_name(atkObj, "children_changed::add", -1, NULL, NULL);
+        return NS_OK;

It should be something like:
case nsIAccessibleEvent::EVENT_OBJECT_CHANGE:
         FireAtkShowHideEvent(aEvent, atkObj, PR_FALSE);
         break;

Although, perhaps
s/FireAtkShowHideEvent/FireAtkChildrenChangedEvent() since it's not necessarily a show or a hide.

Comment 18

[Aaron Leventhal]

I think you have the idea. Basically we want to get rid of the broken system I created with nsIAccessibleEvent::EVENT_REORDER. If I grep for REORDER in mozilla/accessible I should only see it in accessible/src/msaa

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> Nit: NS_ENSURE_ARG_POINTER instead of NS_ENSURE_ARG
> 
> I think we were going to get rid of nsIAccessibleEvent::EVENT_REORDER and
> change it to nsIAccessibleEvent::EVENT_OTHER_CHANGE or something like that.
> That way the nsIAccessibleEvent events can always be on the node that's
> changing, and MSAA or ATK can map it to the correct set of events for them.
> 

These are big changes I tried to avoid. The patch makes we fire reorder event only on parent when there is no show/hide events. So we'll have behavior in gecko like in ATK. For MSAA if I get show/hide events then I fire additionaly reorder event on parent. So it looks we get correct behavior with small changes.

Comment 20

[Aaron Leventhal]

My feeling is that I did it wrong and we should really fix it properly.

I understand the changes would be bigger, but how much bigger?

If you want I can take over the bug and provide a patch that changes nsIAccessibleEvent::EVENT_REORDER to something else.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> My feeling is that I did it wrong and we should really fix it properly.
> 
> I understand the changes would be bigger, but how much bigger?

The changes will be outside of accessible module I don't like it already :)

> If you want I can take over the bug and provide a patch that changes
> nsIAccessibleEvent::EVENT_REORDER to something else.
> 

Actually we fire EVENT_REORDER from unique place. Therefore I think it's not worth to have additional EVENT_CHANGE nor to rename EVENT_REORDER.

Now if would like to fire show/hide/reorder event we call InvalidateChildren (and therefore we have unique place where we fire reorder event). So if you mean  InvalidateChildren should be an action of the event (and of course it shouldn't fire any event) then you or I may do it in follow up bug.

In current approach (plus my patch) I can see one not good thing is InvalidateChildren is not an action of the event.

Comment 22

[Aaron Leventhal]

I think I still like that approach even though it requires some small changes outside of mozilla/accessible.

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #22)
> I think I still like that approach even though it requires some small changes
> outside of mozilla/accessible.
> 

Which approach? To make InvalidateCacheSubtree() an action on show/hide/change event?

Comment 24

[Aaron Leventhal]

Right. We should not have nsIAccessibleEvent::EVENT_REORDER -- we can't keep it like that because REORDER in MSAA is really something fired on a parent, not on the object.  It's very confusing to use the event 2 different ways.

We need a EVENT_CHANGE -- perhaps EVENT_ROLE_CHANGE or something to show the type of object is changing.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #24)
> Right. We should not have nsIAccessibleEvent::EVENT_REORDER -- we can't keep it
> like that because REORDER in MSAA is really something fired on a parent, not on
> the object.  It's very confusing to use the event 2 different ways.

But we do not fire event reorder for object that was changed. Within my patch reorder event is fired only when object is changed (not shown/hidden).

> We need a EVENT_CHANGE -- perhaps EVENT_ROLE_CHANGE or something to show the
> type of object is changing.
> 

But now why do we need change event until all magic is happen inside of InvalidateCacheSubtree()? If we would fire an event when accessible was changed then I would agree but if we call InvalidateCacheSubtree() that fires an event then it doesn't make sense. Am I wrong?

Comment 26

[Aaron Leventhal]

> But we do not fire event reorder for object that was changed. Within my patch
> reorder event is fired only when object is changed (not shown/hidden).
We should still fire it on parent of changed object because the accessible is likely to need to change. Any time an accessible object is no longer correct and is removed from the cache, the parent needs to fire a REORDER, even if there will be a new accessible. This allows the AT to invalidate its cache.

Since REORDER is for parent objects of changes, this line of code is confusing:
http://lxr.mozilla.org/seamonkey/source/layout/base/nsCSSFrameConstructor.cpp#11128

I didn't understand the second paragraph of your comment.

If you want I can take a turn on this patch and then see what you think.

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #26)
> > But we do not fire event reorder for object that was changed. Within my patch
> > reorder event is fired only when object is changed (not shown/hidden).
> We should still fire it on parent of changed object because the accessible is
> likely to need to change. Any time an accessible object is no longer correct
> and is removed from the cache, the parent needs to fire a REORDER, even if
> there will be a new accessible. This allows the AT to invalidate its cache.

That's how we do excepting if invalidateCacheSubtree() is called for document accessible (see http://lxr.mozilla.org/mozilla/source/accessible/src/base/nsDocAccessible.cpp#1484).


> Since REORDER is for parent objects of changes, this line of code is confusing:
> http://lxr.mozilla.org/seamonkey/source/layout/base/nsCSSFrameConstructor.cpp#11128

Not sure. Here we do not fire any event. We just call InvalidateCacheSubtree that takes event names as an argument. In general I think it's not correct to use events as argument of InvalidateCacheSubtree. So I would prefer either:
1) InvalidateCacheSubtree will use own constants (like SHOW/HIDE/CHANGE).
2) We won't call InvalidateCacheSubtree directly but instead we will fire an event that will call InvalidateCacheSubtree.

The first way is much easier and it removes confusing with reorder event.

> If you want I can take a turn on this patch and then see what you think.
> 

yes, please. I think the patch should fix the problem of this bug. With confustions we can deal in another bug.

Comment 28

[Aaron Leventhal]

This is a hack for ATK which is just incorrect.
        g_signal_emit_by_name(atkObj, "children_changed::add", -1, NULL, NULL);

If a child changes in a big way that it needs to be invalidated, we should fire two events: children_changed:remove followed by children_changed:add for the newly created child.

Comment 29

[alexander :surkov (:asurkov)]

Attachment: patch4

(In reply to comment #28)
> This is a hack for ATK which is just incorrect.
>         g_signal_emit_by_name(atkObj, "children_changed::add", -1, NULL, NULL);
> 
> If a child changes in a big way that it needs to be invalidated, we should fire
> two events: children_changed:remove followed by children_changed:add for the
> newly created child.
> 

Ok, I guess it should be almost how you wanted it excepting I didn't renamed EVENT_REORDER. So if you fine with the patch then I'd better to file following up patch to do renaming stuff.

Comment 30

[Aaron Leventhal]

> gWinEventMap[nsIAccessibleEvent::EVENT_HIDE]
> gWinEventMap[nsIAccessibleEvent::EVENT_SHOW]
> gWinEventMap[nsIAccessibleEvent::EVENT_REORDER]
I think it's better to just put EVENT_OBJECT_{HIDE|SHOW|REORDER} directly. The extra indirection doesn't really help IMO.

In accessible/src/atk/nsDocAccessibleWrap we are now firing a hide event for 
case nsIAccessibleEvent::EVENT_MENUPOPUP_START:
We used to fire a show event for that. See the |break| that was purposely missing from that case.

  nsCOMPtr<nsPIAccessible> privateChildAccessible(do_QueryInterface(childAccessible));
+ NS_ENSURE_STATE(privateChildAccessible);
I'd rather use NS_ASSERTION() here if anything since that's always true. No need to take up extra code size for something that's 100% always true.

// XXX todo: We might want to use XPCOM interfaces instead of structs
//     e.g., nsAccessibleTextChangeEvent: public nsIAccessibleTextChangeEvent
You can remove that comment now, since you're doing these one at a time.

-  if (aChangeEventType == nsIAccessibleEvent::EVENT_HIDE) {
-    // Fire EVENT_HIDE or EVENT_MENUPOPUP_END if previous accessible existed
-    // for node being hidden. Fire this before the accessible goes away
- ...
I assume your patch still makes sure that EVENT_HIDE occurs for old accessibles.
However, looking at how REORDER is handled in the ATK code it looks like children_changed:remove and children_changed:add are 
fired for the same object. The remove event should be getting fired for the old object.

Comment 31

[alexander :surkov (:asurkov)]

So probably we shound't fire reorder event in gecko. We can fire just show/hide events and fire additional events when needed for every platform partially. Sounds ok?

Comment 32

[Aaron Leventhal]

Yes, I think that is right.

Comment 33

[Aaron Leventhal]

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

Waiting for new patch based on recent comments.

Comment 34

[alexander :surkov (:asurkov)]

Attachment: patch5

only show/hide events are fired in gecko. I need to file followin up bug to get rid event_reorder constant from nsIAccessibleEvent interface. Also, Aaron, can you check the patch on linux because I have not linux until I get back home.

Comment 35

[alexander :surkov (:asurkov)]

Comment on attachment 269184 [details] [diff] [review]
patch5

ginn, can you check the patch on linux?

Comment 36

[Aaron Leventhal]

The way we fire a special EVENT_SHOW for progress meters that appear is a hack. I'm checking to see if we even need to do that. 

I'd like to simplify the code so that the index in parent calculation is only done in ATK where/when it is needed. I think it's okay if we create an accessible object if we really need the parent. In what situations might that not be okay?

What if we just say that it's okay to create the parent accessible if we're in ATK and the event is being fired. 

Then we can just use nsIAccessible::GetIndexInParent() right? Would we still need GetIndexInParentChain()?

Comment 37

[Aaron Leventhal]

Also, with this patch if an object is changed, will we fire:
EVENT_HIDE
EVENT_REORDER
EVENT_SHOW
EVENT_REORDER
?

That seems to be an extra EVENT_REORDER. I know that I said we should be able to remove that and just fire it in MSAA, but I now think we need to keep it as an nsIAccessibleEvent.  The idea is that AT can use it to keep the cache updated, so the fewer of them, the better. It won't have to update the cache extra times.

In fact, if we fire EVENT_REORDER with FireDelayedAccessibleEvent with aAllowDupes = PR_FALSE, even more of the EVENT_REORDER's will be collated into one, which is good. That means if >1 child is show/hidden within a given parent, only 1 EVENT_REORDER will be fired for the parent accessible, and thus the AT cache is only updated once. Right?

Comment 38

[alexander :surkov (:asurkov)]

Actually we show/hide/reorder events are proccesed in atk/msaa only and currently we do not update the cache on these events (we fire these events when we update the cache), though I think it's more nicer to update the cache on these events. Please correct me if I'm wrong.

In msaa I got an additional reorder event. To avoid this I guess I can fire hide/show/reorder events in gecko when accessible is changed. Correct?

Also I think we can calculate index in parent and parent accessible on windows too because it's nicer to have common code in gecko and probably this information may be usefull for script listeners.

I do not call GetIndexInParent/GetParent and I use GetIndexInParentChain/GetParentInChain because document is not loaded in that moment and I shouldn't create any accessibles. 

Comment 39

[alexander :surkov (:asurkov)]

(In reply to comment #36)
> The way we fire a special EVENT_SHOW for progress meters that appear is a hack.
> I'm checking to see if we even need to do that. 

If we're going to remove this then it's better to do in another bug for easier tracking.

Comment 40

[Aaron Leventhal]

> currently we do not update the cache on these events 
I was talking about the screen reader's cache (if they use one). They often also call this a virtual buffer. They need to use our events to know when to update it.

To avoid extra REORDER events you'll have to use FireDelayedAccesibleEvent for EVENT_REORDER in Gecko, and use aAllowDupes = PR_FALSE as an argument. Is that what you mean?

Comment 41

[Aaron Leventhal]

I suggest we fix bug 385447 and bug 385474 first -- one per day, so we can check for regressions easily.

Comment 42

[Aaron Leventhal]

Comment on attachment 269184 [details] [diff] [review]
patch5

Minusing until we get simpler patch which doesn't need special GetIndexInParentChain* methods.

Comment 43

[alexander :surkov (:asurkov)]

Attachment: patch6

Comment 44

[alexander :surkov (:asurkov)]

checked in

Comment 45

[Ginn Chen]

Alexander, it seems Firefox became unstable with this patch, not sure if it is related.
See http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox-Ports&hours=24&maxdate=1183173738&legend=0

Comment 46

[Ginn Chen]

1)
WARNING: NS_ENSURE_TRUE(privateChildAccessible) failed: file nsDocAccessible.cpp, line 1413
failed too often, we need to address this.

2)
For the crash, to reproduce, you can run this command several times.

firefox  -P default "http://axolotl.mozilla.org/page-loader/loader.pl?delay=1000&nocache=0&maxcyc=4&timeout=30000&auto=1"

The stack is confusing, but I can't recreate the crash if this patch is backed out.

Comment 47

[alexander :surkov (:asurkov)]

(In reply to comment #45)
> Alexander, it seems Firefox became unstable with this patch, not sure if it is
> related.
> See
> http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox-Ports&hours=24&maxdate=1183173738&legend=0
> 

It looks crash should happen at http://lxr.mozilla.org/mozilla/source/accessible/src/base/nsRootAccessible.cpp#584. 

Do we fire any events on 'pagehide' event?

Comment 48

[alexander :surkov (:asurkov)]

Attachment: warning patch

Comment 49

[Ginn Chen]

the crash issue is another bug, bug 386836 is filed.
close this one.