Bug 31299 – WAI-ARIA: implement treegrid

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

[chris fleizach]

the treegrid role needs to be implemented

[chris fleizach]

Created attachment 44760 [details]
patch

[chris fleizach]

the last ARIA role…

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44760 [details] without any errors.

[Darin Adler]

Comment on attachment 44760 [details]
patch

> +    for (int k = index+1; k < rowCount; ++k) {

We put spaces around operators like the "+" here.

> +        if (row->hierarchicalLevel() == (level+1))
> +            disclosedRows.append(row);
> +        // Stop at the first row that doesn't match the correct level.
> +        else
> +            break;

I would reverse the logic and says if != break, then put the append outside the if, avoiding the else. Also, I would omit the parentheses around "level+1" and include the spaces around the "+".

> +    // If the level is 1 or less, than nothing discloses this row.
> +    unsigned level = hierarchicalLevel();
> +    if (level < 2)
> +        return 0;

Comment says 1 or less, code should say <= 1 to match it.

> +    for (int k = index-1; k >= 0; --k) {

Should have a space around the "-".

> +        if (row->hierarchicalLevel() == (level-1))

Space around the "-" and no parentheses around "level - 1".

> +    const AtomicString& ariaMultiSelectable = getAttribute(aria_multiselectableAttr).string();

Please remove the ".string()" here. It causes the compiler to create a new temporary AtomicString by converting to String and back to AtomicString, but without it everything should work fine.

>      // Setting selected rows only works on trees for now.
> -    if (roleValue() != TreeRole)
> +    AccessibilityRole role = roleValue();
> +    if (role != TreeRole && role != TreeGridRole && role != TableRole)
>          return;

Comment no longer makes sense, since it works for tables now.
>      
> -    bool isMultiselectable = elementAttributeValue(aria_multiselectableAttr);
> +    bool isMultiselectable = isMultiSelectable();

This works only because you treated "multiselectable" as one word in the local variable and two words in the function name. The two should be capitalized consistently (all lowercase in both cases) and then you either need to eliminate the local variable or use "this->" syntax to call the function.

The override of the supportsSelectedRows function in AccessibilityARIAGrid.h should be private. It's programming mistake to ever call it on an AccessibilityARIAGrid* and it would be good to have the compiler catch such a mistake.

All of these are minor quibbles. r=me as is

[chris fleizach]

(In reply to comment #4)
Thanx. Will address all of these

[chris fleizach]

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

[Eric Seidel (no email)]

This change geborked the bots:
http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r52140%20(8362)/platform/mac/accessibility/aria-treegrid-pretty-diff.html

:(

[Eric Seidel (no email)]

This broke the leopard bots.  No response from Chris (emailed him). I assume he's asleep.  Going to roll this out so that the tree rolls back green and Chris can land this again in the morning.

[Eric Seidel (no email)]

Committed r52146: <http://trac.webkit.org/changeset/52146>

[Eric Seidel (no email)]

Rolled out the change, so re-opening the bug.

[chris fleizach]

the leopard/tiger test that failed (the new test) should just not be run on leopard...

[chris fleizach]

try again
http://trac.webkit.org/changeset/52159

[Eric Seidel (no email)]

Thanks!