Bug 130563 - AX: AccessibilityObject::children() returns invalid results sometimes
Summary: AX: AccessibilityObject::children() returns invalid results sometimes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on: 130721
Blocks:
  Show dependency treegraph
 
Reported: 2014-03-20 22:19 PDT by James Craig
Modified: 2014-04-03 01:28 PDT (History)
6 users (show)

See Also:


Attachments
patch (4.97 KB, patch)
2014-03-24 16:09 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (4.98 KB, patch)
2014-03-24 16:20 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (5.56 KB, patch)
2014-03-24 16:34 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (5.79 KB, patch)
2014-03-24 16:43 PDT, chris fleizach
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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