AX: display:none content exposed to accessibility when aria-hidden is toggled on ancestor element

[James Craig]

Created attachment 242330 [details]
test case

AX: display:none content exposed to accessibility when aria-hidden is toggled on ancestor element

Repro: 

1. Open test case.
2. Navigate to the list with VoiceOver and verify there are only two links.
3. Hide and then show the section.
4. Navigate to the list with VoiceOver and notice there are now three links.

aria-hidden=false should only override display:none on the current element, not on descendant elements.

<rdar://problem/19093687>

[James Craig]

Chris, what do you think of this (proposed) edit to disambiguate the hidden state? I might need to update the test case but I think it's much more clear.

http://rawgit.com/w3c/aria/issue-688/aria/aria.html#aria-hidden

[James Craig]

https://github.com/w3c/aria/commit/13cf822c68ddf2066da889f403bfd8c5cb55b4bf

[James Craig]

The important point for this bug is:

- The previous was boolean with false as the default. The new version is true/false/undefined where undefined defers to the renderer. 

WebKit should now check per element:

- If element is rendered and aria-hidden is not true on current element or any ancestor: expose to accessibility API.
- If element is not rendered and aria-hidden=false, walk up the ancestor chain making sure every ancestor element is either rendered or explicitly set as aria-hidden=false. If so, expose the current element to the accessibility API.
- Text nodes should always inherit their parent element's level of exposure.

[chris fleizach]

Created attachment 253559 [details]
patch

[Darin Adler]

Comment on attachment 253559 [details]
patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1310
> +    bool requiresAriaHiddenFalse = !node->renderer();

Checking the renderer for null is only safe if we know that style is up to date. The rest of what this does is DOM-only and does not depend on style/renderers. Does the caller update style?

> Source/WebCore/accessibility/AXObjectCache.cpp:1320
> +            if (!testNode->renderer() && !equalIgnoringCase(ariaHiddenValue, "false"))
> +                return false;
> +            if (equalIgnoringCase(ariaHiddenValue, "false"))
> +                ariaHiddenFalsePresent = true;

It’s not great to call ariaHiddenFalsePresent twice. Should refactor and only do it once.

> Source/WebCore/accessibility/AXObjectCache.cpp:1327
> +    if (requiresAriaHiddenFalse && !ariaHiddenFalsePresent)
> +        return false;
> +    
> +    return true;

If this is the clearest way to write it, I am OK, but it could also be:

    return !requiresAriaHiddenFalse || ariaHiddenFalsePresent;

[chris fleizach]

(In reply to comment #5)
> Comment on attachment 253559 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253559&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1310
> > +    bool requiresAriaHiddenFalse = !node->renderer();
> 
> Checking the renderer for null is only safe if we know that style is up to
> date. The rest of what this does is DOM-only and does not depend on
> style/renderers. Does the caller update style?
> 

Yes, it looks like the callers of this method are used when adding children, where we always update the style first

> > Source/WebCore/accessibility/AXObjectCache.cpp:1320
> > +            if (!testNode->renderer() && !equalIgnoringCase(ariaHiddenValue, "false"))
> > +                return false;
> > +            if (equalIgnoringCase(ariaHiddenValue, "false"))
> > +                ariaHiddenFalsePresent = true;
> 
> It’s not great to call ariaHiddenFalsePresent twice. Should refactor and
> only do it once.
> 

Ok

> > Source/WebCore/accessibility/AXObjectCache.cpp:1327
> > +    if (requiresAriaHiddenFalse && !ariaHiddenFalsePresent)
> > +        return false;
> > +    
> > +    return true;
> 
> If this is the clearest way to write it, I am OK, but it could also be:
> 
>     return !requiresAriaHiddenFalse || ariaHiddenFalsePresent;

Good idea

Thanks

[chris fleizach]

http://trac.webkit.org/changeset/184890

[chris fleizach]

http://trac.webkit.org/changeset/184891