Bug 87899

Summary: AX: Need AccessibilityObjects for nodes without renderers in canvas subtree
Product: WebKit Reporter: Dominic Mazzoni <dmazzoni>
Component: AccessibilityAssignee: Dominic Mazzoni <dmazzoni>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cfleizach, eric.carlson, feature-media-reviews, gyuyoung.kim, japhet, jer.noble, jochen, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 91863    
Bug Blocks: 50126, 91794    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
none
Patch
none
Patch none

Description Dominic Mazzoni 2012-05-30 14:32:32 PDT
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.
Comment 1 Dominic Mazzoni 2012-07-14 12:07:43 PDT
Created attachment 152429 [details]
Patch
Comment 2 chris fleizach 2012-07-18 09:25:04 PDT
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?
Comment 3 Dominic Mazzoni 2012-07-19 16:12:23 PDT
Created attachment 153367 [details]
Patch
Comment 4 Dominic Mazzoni 2012-07-19 16:12:52 PDT
(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.
Comment 5 chris fleizach 2012-07-19 16:43:03 PDT
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
Comment 6 Dominic Mazzoni 2012-07-19 22:33:09 PDT
Created attachment 153413 [details]
Patch for landing
Comment 7 WebKit Review Bot 2012-07-19 23:06:21 PDT
Comment on attachment 153413 [details]
Patch for landing

Clearing flags on attachment: 153413

Committed r123182: <http://trac.webkit.org/changeset/123182>
Comment 8 WebKit Review Bot 2012-07-19 23:06:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Review Bot 2012-07-20 07:50:32 PDT
Re-opened since this is blocked by 91863
Comment 11 Dominic Mazzoni 2012-07-20 13:27:46 PDT
Created attachment 153582 [details]
Patch
Comment 12 Dominic Mazzoni 2012-07-20 13:28:56 PDT
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.
Comment 13 chris fleizach 2012-07-20 17:10:04 PDT
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
Comment 14 Dominic Mazzoni 2012-07-22 23:00:34 PDT
Created attachment 153732 [details]
Patch
Comment 15 WebKit Review Bot 2012-07-23 23:26:54 PDT
Comment on attachment 153732 [details]
Patch

Clearing flags on attachment: 153732

Committed r123428: <http://trac.webkit.org/changeset/123428>
Comment 16 WebKit Review Bot 2012-07-23 23:27:01 PDT
All reviewed patches have been landed.  Closing bug.