AX: VoiceOver unable to access content in malformed trees

[Nan Wang]

Created attachment 257513 [details]
simple testcase

Sometimes when the tree is malformed, VoiceOver will skip content.

[Radar WebKit Bug Importer]

<rdar://problem/21997009>

[Nan Wang]

<rdar://problem/14862892>

[Nan Wang]

Created attachment 257514 [details]
patch

[Nan Wang]

Created attachment 257515 [details]
patch2

[Nan Wang]

Created attachment 257519 [details]
patch

some minor modification

[chris fleizach]

Comment on attachment 257519 [details]
patch

it looks like your general approach is to try to invalidate tree role usage after the role has been assigned.
i think instead we should take the approach like AccessibilityList where AXObjectCache will make a new element t(AccessibilityTree.cpp) where we can override determineAccessibilityRole() to look at the children and change the role depending on what we need do that

that way we don't have to worry about missing any cases where tree role still peeks through and we don't have to pollute AccessibilityObject with methods that don't apply to all objects

[Nan Wang]

Created attachment 257593 [details]
patch

[WebKit Commit Bot]

Attachment 257593 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/AccessibilityTree.cpp:30:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/accessibility/AccessibilityTree.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/accessibility/AccessibilityTree.cpp:42:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
ERROR: Source/WebCore/accessibility/AccessibilityTree.cpp:99:  Declaration has space between type name and * in AccessibilityObject *axObj  [whitespace/declaration] [3]
ERROR: Source/WebCore/accessibility/AccessibilityTreeItem.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/accessibility/AccessibilityTree.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/accessibility/AccessibilityTreeItem.cpp:30:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/accessibility/AccessibilityTreeItem.cpp:39:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
ERROR: Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2059:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 9 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 257593 [details]
patch

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

in general, i think we don't want to add new attributes. we want to keep our logic inside ::roleValue() and determineRoleValue()

> Source/WebCore/accessibility/AccessibilityRenderObject.h:214
> +    virtual bool shouldIgnoreAttributeRole() const { return false; }

I doubt we need either of these methods.

> Source/WebCore/accessibility/AccessibilityTree.cpp:62
> +    m_ariaRole = determineAriaRoleAttribute();

it doesn't look like you use this anywhere.

for 1, if the role is not a "tree" role we shouldn't proceed any further

> Source/WebCore/accessibility/AccessibilityTree.cpp:65
> +    m_role = TreeRole;

seems like we should just override canHaveChildren() and return true

> Source/WebCore/accessibility/AccessibilityTree.cpp:74
> +        if (child->isGroup()) {

can we have groups of groups of groups of tree items?

> Source/WebCore/accessibility/AccessibilityTree.cpp:77
> +                    m_isMalformed = true;

this should just set some local variable. we don't need an instance variable to store this

> Source/WebCore/accessibility/AccessibilityTree.cpp:84
> +    // After validating the tree, we want the children to reflect the correct role

this comment is not necessary. it is self evident from the code

> Source/WebCore/accessibility/AccessibilityTreeItem.h:42
> +        virtual bool shouldIgnoreAttributeRole() const override { return !m_isTreeItemValid; }

i don't think this concept is a good one. i think the role should just reflect what the object is. we don't want to have to check another attribute to figure out what the role is

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2059
> +            return @"AXMalformed";

this is not something we want to return. if the object is not a tree, it should just revert to what it normally would be, which will be taken care of by code below

[Nan Wang]

Created attachment 257605 [details]
patch

Fixed style and modified build files

[Nan Wang]

Created attachment 257617 [details]
patch

Fixed an issue that group of group of groups of treeitems is considered valid for a tree.

[Nan Wang]

Created attachment 257712 [details]
patch

Broke the dependency cycle between tree and treeitem.

[chris fleizach]

Comment on attachment 257712 [details]
patch

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

thanks

> Source/WebCore/ChangeLog:9
> +        VoiceOver is skipping the content of the malformed trees. Fixed it by ignoring

VoiceOver is skipping the content of malformed trees. Fixed it by ignoring the tree/treeitem attribute roles for those malformed trees.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2505
> +    if (m_ariaRole != UnknownRole && !shouldIgnoreAriaRole)

put this into the if statement for brevity
shouldIgnoreAttributeRole();

> Source/WebCore/accessibility/AccessibilityRenderObject.h:213
> +    virtual bool shouldIgnoreAttributeRole() const { return false; }

i would put this on AccessibilityObject

> Source/WebCore/accessibility/AccessibilityTree.cpp:75
> +    std::queue<Node*> nodeQueue;

lets use Dequeue here

> Source/WebCore/accessibility/AccessibilityTree.cpp:88
> +        if (is<Element>(*child)) {

i think we can cut down a lot of code using nodeHasRole(node, "tree item") || nodeHas("group")

> Source/WebCore/accessibility/AccessibilityTree.cpp:99
> +            for (Node* groupChild = child->firstChild(); groupChild; groupChild = groupChild->nextSibling())

i think we can use auto in more places here

> Source/WebCore/accessibility/AccessibilityTree.h:46
> +    bool isTreeValid();

i think this can be const

> Source/WebCore/accessibility/AccessibilityTreeItem.cpp:60
> +    AccessibilityObject* axObj = parentObject();

i think we can write this as one for loop line

AXObject *ax= nullptr;
for ( = parent; parent && !parent->IsTreE(); parent = next parent) {}

m_isTreeValid = parent;

> LayoutTests/platform/mac/accessibility/malformed-tree.html:38
> +    shouldBe("tree.role", "\"AXRole: AXOutline\"");

"\"AXRole: AXOutline\"" =>

"'AXRole: AXOutline'"

[Nan Wang]

Created attachment 257795 [details]
patch

[chris fleizach]

Comment on attachment 257795 [details]
patch

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

thanks. almost there

> Source/WebCore/ChangeLog:9
> +        VoiceOver is skipping the content of malformed trees. Fixed it by ignoring

VoiceOver is skipping the content of malformed trees.
This fixes the problem by having the tree check if it has valid children (tree items) and makes tree items check if they are inside trees.

> Source/WebCore/accessibility/AccessibilityObject.h:776
> +    // Function to determine if the attribute role should be ignored

this comment probably not necessary. your method name does the same thing that this comment says

> Source/WebCore/accessibility/AccessibilityTree.cpp:72
> +    // A valid tree can only have treeitem or group of treeitems as child

"... as a child."

> Source/WebCore/accessibility/AccessibilityTree.cpp:75
> +    Deque<Node*> queue;

we should make this queue after we check if there's node (next line)

> Source/WebCore/accessibility/AccessibilityTree.cpp:85
> +        Node* child = queue.takeFirst();

auto child

> Source/WebCore/accessibility/AccessibilityTree.cpp:89
> +        if (nodeHasRole(child, "treeitem"))

does nodeHasRole already check the is<Element> for us? we might be able to get rid of that check

> Source/WebCore/accessibility/AccessibilityTreeItem.cpp:56
> +    m_isTreeItemValid = false;

it looks like we don't need to initialize this to anything so the line below will take care of it

> Source/WebCore/accessibility/AccessibilityTreeItem.cpp:58
> +    // Walk the parent chain looking for a parent that is a tree, treeitem is considered

// Walk the parent chain looking for a parent that is a tree. A tree item is only considered valid if it is in a tree.

[Nan Wang]

Comment on attachment 257795 [details]
patch

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

>> Source/WebCore/accessibility/AccessibilityTree.cpp:89
>> +        if (nodeHasRole(child, "treeitem"))
> 
> does nodeHasRole already check the is<Element> for us? we might be able to get rid of that check

The nodeHasRole will return false if the node is not Element object. But we want to ignore checking nodes that are not Elements, like text nodes. I think we still need to keep that check.

[Nan Wang]

Created attachment 257797 [details]
patch

[WebKit Commit Bot]

Comment on attachment 257797 [details]
patch

Clearing flags on attachment: 257797

Committed r187582: <http://trac.webkit.org/changeset/187582>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #18)
> Comment on attachment 257797 [details]
> patch
> 
> Clearing flags on attachment: 257797
> 
> Committed r187582: <http://trac.webkit.org/changeset/187582>

It broke the Apple Windows build:
WebCore.lib(AccessibilityAllInOne.obj) : error LNK2019: unresolved external symbol "public: static class WTF::Ref<class WebCore::AccessibilityTree> __cdecl WebCore::AccessibilityTree::create(class WebCore::RenderObject *)" (?create@AccessibilityTree@WebCore@@SA?AV?$Ref@VAccessibilityTree@WebCore@@@WTF@@PAVRenderObject@2@@Z) referenced in function "class WTF::Ref<class WebCore::AccessibilityObject> __cdecl WebCore::createFromRenderer(class WebCore::RenderObject *)" (?createFromRenderer@WebCore@@YA?AV?$Ref@VAccessibilityObject@WebCore@@@WTF@@PAVRenderObject@1@@Z) [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj]
WebCore.lib(AccessibilityAllInOne.obj) : error LNK2019: unresolved external symbol "public: static class WTF::Ref<class WebCore::AccessibilityTreeItem> __cdecl WebCore::AccessibilityTreeItem::create(class WebCore::RenderObject *)" (?create@AccessibilityTreeItem@WebCore@@SA?AV?$Ref@VAccessibilityTreeItem@WebCore@@@WTF@@PAVRenderObject@2@@Z) referenced in function "class WTF::Ref<class WebCore::AccessibilityObject> __cdecl WebCore::createFromRenderer(class WebCore::RenderObject *)" (?createFromRenderer@WebCore@@YA?AV?$Ref@VAccessibilityObject@WebCore@@@WTF@@PAVRenderObject@1@@Z) [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\bin32\WebKit.dll : fatal error LNK1120: 2 unresolved externals [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj]

[chris fleizach]

(In reply to comment #20)
> (In reply to comment #18)
> > Comment on attachment 257797 [details]
> > patch
> > 
> > Clearing flags on attachment: 257797
> > 
> > Committed r187582: <http://trac.webkit.org/changeset/187582>
> 
> It broke the Apple Windows build:
> WebCore.lib(AccessibilityAllInOne.obj) : error LNK2019: unresolved external
> symbol "public: static class WTF::Ref<class WebCore::AccessibilityTree>
> __cdecl WebCore::AccessibilityTree::create(class WebCore::RenderObject *)"
> (?create@AccessibilityTree@WebCore@@SA?AV?$Ref@VAccessibilityTree@WebCore@@@W
> TF@@PAVRenderObject@2@@Z) referenced in function "class WTF::Ref<class
> WebCore::AccessibilityObject> __cdecl WebCore::createFromRenderer(class
> WebCore::RenderObject *)"
> (?createFromRenderer@WebCore@@YA?AV?$Ref@VAccessibilityObject@WebCore@@@WTF@@
> PAVRenderObject@1@@Z)
> [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.
> vcxproj\WebKit\WebKit.vcxproj]
> WebCore.lib(AccessibilityAllInOne.obj) : error LNK2019: unresolved external
> symbol "public: static class WTF::Ref<class WebCore::AccessibilityTreeItem>
> __cdecl WebCore::AccessibilityTreeItem::create(class WebCore::RenderObject
> *)"
> (?create@AccessibilityTreeItem@WebCore@@SA?AV?$Ref@VAccessibilityTreeItem@Web
> Core@@@WTF@@PAVRenderObject@2@@Z) referenced in function "class
> WTF::Ref<class WebCore::AccessibilityObject> __cdecl
> WebCore::createFromRenderer(class WebCore::RenderObject *)"
> (?createFromRenderer@WebCore@@YA?AV?$Ref@VAccessibilityObject@WebCore@@@WTF@@
> PAVRenderObject@1@@Z)
> [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.
> vcxproj\WebKit\WebKit.vcxproj]
> C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\bin32\WebKit.dll : fatal error LNK1120: 2
> unresolved externals
> [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.
> vcxproj\WebKit\WebKit.vcxproj]


I think you need to add includes for these two new files to AccessibilityAllInOne.cpp

[Alex Christensen]

Should be fixed in http://trac.webkit.org/changeset/187584