Bug 100275

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 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

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 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

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 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

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

Created attachment 218292 [details] Patch proposal Let's try to make the mac bots happier

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

(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 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?

(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.

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 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 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.

Created attachment 218597 [details] Patch proposal New patch

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 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

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

(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

(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

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 on attachment 218843 [details] Patch proposal Attachment 218843 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/46598055

Comment on attachment 218843 [details] Patch proposal is the windows build failure real?

(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 on attachment 218843 [details] Patch proposal Let's try this then

Comment on attachment 218843 [details] Patch proposal Clearing flags on attachment: 218843 Committed r160417: <http://trac.webkit.org/changeset/160417>

All reviewed patches have been landed. Closing bug.

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

Re-opened since this is blocked by bug 125629

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

(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 on attachment 219535 [details] Patch proposal can you update the patch with the latest DetachmentType code I put in already

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 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

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 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 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

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