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.

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

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.

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

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?

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

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.

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

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.

Please see bug 378175

taking while ginn on vacation and I'm about here

fix SHOW/HIDE events processing

Comment on attachment 265481 [details] [diff] [review] patch new patch is comming up

patch isn't checked on windows

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.

PRBool add; // true for add, false for delete I'd like 3 types: add/remove/change

+ 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.

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

(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.

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.

(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.

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

(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?

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.

(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?

> 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.

(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.

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.

(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.

> 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.

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?

Yes, I think that is right.

Comment on attachment 266609 [details] [diff] [review] patch4 Waiting for new patch based on recent comments.

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 on attachment 269184 [details] [diff] [review] patch5 ginn, can you check the patch on linux?

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()?

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?

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.

(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.

> 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?

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

Comment on attachment 269184 [details] [diff] [review] patch5 Minusing until we get simpler patch which doesn't need special GetIndexInParentChain* methods.

checked in

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

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.

(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?

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