Last Comment Bug 392061 - Live region object attributes are not being persisted for AT-SPI
: Live region object attributes are not being persisted for AT-SPI
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86 Linux
: -- blocker (vote)
: ---
Assigned To: Aaron Leventhal
:
: alexander :surkov
Mentors:
Depends on:
Blocks: aria
  Show dependency treegraph
 
Reported: 2007-08-13 10:37 PDT by Scott Haeger
Modified: 2007-09-18 14:40 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Experimental ":system" string appended to event names when they are not from user input (8.62 KB, patch)
2007-09-06 18:12 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Same patch, but also allow container-foo object attributes on any node, not just last event node. Doesn't change the ":system" code. (10.27 KB, patch)
2007-09-07 06:03 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Builds properly. Also removes a warning and unused link-selected code (13.42 KB, patch)
2007-09-07 13:17 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Fix for text-changed::inserted/removed events, but Accerciser doesn't see "::system" (14.40 KB, patch)
2007-09-07 14:23 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Minimize risk: only affect the events we really need to know this for -- children-changed and text-changed events (5.62 KB, patch)
2007-09-07 16:27 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Same thing, but with single colon ":system" (5.62 KB, patch)
2007-09-08 18:37 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
1) Also fix member-of relation for atomic regions, 2) Move code for container attributes to more appropriate places (13.02 KB, patch)
2007-09-10 11:17 PDT, Aaron Leventhal
ginn.chen: review+
dsicore: approval1.9+
Details | Diff | Splinter Review
Same thing, but add MEMBER_OF to list of supported relations in ATK (13.77 KB, patch)
2007-09-11 06:54 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review

Description Scott Haeger 2007-08-13 10:37:00 PDT
Live region object attributes, those that are derived from the ancestral tree anyways, are not being persisted.  Tests (see below) show that the ancestral attributes do appear but are later changed back to only those attributes that are in the markup for that object.  Subsequent events may interfere somehow.

Test case: http://accessibleajax.clcworld.net/scoreboard/scoreboard.htm
Test procedure:
Using Accerciser, select the h2 representing the clock.
In the IPython console enter the following commands:

import time
for i in range(100):
  time.sleep(0.1)
  print acc.getAttributes()

This will call and print the attributes for this object 100 times.  On average only 2/10 will contain the desired attributes.
Comment 1 Aaron Leventhal 2007-08-13 10:51:33 PDT
Is there a way to squeeze this information into ATK/AT-SPI event data?
Comment 2 Aaron Leventhal 2007-08-31 14:00:01 PDT
Ginn, how can we set the event details?

Is there a more manual way to fire events like text_changed::insert than this?
g_signal_emit_by_name (aObject, "text_changed::insert", start, length);

I'd like to set the text for insert/delete events, and I'd like to use detail2 to provide stuff like event-from-user-input, etc.
Comment 3 Scott Haeger 2007-09-04 08:18:57 PDT
The prototype for g_signal_emit_by_name is the following:

void g_signal_emit_by_name(gpointer instance,const gchar *detailed_signal,...);  

All extra parameters are sent to the signal handler.  Can you adjust the signal handler according to what you want?  My reference was http://library.gnome.org/devel/gobject/unstable/gobject-Signals.html#g-signal-emit-by-name
Comment 4 Aaron Leventhal 2007-09-06 12:44:16 PDT
See spi_atk_bridge_signal_listener in atk-bridge/bridge.c 
(http://svn.gnome.org/viewcvs/at-spi/trunk/atk-bridge/bridge.c)

The only additional info that we can pass on untouched is a string called signal_hint->detail
(from GSignalInvocationHint *signal_hint)

I'm not sure how to set that, but so far looking at the code I think it's actually passed in the string event name, and is the part after ::. For example, in "text_changed::insert" the detail is "insert".

We could change the detail from "insert" to something like "insert::event-from-user-input=true,container-live=polite"

I'm not sure if that would break anything. It probably would.
Comment 5 Aaron Leventhal 2007-09-06 12:59:11 PDT
I propose:
1) object attributes can be fixed for the "container-foo" stuff, because those don't change from 1 event to the next -- it's based on the object's containers, so they really are object attributes.
2) For whether the event is from the user input or not is really an attribute of the event. This we should do by appending the info to the event name, but only when it's a system event. User input generated events would look the same as they do now.
So, typing text would look like:
text-changed:insert
The web page inserting text would look like:
text-changed:insert:system

I really don't think it's bad because it is basically a subclass type of event.

Maybe we can come up with a better word than "system" for it.
Comment 6 Aaron Leventhal 2007-09-06 13:01:13 PDT
Or, maybe we can call spi_atk_emit_eventv() directly.
Comment 7 Aaron Leventhal 2007-09-06 18:12:20 PDT
Created attachment 280002 [details] [diff] [review]
Experimental ":system" string appended to event names when they are not from user input

Don't have my Linux box in front of me, but Scott if you get to it first, feel free to try and see if this breaks anything.
Comment 8 Aaron Leventhal 2007-09-06 18:17:27 PDT
I'm wondering I should put 2 colons "::system" instead of 1.

It seems that the bridge translates 2 colons to 1 and changes _ to - for some odd reason.
Comment 9 Willie Walker 2007-09-07 05:58:42 PDT
The appending of ":system" seems to violate the "class:type:subtype" form of the spec:

http://www.gnome.org/~billh/at-spi-new-idl/html/html/structAccessibility_1_1Event.html#o0

"The string can be interpreted as class:type:subtype For instance ¨object:text-changed:insert¨ is an event from the 'Object' class, which corresponds to Accessible objects general, the type of the event is a ¨text-changed¨ event (i.e. a change in the content of an implementor of the Text interface), and the specific subtype of the change is an insertion event."

BUT....

While the event.any_data field could indeed be used to handle the delivery of additional event information, I think its current use for handling existing events (e.g., the any_data holds the deleted/inserted text for text-changed events) makes it difficult to change.  That is, if Gecko decided to give a completely different any_data structure for a text-changed event, it would cause problems in assistive technologies that expect it to be the way it currently is.

Given the any_data concerns, appending information to the event type seems like a very interesting proposal.  It could also be used to provide us with other important event information that cannot gracefully or compatibly handled by the current implementation. 

The proposal seems like it should work given the general "starts with" pattern of event types.  That is, registering for "object:text-changed" events will get the assistive technology "object:text-changed:delete", "object:text-changed:insert" events, and should also get the assistive technology events "object:text-changed:delete:system" events.  

In discussions with Li Yuan, the AT-SPI implementation doesn't seem to have a strict "class:type:subtype" assumption, so there doesn't appear to be anything to prevent this proposal from working.  If someone can build/test with this patch and see that it doesn't break the current Orca implementation, I'd be OK with seeing this done.

If others agree, I think we also need to consider relaxing the AT-SPI event spec to allow something like "class:type:subtype[:attributes], where "attributes" can be just about any random string.  Alternatively, we might consider tightening up the definition of "attributes" to be a comma separated list of name=value pairs, where the "=value" portion is optional.
Comment 10 Aaron Leventhal 2007-09-07 06:03:20 PDT
Created attachment 280051 [details] [diff] [review]
Same patch, but also allow container-foo object attributes on any node, not just last event node. Doesn't change the ":system" code.
Comment 11 Scott Haeger 2007-09-07 10:33:55 PDT
After patching the code I get the following error when rebuilding Firefox.  Is DoSignal() declared properly?  

nsAccessibleWrap.cpp:1203: error: no matching function for call to ‘nsAccessibleWrap::DoSignal(nsIAccessibleEvent*&, AtkObject*&, const char [17], PRInt32&)’
nsAccessibleWrap.h:128: note: candidates are: static void nsAccessibleWrap::DoSignal(nsIAccessibleEvent*, AtkObject*, char*)
nsAccessibleWrap.cpp:1224: error: no matching function for call to ‘nsAccessibleWrap::DoSignal(nsIAccessibleEvent*&, AtkObject*&, const char [13], PRUint32&, PRUint32&)’
nsAccessibleWrap.h:128: note: candidates are: static void nsAccessibleWrap::DoSignal(nsIAccessibleEvent*, AtkObject*, char*)
nsAccessibleWrap.cpp:1240: error: no matching function for call to ‘nsAccessibleWrap::DoSignal(nsIAccessibleEvent*&, AtkObject*&, const char [12], PRUint32&, PRUint32&)’
nsAccessibleWrap.h:128: note: candidates are: static void nsAccessibleWrap::DoSignal(nsIAccessibleEvent*, AtkObject*, char*)
nsAccessibleWrap.cpp:1261: error: no matching function for call to ‘nsAccessibleWrap::DoSignal(nsIAccessibleEvent*&, AtkObject*&, const char [16], PRUint32&, PRUint32&)’
nsAccessibleWrap.h:128: note: candidates are: static void nsAccessibleWrap::DoSignal(nsIAccessibleEvent*, AtkObject*, char*)
nsAccessibleWrap.cpp:1277: error: no matching function for call to ‘nsAccessibleWrap::DoSignal(nsIAccessibleEvent*&, AtkObject*&, const char [15], PRUint32&, PRUint32&)’
nsAccessibleWrap.h:128: note: candidates are: static void nsAccessibleWrap::DoSignal(nsIAccessibleEvent*, AtkObject*, char*)
nsAccessibleWrap.cpp:1296: error: no matching function for call to ‘nsAccessibleWrap::DoSignal(nsIAccessibleEvent*&, AtkObject*&, const char [14], int&)’
nsAccessibleWrap.h:128: note: candidates are: static void nsAccessibleWrap::DoSignal(nsIAccessibleEvent*, AtkObject*, char*)
nsAccessibleWrap.cpp: In static member function ‘static void nsAccessibleWrap::DoSignal(nsIAccessibleEvent*, AtkObject*, char*)’:
nsAccessibleWrap.cpp:1392: error: invalid conversion from ‘void*’ to ‘char*’
nsAccessibleWrap.cpp: In member function ‘nsresult nsAccessibleWrap::FireAtkPropChangedEvent(nsIAccessibleEvent*, AtkObject*)’:
nsAccessibleWrap.cpp:1535: error: no matching function for call to ‘nsAccessibleWrap::DoSignal(nsIAccessibleEvent*&, AtkObject*&, char*&, AtkPropertyValues*, NULL)’
nsAccessibleWrap.cpp:1383: note: candidates are: static void nsAccessibleWrap::DoSignal(nsIAccessibleEvent*, AtkObject*, char*)
nsAccessibleWrap.cpp: In member function ‘nsresult nsAccessibleWrap::FireAtkShowHideEvent(nsIAccessibleEvent*, AtkObject*, PRBool)’:
nsAccessibleWrap.cpp:1570: error: no matching function for call to ‘nsAccessibleWrap::DoSignal(nsIAccessibleEvent*&, AtkObject*&, const char*, PRInt32&, AtkObject*&, NULL)’
nsAccessibleWrap.cpp:1383: note: candidates are: static void nsAccessibleWrap::DoSignal(nsIAccessibleEvent*, AtkObject*, char*)
gmake[6]: *** [nsAccessibleWrap.o] Error 1
Comment 12 Aaron Leventhal 2007-09-07 13:17:22 PDT
Created attachment 280097 [details] [diff] [review]
Builds properly. Also removes a warning and unused link-selected code
Comment 13 Aaron Leventhal 2007-09-07 14:23:50 PDT
Created attachment 280106 [details] [diff] [review]
Fix for text-changed::inserted/removed events, but Accerciser doesn't see "::system"
Comment 14 Willie Walker 2007-09-07 14:31:45 PDT
(In reply to comment #13)
> Created an attachment (id=280106) [details]
> Fix for text-changed::inserted/removed events, but Accerciser doesn't see
> "::system"
> 

Accerciser might be making some assumptions about the event type string -- I definitely know there's assumptions being made in pyatspi's event.py module.  You might try at-poke for this particular test since I don't think it tries to interpret the event type.
Comment 15 Aaron Leventhal 2007-09-07 14:33:51 PDT
I think it's pyatspi. 

Accerciser doesn't report the new events at all, and in the Accerciser console log I get these pyatspi errors:

Traceback (most recent call last):
  File "/usr/local/lib/python2.5/site-packages/accerciser/pyatspi.zip/pyatspi/registry.py", line 273, in notifyEvent
  File "/usr/local/lib/python2.5/site-packages/accerciser/pyatspi.zip/pyatspi/event.py", line 137, in __init__
  File "/usr/local/lib/python2.5/site-packages/accerciser/pyatspi.zip/pyatspi/event.py", line 226, in __init__
IndexError: tuple index out of range

I'll try ATK poke, good idea.
Comment 16 Aaron Leventhal 2007-09-07 14:36:46 PDT
Yes, at-poke sees the events! Brilliant!
Comment 17 Aaron Leventhal 2007-09-07 14:40:22 PDT
If I uncheck "text-changed" events in AT-Poke, I no longer see text-changed::insert::system or text-changed::delete::system.

That's a good sign.
Comment 18 Aaron Leventhal 2007-09-07 16:27:03 PDT
Created attachment 280128 [details] [diff] [review]
Minimize risk: only affect the events we really need to know this for -- children-changed and text-changed events

I played with the previous patch quite a bit, and found the following:
- children-changed::system and text-changed::system events are coming into at-poke just fine, along with their event data
- text-caret-move::system moves were not being recognized. I also got an gsignal.c error in the console when I fired with "::system". Removing ::system from these events made them work again
- there is no easy way to provide ::system for focus or state change events, because they are fired with atk_focus_tracker_notify and atk_object_notify_state_change(

I originally knew that for live region support, that the info of system vs. user is most important for text-changed and children-changed events. I was planning to provide the info carte blanche for all events, since we can get it. However, since that won't work I'd like to do the least risky thing and just provide it for the 2  kinds of events we really need, where it's clearly working.
Comment 19 Aaron Leventhal 2007-09-07 16:45:35 PDT
(In reply to comment #6)
> Or, maybe we can call spi_atk_emit_eventv() directly.

FWIW that event is static and can't be accessed externally.
Comment 20 Willie Walker 2007-09-07 17:19:49 PDT
(In reply to comment #15)
> I think it's pyatspi. 

:-(  I can advise that pyatspi avoid doing clever things for the dumb assistive technology programmer, but I don't always win.  IMO, cleverness usually comes back to bite you.

BTW, I'm very confused by the double colons in your descriptions.  I'm only used to seeing single colons in the events we get on Orca. 
Comment 21 Aaron Leventhal 2007-09-07 19:26:10 PDT
Does it come out with double colons in Orca?

All I know is that we fire things with underscores and double colons, and something somewhere translates the underscores to hyphens. I see some of the code in the bridge that changes the :: to :

I have no idea why that was done -- typical uncommented B.S. in the bridge IMO. What can I say?

Anyway, if there's a problem with :: vs. : we can easily change that. Let's see what Scott finds out looking at the events in Orca with this patch.
Comment 22 Aaron Leventhal 2007-09-07 21:01:11 PDT
Comment on attachment 280128 [details] [diff] [review]
Minimize risk: only affect the events we really need to know this for -- children-changed and text-changed events

I think Ginn may still be on sick leave. Whoever gets to the review first ... :)
Comment 23 Willie Walker 2007-09-08 02:38:56 PDT
(In reply to comment #15)
> I think it's pyatspi. 

I went digging to create a patch for pyatspi, and I discovered a 'detail' field has been added to it already.  So, if you're up to it, you might consider updating your accerciser and your at-spi implementation.  To build both, you need to use autogen then make and make install.  The autogen for at-spi is very particular:

at-spi: ./autogen.sh --prefix=/usr --libexecdir=/usr/lib/at-spi

You can get the sources here:

svn co http://svn.gnome.org/svn/at-spi/trunk at-spi
svn co http://svn.gnome.org/svn/accerciser/trunk accerciser

(In reply to comment #21)
> Does it come out with double colons in Orca?

They come to Orca with double colons, and when I look at events in at-poke, I don't see double colons.  So, the following from comment #17 confused me:

> If I uncheck "text-changed" events in AT-Poke, I no longer see
> text-changed::insert::system or text-changed::delete::system.

With your changes, do things comes to at-poke with double colons?  If so, this seems wrong.
Comment 24 Scott Haeger 2007-09-08 09:21:07 PDT
Some observations of Real v Nicks http://accessibleajax.clcworld.net/scoreboard/scoreboard.htm 

1)  The clock accessible is giving the following event type and attributes.  They appear correct to me. 

object:text-changed:insert::system
['container-live:off', 'live:off', 'level:2', 'tag:h2', 'id:gameClock']

2) A stat accessible change gives the following event and attributes.  Also included are the attributes for event.source.parent.  Should the live:polite attribute also be included for event.source or are we using container-live:polite for this?

object:text-changed:insert::system
attr= ['container-live:polite', 'tag:td', 'id:gcRebounds']
parent attr= ['container-live:polite', 'live:polite', 'tag:tr']

I have yet to see a children-changed event on Real v Nicks.  Aaron, do you know of an example?  I will keep looking as time permits this weekend.
Comment 25 Aaron Leventhal 2007-09-08 12:28:08 PDT
If you use container-foo attributes you will see the final computed value of those attributes. That way you don't have to walk the parent chain and check foo on each one. That's written up here: http://developer.mozilla.org/en/docs/AJAX:WAI_ARIA_Live_Regions/API_Support
Talk to me on IRC if you have questions on that.

There are no children-changed events on real vs. nicks, only text change events. You can see children-changed events on the log in the chat example.
Comment 26 Aaron Leventhal 2007-09-08 12:28:39 PDT
Scott, what are you seeing for the system events inside Orca? Can you past the exact event names? I want to see if you're getting single or double colons.
Comment 27 Scott Haeger 2007-09-08 18:15:58 PDT
For live regions, Orca is seeing object:children-changed:remove::system and object:children-changed:add::system events on the chat example and object:text-changed:insert::system on Real v Nicks.  Non-live region events of this type look like they originally did.
Comment 28 Aaron Leventhal 2007-09-08 18:28:28 PDT
To clarify, adding "system" to the event name is not based on whether it's in a live region or not. It's based on whether the change was a result of some user input. Firefox keeps track of this information.

For example, in the chat example:
- if you type something and hit Enter, it's not a system event, because the user caused it from their own typing
- if the the log is updated with a message from another user, it's a "system" event -- it's a change that resulted from something other than what the user did.

Both changes actually occurred in the same live region, but Firefox is telling you which was caused by the user and which wasn't ("system").
Comment 29 Aaron Leventhal 2007-09-08 18:37:52 PDT
Created attachment 280218 [details] [diff] [review]
Same thing, but with single colon ":system"
Comment 30 Willie Walker 2007-09-09 07:16:58 PDT
Scott -- with the FF patch in place, do you notice different behavior from Orca?  That is, does the addition of the ":system" attribute to the event type cause Orca to not process these events?  I did a quick check in the Orca sources, and we have a bunch of equality vs. startswith checks for event types (stupid me :-():

atspi.py:            if self.type == "object:active-descendant-changed":
atspi.py:            elif self.type == "object:text-changed:insert":
atspi.py:            elif self.type == "object:text-changed:delete":
atspi.py:            if e.type == "object:state-changed:defunct":
default.py:            shouldNotInterrupt = (event and event.type == "focus:") \
default.py:        if event.type == "object:state-changed:active":
default.py:        if event.type == "object:state-changed:focused":
default.py:            if event.type == "object:state-changed:showing":
Gecko.py:        if event.type == "object:state-changed:checked" \
Gecko.py:        if event.type == "object:state-changed:showing" \
Gecko.py:        if event.type == "object:state-changed:busy":
....

So, the addition of the ":system" stuff may require us to modify code.  The concern here is that we need to make sure Orca 2.20 will work with FF3.  So, we may have to do a freeze break request to change these equality checks to startswith.
Comment 31 Scott Haeger 2007-09-09 09:55:34 PDT
Will,

Yes, there are minor fixes that will need to be done in Orca.  I have seen the equalities that you mentioned and they were easily fixed.  I can make the required changes and have it ready first thing in the morning for testing.  I will file an Orca bug right away.
Comment 32 Willie Walker 2007-09-09 11:07:40 PDT
Thanks Scott!  For the Orca fixes, I think we'll need to shoot for GNOME 2.20.1 instead of GNOME 2.20.0.  I expect there to be a number of them, and I'm hesitant to make so many changes post GNOME 2.20.0 code freeze (today).  This is OK, though.
Comment 33 Aaron Leventhal 2007-09-09 18:13:52 PDT
Will, and at least for now, we're only doing this for children-changed and text-changed events, for the reasons described in comment 18.
Comment 34 Scott Haeger 2007-09-10 07:45:25 PDT
Overall the patch looks good.  The only thing that concerns me are accessibles that have both container-<attr> and the attr.  For instance, the clock on Real v Nicks is marked up with live:off, but also has the attribute container-live:off.  This seems like repeated information.
Comment 35 Aaron Leventhal 2007-09-10 08:43:50 PDT
It's not really repeated, because:
"live" is showing you that the live attribute was actually define on that node
"container-live" is all you need to check. If we didn't provide that you'd need to check it first, then the other.

Many times you will just see "container-live" and you would have to walk up the parent chain to see which defines it.
Comment 36 Aaron Leventhal 2007-09-10 11:17:14 PDT
Created attachment 280371 [details] [diff] [review]
1) Also fix member-of relation for atomic regions, 2) Move code for container attributes to more appropriate places
Comment 37 Scott Haeger 2007-09-10 13:06:06 PDT
I am not seeing any live region related relations.  I would expect to see the following relations:
labelledby: http://accessibleajax.clcworld.net/simple/labelledby.htm
describedby: http://accessibleajax.clcworld.net/simple/describedby.htm
memberof: http://accessibleajax.clcworld.net/simple/atomic_true.htm

I also remember seeing labelledby relations for Real v Nicks, but there is no aria markup.  Is the XHTML attribute 'header' responsible for setting the relation?

The memberof relation was addressed in this bug.  A separate bug will be opened about the other relation problems ( see #395699 ).
Comment 38 Evan Yan 2007-09-10 19:59:05 PDT
Comment on attachment 280371 [details] [diff] [review]
1) Also fix member-of relation for atomic regions, 2) Move code for container attributes to more appropriate places

canceling review request to me since ginn has already r+.
Comment 39 alexander :surkov 2007-09-10 20:01:31 PDT
Possibly it would be better to introduce atoms for attributes and use nsAccUtils::SetAccAttr()
Comment 40 Aaron Leventhal 2007-09-11 06:54:32 PDT
Created attachment 280456 [details] [diff] [review]
Same thing, but add MEMBER_OF to list of supported relations in ATK
Comment 41 Aaron Leventhal 2007-09-15 13:08:28 PDT
Actually I can fix this by exposing all objects with an ID. Peter Parente had asked for that in bug 371156.
Comment 42 Aaron Leventhal 2007-09-15 13:09:05 PDT
Please ignore comment 41, that was for a different bug.

Note You need to log in before you can comment on or make changes to this bug.