Bug 91794

Here's one test we should add along with this patch: * Create an element inside a canvas * Access its AccessibilityNodeObject * Reparent it to an element outside the canvas, so it has a renderer * Now access its AccessibilityRenderObject. * Make sure the AccessibilityNodeObject get freed and also deleted from the Node->Object hash map.

Created attachment 154563 [details] Patch

Comment on attachment 154563 [details] Patch Attachment 154563 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13346633

Comment on attachment 154563 [details] Patch Attachment 154563 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13344647

Comment on attachment 154563 [details] Patch Attachment 154563 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13349608

Comment on attachment 154563 [details] Patch Attachment 154563 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13343681

Created attachment 154655 [details] Fix Chromium port build

Comment on attachment 154655 [details] Fix Chromium port build Attachment 154655 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13353759

Comment on attachment 154655 [details] Fix Chromium port build Attachment 154655 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13343886

Created attachment 154661 [details] Fix Qt port build

Comment on attachment 154661 [details] Fix Qt port build Attachment 154661 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13341842 New failing tests: accessibility/canvas-fallback-content.html fast/events/tabindex-focus-blur-all.html

Created attachment 154677 [details] Archive of layout-test-results from gce-cr-linux-07 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-07 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 156877 [details] Patch

I have a doubt about this change: if we only emit notifications for nodes instead of renderers, aren't we missing notifications for those objects that have a representation only in the RenderObject tree but not in the DOM tree? (e.g. objects in Shadow DOM trees)

Comment on attachment 156877 [details] Patch This seems like it might cause us a step backwards in our ability to deal with generated content, which has render objects but not nodes. What did we do to prevent that?

Darin and Mario, To answer both of your questions, this change won't affect any code that sent an accessibility notification on a RenderObject that didn't have an associated Node. All I changed were lines of this form: From: axObjectCache()->postNotification(node->renderer(), AXValueChanged, true); To: axObjectCache()->postNotification(node, AXValueChanged, true); Similarly, inside accessibility/ I changed a lot of calls of this form: From: return axObjectCache()->getOrCreate(node->renderer()); To: return axObjectCache()->getOrCreate(node); In every case, the renderer was accessed via a node - all I did was make it call AXObjectCache using the node instead. The first thing AXObjectCache does is see if that node has a renderer - and if it does, it falls back on the previous behavior. However, if there's a node without a renderer (like in the canvas subtree), it can now send accessibility notifications. There's still some code that sends accessibility notifications on renderers that aren't associated with a node, and I didn't touch any of that code. I think it's unlikely that very much of that code will be needed in a canvas subtree. Please let me know if that answers your concerns!

(In reply to comment #16) > [...] > Please let me know if that answers your concerns! It does, at least for me (and I guess for Darin too). And yes, I agree there seems not to be any problem with this approach since it looks like everything should keep working as it used to do it before, according to your explanation. I'm now on a flaky connection and so not the best environment for working :-), but I'll try to save some time tomorrow to help with an informal review of the patch, if possible. Thanks for the clarification

Comment on attachment 156877 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156877&action=review Sorry for the delay. Crazy Friday here :-) I informally reviewed your patch and it looks good to me overall. I just have some really minor comments. See them inline below... > Source/WebCore/accessibility/AXObjectCache.cpp:314 > + AccessibilityObject* rendererObj = getOrCreate(node->renderer()); I think you should move this line down, right before the first place where rendererObj is used. Also, I think you could re-write this so the scope of nodeObj and rendererObj variables is the minimum required (just to return them if they are not null). Something like this: [...] if (AccessibilityObject* nodeObj = get(node)) return nodeObj; if (AccessibilityObject* rendererObj = getOrCreate(node->renderer())) return rendererObj; [...] > Source/WebCore/accessibility/AXObjectCache.cpp:531 > + object->contentChanged(); Nit. I would probably rewrite it as this: if (AccessibilityObject* object = getOrCreate(node)) object->contentChanged(); > Source/WebCore/accessibility/AXObjectCache.cpp:552 > + obj->childrenChanged(); Same (nit) here :) > Source/WebCore/accessibility/AXObjectCache.cpp:618 > + if (!node) Shouldn't we return here also if object is null? After all it's used in the call to postNotification. (I think the same issue would apply to the previous implementation of postNotification for RenderObject*) > Source/WebCore/accessibility/AXObjectCache.cpp:701 > obj->handleAriaExpandedChanged(); Same nit again. Maybe it's better something like this? if (AccessibilityObject* obj = getOrCreate(node)) obj->handleAriaExpandedChanged(); > Source/WebCore/accessibility/AXObjectCache.cpp:708 > obj->handleActiveDescendantChanged(); Ditto. > Source/WebCore/accessibility/AXObjectCache.cpp:715 > + static_cast<AccessibilityNodeObject*>(obj)->updateAccessibilityRole(); I think you should use toAccessibilityNodeObject(obj) instead of the static_cast here. > Source/WebCore/accessibility/AXObjectCache.cpp:782 > + if (nodeIsTextControl(const_cast<Element*>(element))) I think you should move this const_cast to nodeIsTextControl and leage this line as it was before? > Source/WebCore/accessibility/AXObjectCache.cpp:789 > +bool AXObjectCache::nodeIsTextControl(Node* node) I would leave this const here, as it looks like it makes sense > Source/WebCore/accessibility/AXObjectCache.cpp:794 > + const AccessibilityObject* axObject = getOrCreate(node); Considering the changes I proposed with regarding to const in the previous comments, I would use a const_cast<Node*>(node) here, to use getOrCreate() for the node > Source/WebCore/accessibility/AXObjectCache.h:127 > + bool nodeIsTextControl(Node*); If you accept my suggestion of the const cast, you'll need to update this too > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2649 > +bool AccessibilityRenderObject::nodeIsTextControl(Node* node) const Keep const here > Source/WebCore/accessibility/AccessibilityRenderObject.h:283 > + bool nodeIsTextControl(Node*) const; Ditto > Source/WebCore/bindings/cpp/WebDOMCustomVoidCallback.cpp:49 > +} I think this is a mistake in the patch? > Source/WebCore/dom/Node.cpp:1289 > + doc->axObjectCache()->attached(this); Why not using m_document directly? Also, I'm not 100% sure (I never am!), but I think m_document can never be null for a Node, so you might save one check here > LayoutTests/platform/gtk/TestExpectations:597 > +BUGWKGTK : accessibility/canvas-fallback-content.html = TEXT Question: I think the main reason for skipping this test is that the roles for GTK do not match those in the expected file (so I would need to generate later on specific expected results for GTK), right?

Created attachment 158227 [details] Patch

Comment on attachment 156877 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156877&action=review >> Source/WebCore/accessibility/AXObjectCache.cpp:314 >> + AccessibilityObject* rendererObj = getOrCreate(node->renderer()); > > I think you should move this line down, right before the first place where rendererObj is used. Also, I think you could re-write this so the scope of nodeObj and rendererObj variables is the minimum required (just to return them if they are not null). Something like this: > > [...] > if (AccessibilityObject* nodeObj = get(node)) > return nodeObj; > > if (AccessibilityObject* rendererObj = getOrCreate(node->renderer())) > return rendererObj; > [...] Good catch - actually it looks like the changes to this method aren't needed at all, let's leave it the way it was, which was cleaner than this. >> Source/WebCore/accessibility/AXObjectCache.cpp:531 >> + object->contentChanged(); > > Nit. I would probably rewrite it as this: > > if (AccessibilityObject* object = getOrCreate(node)) > object->contentChanged(); Sure, changed throughout. >> Source/WebCore/accessibility/AXObjectCache.cpp:618 >> + if (!node) > > Shouldn't we return here also if object is null? After all it's used in the call to postNotification. > (I think the same issue would apply to the previous implementation of postNotification for RenderObject*) No, that couldn't happen - if object is null, the while loop above wouldn't have terminated - unless node is null, in which case we return here. >> Source/WebCore/accessibility/AXObjectCache.cpp:715 >> + static_cast<AccessibilityNodeObject*>(obj)->updateAccessibilityRole(); > > I think you should use toAccessibilityNodeObject(obj) instead of the static_cast here. Good idea, done. >> Source/WebCore/accessibility/AXObjectCache.cpp:794 >> + const AccessibilityObject* axObject = getOrCreate(node); > > Considering the changes I proposed with regarding to const in the previous comments, I would use a const_cast<Node*>(node) here, to use getOrCreate() for the node Makes sense. >> Source/WebCore/bindings/cpp/WebDOMCustomVoidCallback.cpp:49 >> +} > > I think this is a mistake in the patch? Yes, thanks. >> Source/WebCore/dom/Node.cpp:1289 >> + doc->axObjectCache()->attached(this); > > Why not using m_document directly? > > Also, I'm not 100% sure (I never am!), but I think m_document can never be null for a Node, so you might save one check here I'm just copying the pattern above. :) I'm not 100% sure either so I'd rather keep this code safe rather than add an unsafe op to this large change. >> LayoutTests/platform/gtk/TestExpectations:597 >> +BUGWKGTK : accessibility/canvas-fallback-content.html = TEXT > > Question: I think the main reason for skipping this test is that the roles for GTK do not match those in the expected file (so I would need to generate later on specific expected results for GTK), right? That's correct. If the output is all PASS you can just generate an expectations file. There's a possibility there will be other differences, like we discovered with the last test I added. It's not easy to write a cross-platform accessibility test. :)

Comment on attachment 156877 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156877&action=review Thanks for the comments and for attaching a new patch, Dominic. I'll try to (informally) review the new one asap too. >>> Source/WebCore/accessibility/AXObjectCache.cpp:314 >>> + AccessibilityObject* rendererObj = getOrCreate(node->renderer()); >> >> I think you should move this line down, right before the first place where rendererObj is used. Also, I think you could re-write this so the scope of nodeObj and rendererObj variables is the minimum required (just to return them if they are not null). Something like this: >> >> [...] >> if (AccessibilityObject* nodeObj = get(node)) >> return nodeObj; >> >> if (AccessibilityObject* rendererObj = getOrCreate(node->renderer())) >> return rendererObj; >> [...] > > Good catch - actually it looks like the changes to this method aren't needed at all, let's leave it the way it was, which was cleaner than this. Great, even better :-) >>> Source/WebCore/accessibility/AXObjectCache.cpp:618 >>> + if (!node) >> >> Shouldn't we return here also if object is null? After all it's used in the call to postNotification. >> (I think the same issue would apply to the previous implementation of postNotification for RenderObject*) > > No, that couldn't happen - if object is null, the while loop above wouldn't have terminated - unless node is null, in which case we return here. Ah! It's true. Sorry for the noise >>> Source/WebCore/dom/Node.cpp:1289 >>> + doc->axObjectCache()->attached(this); >> >> Why not using m_document directly? >> >> Also, I'm not 100% sure (I never am!), but I think m_document can never be null for a Node, so you might save one check here > > I'm just copying the pattern above. :) I'm not 100% sure either so I'd rather keep this code safe rather than add an unsafe op to this large change. Hehe... I know that feeling very well, so I won't push you harder :-) I'm fine with you leaving that just-in-case extra check there. >>> LayoutTests/platform/gtk/TestExpectations:597 >>> +BUGWKGTK : accessibility/canvas-fallback-content.html = TEXT >> >> Question: I think the main reason for skipping this test is that the roles for GTK do not match those in the expected file (so I would need to generate later on specific expected results for GTK), right? > > That's correct. If the output is all PASS you can just generate an expectations file. > > There's a possibility there will be other differences, like we discovered with the last test I added. It's not easy to write a cross-platform accessibility test. :) Yes, it's not easy at all. The fact that role names are different in each platform (so things like shouldBe(axObject.role, 'Role: AXLink') won't work in mac and gtk at the same time) and also that the final a11y hierarchy exposed is not exactly the same (e.g. we "flatten"/"merge" some parts for GTK) makes it very difficult and one of the reasons why we have so many tests (and not just expectations) in platform/<platform>/accessibility. So I empathize with you on this too, and I'm fine with this bit in your patch

Comment on attachment 158227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158227&action=review This patch looks good to me, so I guess now it's time to ask for an official review over it (Chris?). See below a couple (just two!) of really minor issues for you to consider in the next iteration, or just when landing if you get the r+. > Source/WebCore/accessibility/AXObjectCache.cpp:315 > + This looks just like a mistake > Source/WebCore/accessibility/AXObjectCache.cpp:547 > + obj->childrenChanged(); Same suggestion (nit) than in my previous review: if (AccessibilityObject* obj = get(node)) obj->childrenChanged();

Comment on attachment 158227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158227&action=review >> Source/WebCore/accessibility/AXObjectCache.cpp:315 >> + > > This looks just like a mistake Fixed. >> Source/WebCore/accessibility/AXObjectCache.cpp:547 >> + obj->childrenChanged(); > > Same suggestion (nit) than in my previous review: > if (AccessibilityObject* obj = get(node)) > obj->childrenChanged(); Sorry, missed this one. Found a couple more to fix too.

Created attachment 158373 [details] Patch

Comment on attachment 158373 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158373&action=review one concern is the textMarker Node cache. Is that getting conflated with the other node cache Should we make the get and getOrCreate(RenderObject*) methods private now since we expect other callers to use the node methods? > Source/WebCore/accessibility/AXObjectCache.cpp:705 > + toAccessibilityNodeObject(obj)->updateAccessibilityRole(); This seems like it could be called on AccessibilityObject. There might be a mock element who wants to update its role. that could avoid this node check > Source/WebCore/dom/Node.cpp:417 > + doc->axObjectCache()->remove(this); we still need the removeNodeForUse() call because i think that's a different cache being used for Node being used in text markers. If the cache is the same, then we should remove the *NodeForUse methods which ensure that text markers are still valid > Source/WebCore/dom/Node.cpp:1289 > + doc->axObjectCache()->attached(this); attached it past tense, i think it should be attach() for present tense

Comment on attachment 158373 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158373&action=review > one concern is the textMarker Node cache. Is that getting conflated with the other node cache The text marker Node cache still exists, it's now just private to AXObjectCache - nodes get added to it in textMarkerDataForVisiblePosition(...), and removed in remove(Node*) - which is now called whenever removeNodeForUse(Node*) was called before. > Should we make the get and getOrCreate(RenderObject*) methods private now since we expect other callers to use the node methods? AccessibilityRenderObject still makes use of getOrCreate(RenderObject*) in quite a few places, and they look reasonable. We could make these methods private but make AccessibilityRenderObject a friend class? Or we could move those methods to the bottom of AXObjectCache with a comment that they shouldn't be used from outside the accessibility/ directory. >> Source/WebCore/accessibility/AXObjectCache.cpp:705 >> + toAccessibilityNodeObject(obj)->updateAccessibilityRole(); > > This seems like it could be called on AccessibilityObject. There might be a mock element who wants to update its role. that could avoid this node check Good idea. I made updateAccessibilityRole into a virtual method in AccessibilityObject. >> Source/WebCore/dom/Node.cpp:417 >> + doc->axObjectCache()->remove(this); > > we still need the removeNodeForUse() call because i think that's a different cache being used for Node being used in text markers. > If the cache is the same, then we should remove the *NodeForUse methods which ensure that text markers are still valid I just made remove(Node*) call removeNodeForUse. See above. >> Source/WebCore/dom/Node.cpp:1289 >> + doc->axObjectCache()->attached(this); > > attached it past tense, i think it should be attach() for present tense Done

Created attachment 158512 [details] Patch

Comment on attachment 158512 [details] Patch Attachment 158512 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13494880

Comment on attachment 158512 [details] Patch Attachment 158512 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13494884

Comment on attachment 158512 [details] Patch Attachment 158512 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13509083

Created attachment 158516 [details] Patch

Comment on attachment 158516 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158516&action=review i think everything else look ok. just the attach() method confuses me > Source/WebCore/accessibility/AXObjectCache.cpp:540 > + get(node); this comment is a bit confusing for me. if the node has been reparented it looks like it will be removed from the cache in get(), which would be the opposite of attach

Comment on attachment 158516 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158516&action=review >> Source/WebCore/accessibility/AXObjectCache.cpp:540 >> + get(node); > > this comment is a bit confusing for me. if the node has been reparented it looks like it will be removed from the cache in get(), which would be the opposite of attach The purpose of this method is to give a chance for AXObjectCache to update, after a node has been attached. I didn't mean to imply that AXObjectCache should be attaching the node - it's a notification that the node was attached, not a command to attach. The reason we do this on attach is that at the time the node is re-inserted into the tree, it's not clear at that point if it will have a renderer at that new location in the tree, because the style calculation hasn't happened yet. But after Node::attach(), the style calculation is done and the node will either have a renderer or not - so that's the best time for AXObjectCache to make sure that it doesn't have a stale AccessibilityNodeObject that's associated with a node that now has a renderer. I renamed the method updateCacheAfterNodeIsAttached(Node*), does that make more sense?

Created attachment 158622 [details] Patch

Comment on attachment 158622 [details] Patch r=me

Comment on attachment 158622 [details] Patch Clearing flags on attachment: 158622 Committed r125710: <http://trac.webkit.org/changeset/125710>

All reviewed patches have been landed. Closing bug.