Bug 133163 – AX: WebKit does not recognize ARIA 1.1 tables

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[James Craig]

Created attachment 231847 [details]
test case

This is how we're planning to transition into ARIA 1.1 static tables from ARIA 1.0 grids. (Yes, I know the example is redundant.)

WebKit exposes this as 0 columns, 0 rows due to the fallback role tokens.

<table border="0" role="table grid">
  <tr role="row">
    <th role="columnheader">Foo</th>
    <th role="columnheader">Bar</th>
    <th role="columnheader">Baz</th>
  </tr>
  <tr role="row">
    <td role="cell gridcell">Bif</td>
    <td role="cell gridcell">Baf</td>
    <td role="cell gridcell">Bop</td>
  </tr>
</table>

Technically this should "work" as an ARIA 1.0 grid, today. Roles should fall back to grid/gridcell, respectively.

[Radar WebKit Bug Importer]

<rdar://problem/16997147>

[chris fleizach]

Created attachment 231926 [details]
patch

[Darin Adler]

Comment on attachment 231926 [details]
patch

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

I’m OK with this change, but it doesn’t seem to cover the edge cases of parsing the role attribute very well.

> Source/WebCore/accessibility/AXObjectCache.cpp:257
> +    String roleValue = toElement(node)->fastGetAttribute(roleAttr);

The return value from fastGetAttribute is const AtomicString&, not String. Putting it into a String is OK, but not a good idiom to use most of the time. I’d suggest const AtomicString& or auto& for the type of this value.

> Source/WebCore/accessibility/AXObjectCache.cpp:259
> +    if (roleValue.isEmpty() && equalIgnoringCase(role, nullAtom))
> +        return true;

We don’t need to use equalIgnoringCase to check if the passed role is null; also, if the role passed is null, I don’t think we really want to run space splitting code below. That would return true if there was a leading or trailing space or two spaces in a row, and I don’t think that’s what we want. You should write this:

    if (role.isNull())
        return roleValue.isEmpty();

But we should also consider if a role of null should match a role attribute consisting entirely of spaces. As written, a role of null will return true if the role attribute is not present, or if it’s present and the empty string, but not if it’s present and has only spaces in it.

> Source/WebCore/accessibility/AXObjectCache.cpp:267
> +    Vector<String> roles;
> +    roleValue.split(' ', roles);
> +    for (const auto& splitRole : roles) {
> +        if (equalIgnoringCase(splitRole, role))
> +            return true;
> +    }
> +    return false;

Is a literal space character the only separator that works? What about other kinds of whitespace? Actually allocating a vector of strings is not a very efficient way to do this operation. We have a class in the DOM, SpaceSplitString, designed for this kind of thing, although it’s probably also allocating a vector and probably not optimized for the use here. But to use it we would just write:

    return SpaceSplitString(roleValue, true).contains(role);

The class is old enough that we use a boolean for the case folding (not an enum as we would in more modern code). I’m not sure that SpaceSplitString will get the edge cases exactly right. To find out we would need more test cases.

[chris fleizach]

(In reply to comment #3)
> (From update of attachment 231926 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231926&action=review
> 
> I’m OK with this change, but it doesn’t seem to cover the edge cases of parsing the role attribute very well.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:257
> > +    String roleValue = toElement(node)->fastGetAttribute(roleAttr);
> 
> The return value from fastGetAttribute is const AtomicString&, not String. Putting it into a String is OK, but not a good idiom to use most of the time. I’d suggest const AtomicString& or auto& for the type of this value.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:259
> > +    if (roleValue.isEmpty() && equalIgnoringCase(role, nullAtom))
> > +        return true;
> 
> We don’t need to use equalIgnoringCase to check if the passed role is null; also, if the role passed is null, I don’t think we really want to run space splitting code below. That would return true if there was a leading or trailing space or two spaces in a row, and I don’t think that’s what we want. You should write this:
> 
>     if (role.isNull())
>         return roleValue.isEmpty();
> 
> But we should also consider if a role of null should match a role attribute consisting entirely of spaces. As written, a role of null will return true if the role attribute is not present, or if it’s present and the empty string, but not if it’s present and has only spaces in it.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:267
> > +    Vector<String> roles;
> > +    roleValue.split(' ', roles);
> > +    for (const auto& splitRole : roles) {
> > +        if (equalIgnoringCase(splitRole, role))
> > +            return true;
> > +    }
> > +    return false;
> 
> Is a literal space character the only separator that works? What about other kinds of whitespace? Actually allocating a vector of strings is not a very efficient way to do this operation. We have a class in the DOM, SpaceSplitString, designed for this kind of thing, although it’s probably also allocating a vector and probably not optimized for the use here. But to use it we would just write:
> 
>     return SpaceSplitString(roleValue, true).contains(role);
> 
> The class is old enough that we use a boolean for the case folding (not an enum as we would in more modern code). I’m not sure that SpaceSplitString will get the edge cases exactly right. To find out we would need more test cases.

Thanks for the review. I'll apply these comments and use SpaceSplitString and add a few more test cases to see what falls out.

[chris fleizach]

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