Bug 40132 – AX: presentational role needs to be inherited by required elements

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

The current implementation of 'presentation' is correct (no work lost) but incomplete in that it did not support presentation inheritance. The previous draft was somewhat vague in that area, but the description of the presentation role (mainly the part about presentation inheritance) has been completely rewritten as of today. 

http://www.w3.org/WAI/PF/aria/complete#presentation

Changes to WebKit will include inheritance of the presentation role to structural descendants and labeling element unless those elements have an explicit role defined. Also, presentation role is to be ignored if the element is focusable. See referenced portion of spec for details.

[chris fleizach]

Created attachment 58709 [details]
patch

[chris fleizach]

this makes sure that WebKit behaves correctly by applying the presentation role status to required elements of an element that is presentational

translation: if <ul> is presentation, <li> will be too

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/accessibility/AccessibilityRenderObject.cpp:3071:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 6 files


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

[chris fleizach]

Created attachment 58710 [details]
patch

[David Kilzer (:ddkilzer)]

Comment on attachment 58710 [details]
patch

> Index: WebCore/accessibility/AccessibilityRenderObject.cpp
> +Node* AccessibilityRenderObject::node() const
> +{
> +    if (!m_renderer) 
> +        return 0;
> +    return m_renderer->node();
> +}

You may want to consider a follow-up patch for AccessibilityRenderObject.cpp to switch to using this new node() method, and moving the method to AccessibilityRenderObject.h to make it inline.

>  static inline bool isInlineWithContinuation(RenderObject* renderer)
>  {
> @@ -1738,6 +1745,9 @@
>      if (roleValue() == PresentationalRole)
>          return true;
>      
> +    if (roleValue() == PresentationalRole || inheritsPresentationalRole())
> +        return true;
> +    

You probably want to remove the previous if statement here.

> @@ -3051,6 +3061,45 @@
>      return AccessibilityObject::orientation();
>  }
>      
> +bool AccessibilityRenderObject::inheritsPresentationalRole() const
> +{
> +    // ARIA spec says that when a parent object is presentational, and it has required child elements,
> +    // those child elements are also presentational. For example, <li> becomes presentational from <ul>.

Might be nice to provide a link to the spec here if possible.

> +    bool checkForPresentationalParent = false;
> +    HashSet<QualifiedName> possibleParentTagNames;

Are you sure this HashSet works as intended?  Looking at QualifiedName.cpp, I see it defines:

typedef HashSet<QualifiedName::QualifiedNameImpl*, QualifiedNameHash> QNameSet;

And NodeRareData.h defines:

typedef HashMap<RefPtr<QualifiedName::QualifiedNameImpl>, TagNodeList*> TagNodeListCache;

Looking at const QualifiedName& operator=(const QualifiedName&) in QualifiedName.h leads me to believe that two different QualifiedName objects can point to the same QualiifedNameImpl, in which case you're not really getting a set.  I suspect you want either the QNameSet from QualifiedName.cpp (to avoid the RefPtr<> overhead) or something like this:

HashSet<RefPtr<QualifiedName::QualifiedNameImpl> > possibleParentTagNames;

> +    switch (roleValue()) {
> +    case ListItemRole:
> +    case ListMarkerRole:
> +        checkForPresentationalParent = true;
> +        possibleParentTagNames.add(ulTag);
> +        possibleParentTagNames.add(olTag);
> +        possibleParentTagNames.add(dlTag);
> +        break;
> +    default:
> +        break;
> +    }

Instead of dynamically creating possibleParentTagNames each time this method is called, it might be nice to define possibleParentTagNames as a pointer (HashSet<RefPtr<QualifiedName::QualifiedNameImpl> > *possibleParentTagNames), then use DEFINE_STATIC_LOCAL() in the ListItemRole/ListMarkerRole case statement to create it and assign it to possibleParentTagNames when needed.  You would need some additional NULL checks of possibleParentTagNames below if you did this.

This would also shortcut some hash lookups in the for loop below if the hash is empty.

> +    // Not all elements need to check for this.
> +    if (!checkForPresentationalParent)
> +        return false;
> +    
> +    for (AccessibilityObject* parent = parentObject(); parent; parent = parent->parentObject()) { 
> +        if (!parent->isAccessibilityRenderObject())
> +            continue;
> +        
> +        Node* elementNode = static_cast<AccessibilityRenderObject*>(parent)->node();
> +        if (!elementNode || !elementNode->isElementNode())
> +            continue;
> +        
> +        // If native tag of the parent element matches an acceptable name, then return
> +        // based on its presentational status.
> +        if (possibleParentTagNames.contains(static_cast<Element*>(elementNode)->tagQName()))
> +            return parent->roleValue() == PresentationalRole;
> +    }
> +    
> +    return false;
> +}

r- to remove the redundant if statement and to fix the HashSet<QualifiedName> issue.  The rest is optional.

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> (From update of attachment 58710 [details])
> > +    bool checkForPresentationalParent = false;
> > +    HashSet<QualifiedName> possibleParentTagNames;
> 
> Are you sure this HashSet works as intended?  Looking at QualifiedName.cpp, I see it defines:
> 
> typedef HashSet<QualifiedName::QualifiedNameImpl*, QualifiedNameHash> QNameSet;
> 
> And NodeRareData.h defines:
> 
> typedef HashMap<RefPtr<QualifiedName::QualifiedNameImpl>, TagNodeList*> TagNodeListCache;
> 
> Looking at const QualifiedName& operator=(const QualifiedName&) in QualifiedName.h leads me to believe that two different QualifiedName objects can point to the same QualiifedNameImpl, in which case you're not really getting a set.  I suspect you want either the QNameSet from QualifiedName.cpp (to avoid the RefPtr<> overhead) or something like this:
> 
> HashSet<RefPtr<QualifiedName::QualifiedNameImpl> > possibleParentTagNames;

Ignore my above comments.  The code at the bottom of QualifiedName.h handles the hashing for you.  It's interesting that it doesn't appear to be used elsewhere in WebCore, though.

[chris fleizach]

Created attachment 58905 [details]
Patch

[chris fleizach]

addressed all of david's comments in this latest patch

[David Kilzer (:ddkilzer)]

Comment on attachment 58905 [details]
Patch

WebCore/accessibility/AccessibilityRenderObject.cpp:3059
 +      DEFINE_STATIC_LOCAL(HashSet<QualifiedName>, listItemParents, ());
This static local should be defined within the scope of the switch statement below so it's only created when it's needed.

WebCore/accessibility/AccessibilityRenderObject.cpp:3065
 +      case ListMarkerRole:
The static local variable should be defined here.

r=me

[chris fleizach]

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

[chris fleizach]

doesn't work on windows

c:\cygwin\home\buildbot\slave\win-release\build\WebCore\accessibility\AccessibilityRenderObject.cpp(3072) : error C2361: initialization of 'listItemParents' is skipped by 'default' label
        c:\cygwin\home\buildbot\slave\win-release\build\WebCore\accessibility\AccessibilityRenderObject.cpp(3064) : see declaration of 'listItemParents'

[WebKit Review Bot]

http://trac.webkit.org/changeset/61421 might have broken Chromium Win Release

[chris fleizach]

tiger and leopard are equipped to deal with lists. will ignore those tests

[chris fleizach]

http://trac.webkit.org/changeset/61427
to skip the tests on tiger and leopard since lists are not supported there

[WebKit Review Bot]

http://trac.webkit.org/changeset/61427 might have broken SnowLeopard Intel Release (Tests)

[chris fleizach]

(In reply to comment #15)
> http://trac.webkit.org/changeset/61427 might have broken SnowLeopard Intel Release (Tests)

it did not break that bot