Bug 100275 - Programmatically-inserted children lack accessibility events
Summary: Programmatically-inserted children lack accessibility events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://webchat.freenode.net
Keywords:
Depends on: 125624 125629
Blocks: 25531
  Show dependency treegraph
 
Reported: 2012-10-24 11:34 PDT by Joanmarie Diggs (irc: joanie)
Modified: 2013-12-20 03:49 PST (History)
13 users (show)

See Also:


Attachments
event listener (368 bytes, text/plain)
2012-10-24 11:34 PDT, Joanmarie Diggs (irc: joanie)
no flags Details
Patch proposal (21.03 KB, patch)
2013-12-02 08:43 PST, Mario Sanchez Prada
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (501.86 KB, application/zip)
2013-12-02 10:17 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (453.80 KB, application/zip)
2013-12-02 10:36 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (516.54 KB, application/zip)
2013-12-02 11:17 PST, Build Bot
no flags Details
Patch proposal (22.35 KB, patch)
2013-12-03 06:35 PST, Mario Sanchez Prada
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Patch proposal (24.05 KB, patch)
2013-12-06 04:20 PST, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (24.01 KB, patch)
2013-12-06 08:33 PST, Mario Sanchez Prada
cfleizach: review+
Details | Formatted Diff | Diff
Patch proposal (27.38 KB, patch)
2013-12-08 04:52 PST, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (27.04 KB, patch)
2013-12-10 01:30 PST, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (25.94 KB, patch)
2013-12-18 07:51 PST, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (25.70 KB, patch)
2013-12-19 10:31 PST, Mario Sanchez Prada
cfleizach: review+
eflews.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs (irc: joanie) 2012-10-24 11:34:41 PDT
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.
Comment 1 Mario Sanchez Prada 2013-12-02 08:43:19 PST
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.
Comment 2 Build Bot 2013-12-02 10:17:31 PST
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
Comment 3 Build Bot 2013-12-02 10:17:33 PST
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
Comment 4 Build Bot 2013-12-02 10:35:59 PST
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
Comment 5 Build Bot 2013-12-02 10:36:02 PST
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
Comment 6 Build Bot 2013-12-02 11:17:21 PST
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
Comment 7 Build Bot 2013-12-02 11:17:24 PST
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
Comment 8 Mario Sanchez Prada 2013-12-03 06:35:05 PST
Created attachment 218292 [details]
Patch proposal

Let's try to make the mac bots happier
Comment 9 EFL EWS Bot 2013-12-04 03:05:46 PST
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
Comment 10 Mario Sanchez Prada 2013-12-04 03:38:20 PST
(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
Comment 11 chris fleizach 2013-12-05 09:00:07 PST
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?
Comment 12 Mario Sanchez Prada 2013-12-06 02:40:27 PST
(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.
Comment 13 Mario Sanchez Prada 2013-12-06 04:20:52 PST
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.
Comment 14 chris fleizach 2013-12-06 08:06:20 PST
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)
Comment 15 Mario Sanchez Prada 2013-12-06 08:29:05 PST
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.
Comment 16 Mario Sanchez Prada 2013-12-06 08:33:28 PST
Created attachment 218597 [details]
Patch proposal

New patch
Comment 17 chris fleizach 2013-12-06 08:45:08 PST
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
Comment 18 Mario Sanchez Prada 2013-12-06 08:53:42 PST
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
Comment 19 Mario Sanchez Prada 2013-12-08 04:52:08 PST
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
Comment 20 chris fleizach 2013-12-09 08:52:39 PST
(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
Comment 21 Mario Sanchez Prada 2013-12-09 10:45:28 PST
(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
Comment 22 Mario Sanchez Prada 2013-12-10 01:30:50 PST
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.
Comment 23 Build Bot 2013-12-10 02:14:42 PST
Comment on attachment 218843 [details]
Patch proposal

Attachment 218843 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/46598055
Comment 24 chris fleizach 2013-12-10 11:21:30 PST
Comment on attachment 218843 [details]
Patch proposal

is the windows build failure real?
Comment 25 Mario Sanchez Prada 2013-12-10 11:54:29 PST
(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 :)
Comment 26 Mario Sanchez Prada 2013-12-11 01:57:32 PST
Comment on attachment 218843 [details]
Patch proposal

Let's try this then
Comment 27 WebKit Commit Bot 2013-12-11 02:25:19 PST
Comment on attachment 218843 [details]
Patch proposal

Clearing flags on attachment: 218843

Committed r160417: <http://trac.webkit.org/changeset/160417>
Comment 28 WebKit Commit Bot 2013-12-11 02:25:23 PST
All reviewed patches have been landed.  Closing bug.
Comment 29 Gustavo Noronha (kov) 2013-12-12 03:13:05 PST
A bisect points to this change as the cause to bug 125624
Comment 30 WebKit Commit Bot 2013-12-12 03:58:21 PST
Re-opened since this is blocked by bug 125629
Comment 31 Mario Sanchez Prada 2013-12-18 07:51:12 PST
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
Comment 32 Mario Sanchez Prada 2013-12-19 09:57:21 PST
(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!
Comment 33 chris fleizach 2013-12-19 09:59:47 PST
Comment on attachment 219535 [details]
Patch proposal

can you update the patch with the latest DetachmentType code I put in already
Comment 34 chris fleizach 2013-12-19 10:02:20 PST
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
Comment 35 Mario Sanchez Prada 2013-12-19 10:15:23 PST
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
Comment 36 Mario Sanchez Prada 2013-12-19 10:31:09 PST
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.
Comment 37 chris fleizach 2013-12-19 10:38:41 PST
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:
{
    ...
}
Comment 38 EFL EWS Bot 2013-12-19 11:37:00 PST
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
Comment 39 Mario Sanchez Prada 2013-12-20 03:49:06 PST
Committed r160903: <http://trac.webkit.org/changeset/160903>