Closed
Bug 415069
Opened 16 years ago
Closed 16 years ago
ARIA alerts triggering events with :system
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: scott, Assigned: aaronlev)
Details
Attachments
(2 files, 2 obsolete files)
8.00 KB,
patch
|
Details | Diff | Splinter Review | |
16.15 KB,
patch
|
aaronlev
:
review+
mtschrep
:
approval1.9+
|
Details | Diff | Splinter Review |
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]
Assignee | ||
Comment 1•16 years ago
|
||
I think I've seen this as well. Unfortunately because of the asynch factor of layout events our code is a bit complicated.
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
Assignee | ||
Comment 3•16 years ago
|
||
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).
Perhaps we should do nsAccEvent::PrepareForEvent() for show/create events before creating text-changed events.
Assignee | ||
Comment 5•16 years ago
|
||
I'm working on a patch. I think it's the same basic idea.
Assignee | ||
Comment 6•16 years ago
|
||
I have not tested this on Linux, but I think it will work.
Attachment #301445 -
Flags: review?(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.
Assignee | ||
Comment 8•16 years ago
|
||
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.
Attachment #301445 -
Attachment is obsolete: true
Attachment #301501 -
Flags: review?(ginn.chen)
Attachment #301445 -
Flags: review?(ginn.chen)
Reporter | ||
Comment 9•16 years ago
|
||
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•16 years ago
|
||
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.
Reporter | ||
Comment 11•16 years ago
|
||
Also getting object:text-changed:delete:system and object:text-changed:insert events.
Assignee | ||
Comment 12•16 years ago
|
||
Scott, here is a try server build -- will you see if this works better? https://build.mozilla.org/tryserver-builds/2008-02-11_12:51-aaronleventhal@moonset.net-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.
Assignee | ||
Comment 13•16 years ago
|
||
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 ()
Attachment #302669 -
Flags: review?(ginn.chen)
Reporter | ||
Comment 14•16 years ago
|
||
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.
Assignee | ||
Comment 15•16 years ago
|
||
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.
Attachment #301501 -
Flags: review?(ginn.chen)
Attachment #302669 -
Flags: review?(ginn.chen) → review+
Comment 16•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Attachment #302669 -
Flags: approval1.9?
Assignee | ||
Comment 17•16 years ago
|
||
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.
Reporter | ||
Comment 18•16 years ago
|
||
(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?
Assignee | ||
Comment 19•16 years ago
|
||
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?
Reporter | ||
Comment 20•16 years ago
|
||
(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
Assignee | ||
Comment 21•16 years ago
|
||
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•16 years ago
|
||
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
Attachment #302669 -
Flags: approval1.9? → approval1.9+
Comment 23•16 years ago
|
||
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•16 years ago
|
||
as comment #23. Aaron, is it correct?
Attachment #303229 -
Flags: review?(aaronleventhal)
Assignee | ||
Comment 25•16 years ago
|
||
Comment on attachment 303229 [details] [diff] [review] patch Thank you Ginn.
Attachment #303229 -
Flags: review?(aaronleventhal)
Attachment #303229 -
Flags: review+
Attachment #303229 -
Flags: approval1.9?
Updated•16 years ago
|
Attachment #303229 -
Flags: approval1.9? → approval1.9+
Attachment #302669 -
Attachment is obsolete: true
Comment 26•16 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•