Closed Bug 392061 Opened 17 years ago Closed 17 years ago

Live region object attributes are not being persisted for AT-SPI

Categories

(Core :: Disability Access APIs, defect)

x86
Linux
defect
Not set
blocker

Tracking

()

RESOLVED FIXED

People

(Reporter: scott, Assigned: aaronlev)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(2 files, 6 obsolete files)

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.
Severity: normal → blocker
Keywords: access
Is there a way to squeeze this information into ATK/AT-SPI event data?
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.
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
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.
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.
Or, maybe we can call spi_atk_emit_eventv() directly.
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.
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.
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.
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
Attachment #280051 - Attachment is obsolete: true
Attachment #280002 - Attachment is obsolete: true
(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.
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.
Yes, at-poke sees the events! Brilliant!
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.
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.
Attachment #280106 - Attachment is obsolete: true
Attachment #280128 - Flags: review?(ginn.chen)
(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.
(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. 
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 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 ... :)
Attachment #280128 - Flags: review?(Evan.Yan)
(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.
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.
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.
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.
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.
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").
Attachment #280128 - Attachment is obsolete: true
Attachment #280218 - Flags: review?(ginn.chen)
Attachment #280128 - Flags: review?(ginn.chen)
Attachment #280128 - Flags: review?(Evan.Yan)
Attachment #280218 - Flags: review?(Evan.Yan)
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.
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.
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.
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.
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.
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.
Attachment #280218 - Attachment is obsolete: true
Attachment #280371 - Flags: review?(ginn.chen)
Attachment #280218 - Flags: review?(ginn.chen)
Attachment #280218 - Flags: review?(Evan.Yan)
Attachment #280371 - Flags: review?(Evan.Yan)
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 ).
Attachment #280371 - Flags: review?(ginn.chen) → review+
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+.
Attachment #280371 - Flags: review?(Evan.Yan)
Possibly it would be better to introduce atoms for attributes and use nsAccUtils::SetAccAttr()
Attachment #280371 - Flags: approval1.9?
Actually I can fix this by exposing all objects with an ID. Peter Parente had asked for that in bug 371156.
Please ignore comment 41, that was for a different bug.
Attachment #280371 - Flags: approval1.9? → approval1.9+
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: