Programmatically-inserted children lack accessibility events

• Component: Accessibility
• Status: RESOLVED FIXED

[Joanmarie Diggs]

Created attachment 170441 [details]
event listener

Steps to reproduce:

1. Sign in to webchat.freenode.net (or use empathy version 3.6)
2. Launch the attached event listener in a terminal
3. Type some text and pres Return to send it to a channel, conference room, etc.

Expected result: At step 3, children-changed:add events with the would be seen.
Actual result: At step 3, no children-changed:add events are seen.

[Mario Sanchez Prada]

Created attachment 218174 [details]
Patch proposal

Attaching a patch that fix this issue by:

 1. Making sure that the accessibility subtree under a given element internally receiving AXChildrenChanged is regenerated
 2. Emitting children-changed::remove at the moment an AtkObject is about to be detached BUT not because of the AXObjectCache being destroyed
 3. Emitting children-changed::add at the moment a new AtkObject is being attached, right after creation, unless we are in the middle of a layout update.

[Build Bot]

Comment on attachment 218174 [details]
Patch proposal

Attachment 218174 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/42138029

New failing tests:
accessibility/children-changed-sends-notification.html
accessibility/loading-iframe-sends-notification.html

[Build Bot]

Created attachment 218181 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 218174 [details]
Patch proposal

Attachment 218174 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/39458126

New failing tests:
accessibility/children-changed-sends-notification.html
accessibility/loading-iframe-sends-notification.html

[Build Bot]

Created attachment 218183 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 218174 [details]
Patch proposal

Attachment 218174 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/42218003

New failing tests:
accessibility/children-changed-sends-notification.html
accessibility/loading-iframe-sends-notification.html

[Build Bot]

Created attachment 218194 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Mario Sanchez Prada]

Created attachment 218292 [details]
Patch proposal

Let's try to make the mac bots happier

[EFL EWS Bot]

Comment on attachment 218292 [details]
Patch proposal

Attachment 218292 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/42738017

[Mario Sanchez Prada]

(In reply to comment #9)
> (From update of attachment 218292 [details])
> Attachment 218292 [details] did not pass efl-wk2-ews (efl-wk2):
> Output: http://webkit-queues.appspot.com/results/42738017

This looks to me like an error unrelated to this patch

[chris fleizach]

Comment on attachment 218292 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=218292&action=review

> LayoutTests/accessibility/children-changed-sends-notification.html:16
> +description("This test sure that a notification is being emitted when children are added or removed for an accessibility object");

sure -> ensures

> Source/WebCore/accessibility/AXObjectCache.h:233
> +    bool m_destructionInProgress;

instead of a global AXObjectCache flag, maybe we should have a parameter for detach() that informs us which kind of detach it is. DocumentDestroyed, ElementDestroyed (are there others?)

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:41
> +    AtkObject* wrapper = obj->wrapper();

you might want to ASSERT(wrapper) here

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:51
> +            // since the accessibility hierarchy in WebCore won't be longer navigable now.

'in WebCore will no longer be navigable'

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:195
> +            GRefPtr<AtkObject> child(atk_object_ref_accessible_child(axObject, i));

Does this make a AtkObject with the WebCore object?
If so, where does it get cached?

[Mario Sanchez Prada]

(In reply to comment #11)
> (From update of attachment 218292 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218292&action=review
> 
> > LayoutTests/accessibility/children-changed-sends-notification.html:16
> > +description("This test sure that a notification is being emitted when children are added or removed for an accessibility object");
> 
> sure -> ensures

Ok.

> > Source/WebCore/accessibility/AXObjectCache.h:233
> > +    bool m_destructionInProgress;
> 
> instead of a global AXObjectCache flag, maybe we should have a parameter for 
> detach() that informs us which kind of detach it is. DocumentDestroyed, 
> ElementDestroyed (are there others?)
> 
Ok. That was actually my first approach to the problem, but in the end I did this just to keep the patch smaller.

But I can change it back anyway. Thanks for the suggestion.

> > Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:41
> > +    AtkObject* wrapper = obj->wrapper();
> 
> you might want to ASSERT(wrapper) here
> 
Makes sense.

> > Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:51
> > +            // since the accessibility hierarchy in WebCore won't be longer navigable now.
> 
> 'in WebCore will no longer be navigable'
> 
Ok

> > Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:195
> > +            GRefPtr<AtkObject> child(atk_object_ref_accessible_child(axObject, i));
> 
> Does this make a AtkObject with the WebCore object?
> If so, where does it get cached?

This call creates and uses the childrenVector of WebCore objects from the object wrapped by axObject to create and return a child AtkObject wrapping an already existing WebCore object.

So, the returned AtkObject will always wrap a WebCore object that has been previously added to the AXObjectCache.

[Mario Sanchez Prada]

Created attachment 218589 [details]
Patch proposal

Attaching new patch addressing those comments. Also, I made the most to convert that ugly if in postPlatformNotification for ATK into a switch statement.

[chris fleizach]

Comment on attachment 218589 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=218589&action=review

looks pretty much ok, just a few minor things

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:45
> +        bomb->updateBackingStore();

i assume you don't want to add this

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:89
> +    size_t index = coreParent->children().find(obj);

you should probably check that index != WTF::notFOund before proceeding and possible assert that condition

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:199
> +        for (int i = 0; i < atk_object_get_n_accessible_children(axObject); ++i)

this should be in a var so we don't query each iteration through the run loop
atk_object_get_n_accessible_children(axObject)

[Mario Sanchez Prada]

Comment on attachment 218589 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=218589&action=review

>> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:45
>> +        bomb->updateBackingStore();
> 
> i assume you don't want to add this

Oops! I certainly don't want to add that. Just a quick check with a release build :)

>> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:89
>> +    size_t index = coreParent->children().find(obj);
> 
> you should probably check that index != WTF::notFOund before proceeding and possible assert that condition

The problem with that is that I've observed that sometimes we might get a -1 here (meaning that the object has not been added yet to the children vector of the parent) and in those cases, even if not ideal, we would still like to notify to ATs that the parent AtkObject (which is accessible by navigating the hierarchy anyway) has gained a new children (-1 is a valid value to be emitted along with this signal, as per ATK documentation, when you can't determine the offset at the time of emission).

So, even if I recognise is not a very clean behavior, I think it's better to emit the signal anyway as long as we have an AtkObject for the child and for the parent, even if we can't properly determine the offset sometimes. Perhaps this is something we could try to improve later anyway.

>> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:199
>> +        for (int i = 0; i < atk_object_get_n_accessible_children(axObject); ++i)
> 
> this should be in a var so we don't query each iteration through the run loop
> atk_object_get_n_accessible_children(axObject)

Ok.

[Mario Sanchez Prada]

Created attachment 218597 [details]
Patch proposal

New patch

[chris fleizach]

Comment on attachment 218597 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=218597&action=review

> LayoutTests/accessibility/children-changed-sends-notification.html:33
> +        testRunner.waitUntilDone();

technically i don't think this is needed if you use jsTestIsAsync = true

[Mario Sanchez Prada]

Comment on attachment 218597 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=218597&action=review

>> LayoutTests/accessibility/children-changed-sends-notification.html:33
>> +        testRunner.waitUntilDone();
> 
> technically i don't think this is needed if you use jsTestIsAsync = true

You are right, that's a leftover of the test I based this one in. I'll remove it before landing.

Thanks

[Mario Sanchez Prada]

Created attachment 218683 [details]
Patch proposal

Chris, would you mind partly reviewing the patch again before I landed it?

The reason for that is that I'm now in the WebKitGTK+ Hackfest[1] and both Joanie and me realized it would be a good idea to add an extra check before emitting the children-changed::add signal, to avoid getting notifications about objects that we can already (and quickly) now in advance won't get exposed.

The only difference with the previous patch is that I added this check in attachWrapper():

+    // Don't emit the signal for objects that we already know won't be exposed
+    // directly for ATK. we don't want to use accessibilityObjectIsIgnoredByDefault()
+    // or other related methods (e.g. accessibilityIsIgnored()) here because those might
+    // trigger the execution of code that might be dangerous or just computationally
+    // excessive at this stage (creating new objects by traversing the accessibility tree).
+    AccessibilityObject* coreParent = obj->parentObjectUnignored();
+    if (!coreParent || coreParent->accessibilityPlatformIncludesObject() == IgnoreObject)
+        return;
+

This addition alone prevents WebKitGTK+ from emitting a whole lot of signals about irrelevant objects in the webchat use case, leaving only those ones that matter, so I think it's a thing we should definitely add.

The other difference with the previous patch is that I removed the waitUntilDone() bit in the layout test.

Thanks, and sorry for bothering you one more time with this.

[1] https://wiki.gnome.org/Hackfests/WebKitGTK2013

[chris fleizach]

(In reply to comment #19)
> Created an attachment (id=218683) [details]
> Patch proposal
> 
> Chris, would you mind partly reviewing the patch again before I landed it?
> 
> The reason for that is that I'm now in the WebKitGTK+ Hackfest[1] and both Joanie and me realized it would be a good idea to add an extra check before emitting the children-changed::add signal, to avoid getting notifications about objects that we can already (and quickly) now in advance won't get exposed.
> 
> The only difference with the previous patch is that I added this check in attachWrapper():
> 
> +    // Don't emit the signal for objects that we already know won't be exposed
> +    // directly for ATK. we don't want to use accessibilityObjectIsIgnoredByDefault()
> +    // or other related methods (e.g. accessibilityIsIgnored()) here because those might
> +    // trigger the execution of code that might be dangerous or just computationally
> +    // excessive at this stage (creating new objects by traversing the accessibility tree).
> +    AccessibilityObject* coreParent = obj->parentObjectUnignored();
> +    if (!coreParent || coreParent->accessibilityPlatformIncludesObject() == IgnoreObject)
> +        return;
> +

By calling
 obj->parentObjectUnignored();
you still may end up calling accessibilityIsIgnored(). Since this is happening in attachWrapper, it seems OK because I don't think we create AX objects until they're requested (but i could be wrong)

> 
> This addition alone prevents WebKitGTK+ from emitting a whole lot of signals about irrelevant objects in the webchat use case, leaving only those ones that matter, so I think it's a thing we should definitely add.
> 
> The other difference with the previous patch is that I removed the waitUntilDone() bit in the layout test.
> 
> Thanks, and sorry for bothering you one more time with this.
> 
> [1] https://wiki.gnome.org/Hackfests/WebKitGTK2013

[Mario Sanchez Prada]

(In reply to comment #20)
> (In reply to comment #19)
> > Created an attachment (id=218683) [details] [details]
> > Patch proposal
> > 
> > Chris, would you mind partly reviewing the patch again before I landed it?
> > 
> > The reason for that is that I'm now in the WebKitGTK+ Hackfest[1] and both Joanie and me realized it would be a good idea to add an extra check before emitting the children-changed::add signal, to avoid getting notifications about objects that we can already (and quickly) now in advance won't get exposed.
> > 
> > The only difference with the previous patch is that I added this check in attachWrapper():
> > 
> > +    // Don't emit the signal for objects that we already know won't be exposed
> > +    // directly for ATK. we don't want to use accessibilityObjectIsIgnoredByDefault()
> > +    // or other related methods (e.g. accessibilityIsIgnored()) here because those might
> > +    // trigger the execution of code that might be dangerous or just computationally
> > +    // excessive at this stage (creating new objects by traversing the accessibility tree).
> > +    AccessibilityObject* coreParent = obj->parentObjectUnignored();
> > +    if (!coreParent || coreParent->accessibilityPlatformIncludesObject() == IgnoreObject)
> > +        return;
> > +
> 
> By calling
>  obj->parentObjectUnignored();
> you still may end up calling accessibilityIsIgnored(). Since this is happening in attachWrapper,
> it seems OK because I don't think we create AX objects until they're requested (but i could be wrong)
>

That's right, and so the comment is definitely wrong. Perhaps there's no need to restrict it to  accessibilityPlatformIncludesObject() and I can just use accessibilityIsIgnored() directly since, as you pointed it out, it might get called anyway in parentObjectUnignored. 


I'll try that and come up with a new atch asap. Thanks for the review

[Mario Sanchez Prada]

Created attachment 218843 [details]
Patch proposal

(In reply to comment #21)
> [...]
> > By calling
> >  obj->parentObjectUnignored();
> > you still may end up calling accessibilityIsIgnored(). Since this is happening in attachWrapper,
> > it seems OK because I don't think we create AX objects until they're requested (but i could be wrong)
> >
> 
> That's right, and so the comment is definitely wrong. Perhaps there's no need to restrict it to  accessibilityPlatformIncludesObject() and I can just use accessibilityIsIgnored() directly since, as you pointed it out, it might get called anyway in parentObjectUnignored. 
> 
> 
> I'll try that and come up with a new atch asap. Thanks for the review

I've tested that suggestion and it works fine indeed both in WK1 and WK2 for debug and release builds, so I'm updating the patch to use accessibilityIsIgnoredByDefault() instead.

Again, thanks for pointing that out.

[Build Bot]

Comment on attachment 218843 [details]
Patch proposal

Attachment 218843 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/46598055

[chris fleizach]

Comment on attachment 218843 [details]
Patch proposal

is the windows build failure real?

[Mario Sanchez Prada]

(In reply to comment #24)
> (From update of attachment 218843 [details])
> is the windows build failure real?

Is the same patch than before only with that one-line modification, so I really doubt it so :)

[Mario Sanchez Prada]

Comment on attachment 218843 [details]
Patch proposal

Let's try this then

[WebKit Commit Bot]

Comment on attachment 218843 [details]
Patch proposal

Clearing flags on attachment: 218843

Committed r160417: <http://trac.webkit.org/changeset/160417>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Gustavo Noronha (kov)]

A bisect points to this change as the cause to bug 125624

[WebKit Commit Bot]

Re-opened since this is blocked by bug 125629

[Mario Sanchez Prada]

Created attachment 219535 [details]
Patch proposal

New attempt. This time I added an optional parameter to AccessibilityObject::children() so we can ask it to return jsut m_children as it is, without updating it if needed, which is what was causing the trouble with the inspector reported in bug 125624.

This of course can mean that sometimes we won't get the actual offset, but that should be enough anyway since the offset is an optional thing for the children-changed::add signal (so it's not a huge problem if it's not 100% accurate). The really important thing is the AtkObject that is being added, and that should be correct

Please review

[Mario Sanchez Prada]

(In reply to comment #31)
> [...]
> Please review

As much as I hate asking for reviews explicitly, I'll be off for two weeks after this weekend, so it would be awesome if we could give this a try during tomorrow :)

Thanks in advance!

[chris fleizach]

Comment on attachment 219535 [details]
Patch proposal

can you update the patch with the latest DetachmentType code I put in already

[chris fleizach]

Comment on attachment 219535 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=219535&action=review

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:85
> +    AtkObject* atkParent = coreParent ? coreParent->wrapper() : 0;

you already checked for coreParent == nil above so you don't need to check again

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:199
> +        if (int numOfChildren = atk_object_get_n_accessible_children(axObject)) {

i don't think you need this if statement. the for loop will take care of not doing any work for you

[Mario Sanchez Prada]

Comment on attachment 219535 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=219535&action=review

(In reply to comment #33)
> (From update of attachment 219535 [details])
> can you update the patch with the latest DetachmentType code I put in already

Sure

>> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:85
>> +    AtkObject* atkParent = coreParent ? coreParent->wrapper() : 0;
> 
> you already checked for coreParent == nil above so you don't need to check again

Right

>> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:199
>> +        if (int numOfChildren = atk_object_get_n_accessible_children(axObject)) {
> 
> i don't think you need this if statement. the for loop will take care of not doing any work for you

You are right. Actually I think I just did it to avoid declaring the numOfChildren variable outside the switch statement, or using an anonymous { } block just to bypass the limitation of switch statements, which do not allow declaring new variables in case sections :)

So, I would keep it anyway, if you don't mind

[Mario Sanchez Prada]

Created attachment 219657 [details]
Patch proposal

Attaching a new patch now, using the newly added AccessibilityDetachmentType from AccessibilityObject and removing the unneeded check for coreParent.

[chris fleizach]

Comment on attachment 219535 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=219535&action=review

>>> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:199
>>> +        if (int numOfChildren = atk_object_get_n_accessible_children(axObject)) {
>> 
>> i don't think you need this if statement. the for loop will take care of not doing any work for you
> 
> You are right. Actually I think I just did it to avoid declaring the numOfChildren variable outside the switch statement, or using an anonymous { } block just to bypass the limitation of switch statements, which do not allow declaring new variables in case sections :)
> 
> So, I would keep it anyway, if you don't mind

you should be able to do
case AXChildren:
{
    ...
}

[EFL EWS Bot]

Comment on attachment 219657 [details]
Patch proposal

Attachment 219657 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/46688048

[Mario Sanchez Prada]

Committed r160903: <http://trac.webkit.org/changeset/160903>