[webkit-reviews] review granted: [Bug 133163] AX: WebKit does not recognize ARIA 1.1 tables : [Attachment 231926] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 133163: AX: WebKit does not recognize ARIA 1.1 tables
https://bugs.webkit.org/show_bug.cgi?id=133163

Attachment 231926: patch
https://bugs.webkit.org/attachment.cgi?id=231926&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.