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

RESOLVED FIXED

Status

()

Core
Disability Access APIs
--
blocker
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Scott Haeger, Assigned: Aaron Leventhal)

Tracking

(Blocks: 1 bug, {access})

Trunk
x86
Linux
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Assignee)

Updated

10 years ago
Severity: normal → blocker
Keywords: access
(Assignee)

Comment 1

10 years ago
Is there a way to squeeze this information into ATK/AT-SPI event data?
(Assignee)

Comment 2

10 years ago
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.
(Reporter)

Comment 3

10 years ago
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
(Assignee)

Comment 4

10 years ago
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.
(Assignee)

Comment 5

10 years ago
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.
(Assignee)

Comment 6

10 years ago
Or, maybe we can call spi_atk_emit_eventv() directly.
(Assignee)

Comment 7

10 years ago
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.
(Assignee)

Comment 8

10 years ago
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

10 years ago
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.
(Assignee)

Comment 10

10 years ago
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.
(Reporter)

Comment 11

10 years ago
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
(Assignee)

Comment 12

10 years ago
Created attachment 280097 [details] [diff] [review]
Builds properly. Also removes a warning and unused link-selected code
(Assignee)

Updated

10 years ago
Attachment #280051 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #280002 - Attachment is obsolete: true
(Assignee)

Comment 13

10 years ago
Created attachment 280106 [details] [diff] [review]
Fix for text-changed::inserted/removed events, but Accerciser doesn't see "::system"
Attachment #280097 - Attachment is obsolete: true

Comment 14

10 years ago
(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.
(Assignee)

Comment 15

10 years ago
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.
(Assignee)

Comment 16

10 years ago
Yes, at-poke sees the events! Brilliant!
(Assignee)

Comment 17

10 years ago
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.
(Assignee)

Comment 18

10 years ago
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.
Attachment #280106 - Attachment is obsolete: true
Attachment #280128 - Flags: review?(ginn.chen)
(Assignee)

Comment 19

10 years ago
(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

10 years ago
(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. 
(Assignee)

Comment 21

10 years ago
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.
(Assignee)

Comment 22

10 years ago
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)

Comment 23

10 years ago
(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.
(Reporter)

Comment 24

10 years ago
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.
(Assignee)

Comment 25

10 years ago
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.
(Assignee)

Comment 26

10 years ago
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.
(Reporter)

Comment 27

10 years ago
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.
(Assignee)

Comment 28

10 years ago
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").
(Assignee)

Comment 29

10 years ago
Created attachment 280218 [details] [diff] [review]
Same thing, but with single colon ":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)
(Assignee)

Updated

10 years ago
Attachment #280218 - Flags: review?(Evan.Yan)

Comment 30

10 years ago
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.
(Reporter)

Comment 31

10 years ago
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

10 years ago
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.
(Assignee)

Comment 33

10 years ago
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.
(Reporter)

Comment 34

10 years ago
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.
(Assignee)

Comment 35

10 years ago
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.
(Assignee)

Comment 36

10 years ago
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
Attachment #280218 - Attachment is obsolete: true
Attachment #280371 - Flags: review?(ginn.chen)
Attachment #280218 - Flags: review?(ginn.chen)
Attachment #280218 - Flags: review?(Evan.Yan)
(Assignee)

Updated

10 years ago
Attachment #280371 - Flags: review?(Evan.Yan)
(Reporter)

Comment 37

10 years ago
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 ).

Updated

10 years ago
Attachment #280371 - Flags: review?(ginn.chen) → review+

Comment 38

10 years ago
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)

Comment 39

10 years ago
Possibly it would be better to introduce atoms for attributes and use nsAccUtils::SetAccAttr()
(Assignee)

Updated

10 years ago
Attachment #280371 - Flags: approval1.9?
(Assignee)

Comment 40

10 years ago
Created attachment 280456 [details] [diff] [review]
Same thing, but add MEMBER_OF to list of supported relations in ATK
(Assignee)

Comment 41

10 years ago
Actually I can fix this by exposing all objects with an ID. Peter Parente had asked for that in bug 371156.
(Assignee)

Comment 42

10 years ago
Please ignore comment 41, that was for a different bug.

Updated

10 years ago
Attachment #280371 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.