Bug 130563

Summary: AX: AccessibilityObject::children() returns invalid results sometimes
Product: WebKit Reporter: James Craig <jcraig>
Component: AccessibilityAssignee: chris fleizach <cfleizach>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jcraig, joepeck, mario, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 130721    
Bug Blocks:    
Attachments:
Description Flags
patch
none
patch
none
patch
none
patch joepeck: review+

Description James Craig 2014-03-20 22:19:46 PDT
At least one of the test elements in the inspector-protocol tests gets an invalid result for children(). It should either be including the <span> as an child, or promoting the <span>'s descendant nodes (because the <span> is ignored), but WebCore is doing neither of these things. Somehow it's coming out right in the AX API (the ignored <span> is being exposed as an AXGroup) but that output need to be reconciled with this output.

Test case is in LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForNode.html

<div role="group" class="ex">
    <!-- FIXME: accessibilityChildNodeIds.length should be 3 (foo, bar, baz). WebCore only reports 1 (foo). -->
    <div>foo</div>
    <span>
        bar
        <span role="button">baz</span>
    </span>
    <div></div>
    <div></div>
</div>

Source/WebCore/accessibility/AccessibilityObject.cpp:1389

Blocked by http://webkit.org/b/130264
Probably related to http://webkit.org/b/130181

If there is a reason it's done this way, kick this back to me in the Web Inspector component. It's possible I'll need to add a new method for AccessibilityObject::unignoredChildren()
Comment 1 Radar WebKit Bug Importer 2014-03-20 22:25:21 PDT
<rdar://problem/16388161>
Comment 2 chris fleizach 2014-03-24 16:09:00 PDT
Created attachment 227699 [details]
patch
Comment 3 WebKit Commit Bot 2014-03-24 16:11:30 PDT
Attachment 227699 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/InspectorDOMAgent.h:237:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WebCore/inspector/InspectorDOMAgent.cpp:1418:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 chris fleizach 2014-03-24 16:20:43 PDT
Created attachment 227701 [details]
patch
Comment 5 James Craig 2014-03-24 16:28:29 PDT
Comment on attachment 227701 [details]
patch

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

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1418
> +void InspectorDOMAgent::processAccessibilityChildren(PassRefPtr<AccessibilityObject> axObject, RefPtr<Inspector::TypeBuilder::Array<int>>& childNodeIds)

Wouldn't this be better as AccessibilityObject::childrenPromoted() similar to AccessibilityObject::parentObjectUnignored()? This is is the approach I was planning to take if you determined AccessibilityObject::children() behaved correctly.

> LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForNode-expected.txt:397
> -    childNodeIds.length: 1
> +    childNodeIds.length: 3

There's a FIXME comment in the layout test that should be removed, too.
Comment 6 chris fleizach 2014-03-24 16:34:13 PDT
Comment on attachment 227701 [details]
patch

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

>> Source/WebCore/inspector/InspectorDOMAgent.cpp:1418
>> +void InspectorDOMAgent::processAccessibilityChildren(PassRefPtr<AccessibilityObject> axObject, RefPtr<Inspector::TypeBuilder::Array<int>>& childNodeIds)
> 
> Wouldn't this be better as AccessibilityObject::childrenPromoted() similar to AccessibilityObject::parentObjectUnignored()? This is is the approach I was planning to take if you determined AccessibilityObject::children() behaved correctly.

No. The DOM inspector only cares about Nodes, while the AXTree also cares about render objects.
Comment 7 chris fleizach 2014-03-24 16:34:49 PDT
Created attachment 227703 [details]
patch
Comment 8 James Craig 2014-03-24 16:36:52 PDT
Comment on attachment 227703 [details]
patch

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

> LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForNode-expected.txt:397
> -    childNodeIds.length: 1
> +    childNodeIds.length: 3

You need to regenerate the layout test result again now that you've removed the comment. The outerHTML (including FIXME comment) gets output.
Comment 9 chris fleizach 2014-03-24 16:43:09 PDT
Created attachment 227704 [details]
patch
Comment 10 Joseph Pecoraro 2014-04-01 22:54:02 PDT
Comment on attachment 227704 [details]
patch

r+

Is this likely to produce very large lists of children if you select something like <body>?
Comment 11 James Craig 2014-04-02 00:04:40 PDT
It doesn't return all the leaf nodes of the entire tree if that's what you mean. It stops at accessible group boundaries (e.g. a list, but not all it's listitem children). This will return long lists in places, but the UI tweak for that should be covered in bug 130911.
Comment 12 chris fleizach 2014-04-03 01:28:01 PDT
http://trac.webkit.org/changeset/166703