415069 - ARIA alerts triggering events with :system

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Scott Haeger]

The ARIA alerts triggered from http://www.mozilla.org/access/dhtml/alert are inconsistent with regards to the event.type ':system' postfix.  I would expect all event types not to have ':system' because these alerts are triggered by user interaction.  The following are event logs for all six alert types.  Only object:text-changed and object:children-changed were monitored.

Alert 1
object:text-changed:insert:system(0, 1, )
	source: [section | ]
	application: [application | Minefield]
object:children-changed:add:system(0, 0, [alert | ])
	source: [section | ]
	application: [application | Minefield]

Alert 2
object:text-changed:insert:system(0, 1, )
	source: [section | ]
	application: [application | Minefield]
object:children-changed:add:system(0, 0, [alert | ])
	source: [section | ]
	application: [application | Minefield]

Alert 3
object:text-changed:insert(9, 1, )
	source: [document frame | Accessible DHTML Alerts]
	application: [application | Minefield]
object:children-changed:add(8, 0, [alert | ])
	source: [document frame | Accessible DHTML Alerts]
	application: [application | Minefield]

Alert 4
object:text-changed:insert(9, 1, )
	source: [document frame | Accessible DHTML Alerts]
	application: [application | Minefield]
object:children-changed:add(8, 0, [alert | ])
	source: [document frame | Accessible DHTML Alerts]
	application: [application | Minefield]

Alert 5
object:text-changed:insert:system(9, 1, )
	source: [document frame | Accessible DHTML Alerts]
	application: [application | Minefield]
object:children-changed:add(8, 0, [alert | ])
	source: [document frame | Accessible DHTML Alerts]
	application: [application | Minefield]

Alert 6
object:text-changed:insert:system(9, 1, )
	source: [document frame | Accessible DHTML Alerts]
	application: [application | Minefield]
object:children-changed:add(8, 0, [alert | ])
	source: [document frame | Accessible DHTML Alerts]
	application: [application | Minefield]

Comment 1

[Aaron Leventhal]

I think I've seen this as well. Unfortunately because of the asynch factor of layout events our code is a bit complicated.

Comment 2

[Ginn Chen]

Aaron,
I'm confused by the sequence of
CreateTextChangeEventForNode
nsAccEvent::CaptureIsFromUserInput
nsAccEvent::PrepareForEvent
FireAccessibleEvent

Creating AccEvents will call CaptureIsFromUserInput.
For synch events, PrepareForEvent is called inside CaptureIsFromUserInput, before mIsFromUserInput is set.
For asynch events, header file said PrepareForEvent should be called from the synchronous code which is the true source of the event, before the event is fired.
But calling PrepareForEvent after CaptureIsFromUserInput won't change mIsFromUserInput.
So I'm confused.

See: nsDocAccessible.cpp line 1533 to line 1550

Comment 3

[Aaron Leventhal]

Hi Ginn, I started writing a long explanation and then found out you must be right.

This must be why it works correctly on Windows -- there we don't use mIsFromUserInput. We use the global variables in order to set on object attribute and hope they are listening to events in process (as JAWS and Window-Eyes do).

Comment 4

[Ginn Chen]

Perhaps we should do nsAccEvent::PrepareForEvent() for show/create events before creating text-changed events.

Comment 5

[Aaron Leventhal]

I'm working on a patch. I think it's the same basic idea.

Comment 6

[Aaron Leventhal]

Attachment: When one event creates another, make use old isFromuserInput in new event and store with the event

I have not tested this on Linux, but I think it will work.

Comment 7

[Ginn Chen]

Comment on attachment 301445 [details] [diff] [review]
When one event creates another, make use old isFromuserInput in new event and store with the event

PrepareForEvent(eventNode);
or
PrepareForEvent(eventNode, aForceIsFromUserInput);
only sets gLastEventFromUserInput.

so
mIsFromUserInput = gLastEventFromUserInput;
should not be removed
and after
+      // One more try -- check to see if we are currently responding to user input
+      PrepareForEvent(eventNode, aForceIsFromUserInput);
should do
aEvent->SetIsFromUserInput(gLastEventFromUserInput);
return;

Also I don't think nsAccEvent::CaptureIsFromUserInput is correct.
If the node is not the previous node, we should reset gLastEventNodeWeak and  gLastEventFromUserInput together.

Comment 8

[Aaron Leventhal]

Attachment: Do what Ginn asked but slightly differently

Ginn, I agreed with what you said, but when I tried it there was a problem on the visibility example. In that case the text node becoming invisible fires an event (which is eventually removed from the queue via eCoalesceFromSameSubtree). However, if ::CaptureIsFromUserInput() resets the global input state then the next event doesn't get the right :system flag even though it is on the node that aria-hidden changed on.

So I made this a little less dependent on where the event happens, and more on "when". Here are the new rules:
1. Reset the input state flag at the end of FlushPendingEvents(). This assumes that a queue of events all probably occured from the same thing, and if not, it's not likely to matter much
2. Don't use a value of TRUE unless it's on the same node, but don't reset the input state just because there is an event on a different node.

I'm tempted to get rid of rule #2: allow the last input state to be used on any event that happens in the same queue -- basically happening at the same time. However, I do think these current rules provide good results. It's safe enough to code such that 2 events that occured basically at the same time on the same node share whether they were caused by the user or not.

Comment 9

[Scott Haeger]

I just tested a try server build and I am getting object:children-changed:add events(good!) and object:children-changed:remove:system events (not good) for all triggered alerts.  The :system on the remove events seems wrong to me because the user presses a link to close the dialog.

Comment 10

[Ginn Chen]

The patch looks good to me.
But I didn't test it yet, and I don't know why children-changed:remove events are still system.

Comment 11

[Scott Haeger]

Also getting object:text-changed:delete:system and object:text-changed:insert events.  

Comment 12

[Aaron Leventhal]

Attachment: Small change -- considers a handful of events getting flushed at the same time to be from the same source

Scott, here is a try server build -- will you see if this works better?

https://build.mozilla.org/tryserver-builds/2008-02-11_12:[e-mail]-IsEventFromUserInput415069/

Basically if an event is from user input, it will copy that flag to other events in the same event queue. Once the queue is flushed that flag is reset. I suspect this is the way to go, as I'm not sure how you could have some events in the same queue that are from user input and later events that aren't.

Comment 13

[Aaron Leventhal]

Comment on attachment 302669 [details] [diff] [review]
Small change -- considers a handful of events getting flushed at the same time to be from the same source

Ginn, this only changes nsAccEvent::CaptureIsFromUserInput
()

Comment 14

[Scott Haeger]

In regards to the try server build as linked from comment #12, I am still seeing :system for most delete and remove events.  The times I do see no :system is when I see an <invalid> object.  The following is the beginning of an event log taken from triggering all the alerts on the Mozilla ARIA alert page.  The remainder of the log is similar.

object:text-changed:delete(0, 1, )
	source: [section | ]
	application: [application | Minefield]
object:text-changed:insert(0, 1, )
	source: [section | ]
	application: [application | Minefield]
object:children-changed:add(0, 0, [invalid | ])
	source: [section | ]
	application: [application | Minefield]
object:children-changed:remove(0, 0, [invalid | ])
	source: [section | ]
	application: [application | Minefield]
object:text-changed:insert(0, 1, )
	source: [section | ]
	application: [application | Minefield]
object:children-changed:add(0, 0, [alert | ])
	source: [section | ]
	application: [application | Minefield]
object:text-changed:delete(0, 1, )
	source: [section | ]
	application: [application | Minefield]
object:children-changed:remove:system(0, 0, [invalid | ])
	source: [section | ]
	application: [application | Minefield]

With all this said, this is a low priority item for Orca.  The <invalid> objects concern me much more than the :system postfix.

Comment 15

[Aaron Leventhal]

I think we should at least get in this change that we have posted, because it is a clear improvement.

Ginn do you want to look and see if you can determine the cause of the other issues?

Or should we just get this in.

Comment 16

[Ginn Chen]

Aaron, the patch is OK to get in.
I'll take a look at the other issues tomorrow.

Scott, I think the <invalid> objects issue is because the accessible object was defunct, when accerciser is trying to get its name and role.

Comment 17

[Aaron Leventhal]

That makes sense to me -- after an object is removal it's correct for it to be invalid (defunct).

Ginn, we should look at why the removal/delete events are :system if they shouldn't be.

Comment 18

[Scott Haeger]

(In reply to comment #17)
> That makes sense to me -- after an object is removal it's correct for it to be
> invalid (defunct).

It also make sense to me, but how would I report that a user has left a chat room.  It would be impossible with an <invalid> object.  Is this a limitation of AT-SPI?

Comment 19

[Aaron Leventhal]

Yes, AT-SPI events being asynch is a problem.

I see two ways:
1. You would have to cache everything in any place where aria-relevant contained removals
2. We'd use event data

On #2, what do the text removal events say ... you're only getting the embedded obj char as being removed right?

Comment 20

[Scott Haeger]

(In reply to comment #19)
> Yes, AT-SPI events being asynch is a problem.
> 
> I see two ways:
> 1. You would have to cache everything in any place where aria-relevant
> contained removals
> 2. We'd use event data
This is a great idea in theory.  However, I have avoided holding onto objects because of numerous issues seen in the past for both Orca and Firefox.

> On #2, what do the text removal events say ... you're only getting the embedded
> obj char as being removed right?
> 
correct

Comment 21

[Aaron Leventhal]

Maybe for text removal events we should expand out the embedded object chars so you get all the text in the event data.

Anyway we need to file a separate bug for it.

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 302669 [details] [diff] [review]
Small change -- considers a handful of events getting flushed at the same time to be from the same source

a=beltzner

Comment 23

[Ginn Chen]

Aaron, please hold the patch.

Sorry, I didn't review it carefully enough.

The patch didn't fully address comment #7.
We should do
mIsFromUserInput = gLastEventFromUserInput;
for both synch and asynch case in CaptureIsFromUserInput().
Otherwise we have problems with synch events as removal/delete events.

Sorry, I didn't make it clear.

And, I think it doesn't make sense to have aIsAsynch parameter together with nsIAccessibleEvent object for nsDocAccessible::FireDelayedAccessibleEvent()
PrepareForEvent(domNode) should be done when creating nsIAccessibleEvent object, not in nsDocAccessible::FireDelayedAccessibleEvent(nsIAccessibleEvent, EDupeEventRule, PRBool aIsAsynch);

These lines should be removed.
1419   if (!aIsAsynch) {
1420     // If already asynchronous don't call PrepareFromEvent() -- it
1421     // should only be called while ESM still knows if the event occurred
1422     // originally because of user input
1423     nsAccEvent::PrepareForEvent(newEventDOMNode);
1424   }
And we should fix
1391   nsCOMPtr<nsIAccessibleEvent> event =
1392     new nsAccEvent(aEvent, aDOMNode, PR_TRUE);
1393   NS_ENSURE_TRUE(event, NS_ERROR_OUT_OF_MEMORY);
1394 
1395   return FireDelayedAccessibleEvent(event, aAllowDupes, aIsAsynch);
and
1966       nsCOMPtr<nsIAccessibleEvent> reorderEvent =
1967         new nsAccEvent(nsIAccessibleEvent::EVENT_REORDER, containerAccessible, PR_TRUE);
1968       NS_ENSURE_TRUE(reorderEvent, NS_ERROR_OUT_OF_MEMORY);
1969       FireDelayedAccessibleEvent(reorderEvent, eCoalesceFromSameSubtree, isAsynch);

Comment 24

[Ginn Chen]

Attachment: patch

as comment #23.

Aaron, is it correct?

Comment 25

[Aaron Leventhal]

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

Thank you Ginn.

Comment 26

[Ginn Chen]

Checking in public/nsIAccessibleEvent.idl;
/cvsroot/mozilla/accessible/public/nsIAccessibleEvent.idl,v  <--  nsIAccessibleEvent.idl
new revision: 1.20; previous revision: 1.19
done
Checking in src/base/nsAccessibleEventData.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessibleEventData.cpp,v  <--  nsAccessibleEventData.cpp
new revision: 1.28; previous revision: 1.27
done
Checking in src/base/nsAccessibleEventData.h;
/cvsroot/mozilla/accessible/src/base/nsAccessibleEventData.h,v  <--  nsAccessibleEventData.h
new revision: 1.31; previous revision: 1.30
done
Checking in src/base/nsDocAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsDocAccessible.cpp,v  <--  nsDocAccessible.cpp
new revision: 1.229; previous revision: 1.228
done
Checking in src/base/nsDocAccessible.h;
/cvsroot/mozilla/accessible/src/base/nsDocAccessible.h,v  <--  nsDocAccessible.h
new revision: 1.74; previous revision: 1.73
done
Checking in src/base/nsRootAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsRootAccessible.cpp,v  <--  nsRootAccessible.cpp
new revision: 1.258; previous revision: 1.257
done