Bug 87899 – AX: Need AccessibilityObjects for nodes without renderers in canvas subtree

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Dominic Mazzoni]

Basically, it should be possible to create an AccessibilityObject out of a Node that doesn't have a renderer. This is necessary to support accessibility for canvas fallback content (https://bugs.webkit.org/show_bug.cgi?id=50126&GoAheadAndLogIn=1) but may also help clean up AccessibilityRenderObject so the render-specific logic is separate from the logic that only depends on a Node.

[Dominic Mazzoni]

Created attachment 152429 [details]
Patch

[chris fleizach]

Comment on attachment 152429 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:196
> +    if (node->renderer())

we should add a comment that Nodes with renderers should be made into AccessibilityRenderObjects because we prefer using the render tree as the data model

> Source/WebCore/accessibility/AXObjectCache.cpp:444
> +    if (node->renderer()) {

is it possible a node got a renderer attached after we had made the object, meaning that we'd still want to try to clean up the m_nodeObjectMapping?

> Source/WebCore/accessibility/AXObjectCache.h:188
> +    HashMap<Node*, AXID> m_nodeObjectMapping;

does it make sense to have three of these hash maps? or should it be a more generic type?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:142
> +    m_node = renderer->node();

should we call setNode() here so that AccessibilityRenderObject doesn't need to know about m_node

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3469
> +        AccessibilityNodeObject::addChildren();

can we add these lines in a new method like "addCanvasChildren" and put it below with the other special cases?

[Dominic Mazzoni]

Created attachment 153367 [details]
Patch

[Dominic Mazzoni]

(In reply to comment #2)
> (From update of attachment 152429 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152429&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:196
> > +    if (node->renderer())
> 
> we should add a comment that Nodes with renderers should be made into AccessibilityRenderObjects because we prefer using the render tree as the data model

Good idea, done.

> > Source/WebCore/accessibility/AXObjectCache.cpp:444
> > +    if (node->renderer()) {
> 
> is it possible a node got a renderer attached after we had made the object, meaning that we'd still want to try to clean up the m_nodeObjectMapping?

True, I think this could happen if an element is first inside of a canvas and then later reparented outside the canvas. In that case the node -> AccessibilityObject would leak.

In the next change I'm going to replace all of the calls to AXObjectCache from everywhere else in WebCore so that they act on a Node instead of a Renderer. That will fix notifications and I think I can fix this corner case then. I specifically mentioned this test in https://bugs.webkit.org/show_bug.cgi?id=91794 and I'll write a test to confirm it's fixed then.

> > Source/WebCore/accessibility/AXObjectCache.h:188
> > +    HashMap<Node*, AXID> m_nodeObjectMapping;
> 
> does it make sense to have three of these hash maps? or should it be a more generic type?

Unfortunately I can't think of an easy way to use a more generic type without doing more unsafe type conversions. I think this is fine since every AccessibilityObject only appears in one hash map.

> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:142
> > +    m_node = renderer->node();
> 
> should we call setNode() here so that AccessibilityRenderObject doesn't need to know about m_node

Good idea, made m_node private too.

> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3469
> > +        AccessibilityNodeObject::addChildren();
> 
> can we add these lines in a new method like "addCanvasChildren" and put it below with the other special cases?

Sure, done. I had to modify canHaveChildren also.

[chris fleizach]

Comment on attachment 153367 [details]
Patch

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

looks good. let's go for it

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:294
> +            unsigned length = children.size();

i believe you want size_t for size() calls

[Dominic Mazzoni]

Created attachment 153413 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 153413 [details]
Patch for landing

Clearing flags on attachment: 153413

Committed r123182: <http://trac.webkit.org/changeset/123182>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 91863

[Jer Noble]

This patch was rolled out in <http://trac.webkit.org/changeset/123219>.

The cause for the rollout were ASSERT crashes in debug builds.  See:

http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r123210%20(1119)/results.html
http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r123210%20(1119)/accessibility/canvas-accessibilitynodeobject-crash-log.txt
http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r123210%20(1119)/accessibility/canvas-crash-log.txt

[Dominic Mazzoni]

Created attachment 153582 [details]
Patch

[Dominic Mazzoni]

This latest patch fixes the assertion failure. Sorry, I didn't check thoroughly enough after making the final changes.

Now I remember why I originally had AccessibilityRenderObject::addChildren call AccessibilityNodeObject::addChildren at the top - because both methods assume m_haveChildren is false initially.

I fixed it by having addCanvasChildren assert there are no children and then reset m_haveChildren to false before calling AccessibilityNodeObject::addChildren. Everything else is the same.

[chris fleizach]

Comment on attachment 153582 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3460
> +    if (node() && node()->hasTagName(canvasTag)) {

we should re-write this so it uses early return

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3465
> +        return;

no need for return statement here

[Dominic Mazzoni]

Created attachment 153732 [details]
Patch

[WebKit Review Bot]

Comment on attachment 153732 [details]
Patch

Clearing flags on attachment: 153732

Committed r123428: <http://trac.webkit.org/changeset/123428>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.