Bug 31299

Summary: WAI-ARIA: implement treegrid
Product: WebKit Reporter: chris fleizach <cfleizach>
Component: AccessibilityAssignee: chris fleizach <cfleizach>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, eric, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Macintosh   
OS: OS X 10.5   
Attachments:
Description Flags
patch darin: review+

Description chris fleizach 2009-11-10 09:11:57 PST
the treegrid role needs to be implemented
Comment 1 chris fleizach 2009-12-13 13:43:29 PST
Created attachment 44760 [details]
patch
Comment 2 chris fleizach 2009-12-13 13:44:04 PST
the last ARIA roleā€¦
Comment 3 WebKit Review Bot 2009-12-13 13:44:22 PST
style-queue ran check-webkit-style on attachment 44760 [details] without any errors.
Comment 4 Darin Adler 2009-12-14 09:56:41 PST
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
Comment 5 chris fleizach 2009-12-14 23:12:52 PST
(In reply to comment #4)
Thanx. Will address all of these
Comment 6 chris fleizach 2009-12-14 23:32:46 PST
http://trac.webkit.org/changeset/52140
Comment 8 Eric Seidel (no email) 2009-12-15 02:10:30 PST
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.
Comment 9 Eric Seidel (no email) 2009-12-15 02:18:35 PST
Committed r52146: <http://trac.webkit.org/changeset/52146>
Comment 10 Eric Seidel (no email) 2009-12-15 02:19:18 PST
Rolled out the change, so re-opening the bug.
Comment 11 chris fleizach 2009-12-15 10:01:44 PST
the leopard/tiger test that failed (the new test) should just not be run on leopard...
Comment 12 chris fleizach 2009-12-15 10:08:07 PST
try again
http://trac.webkit.org/changeset/52159
Comment 13 Eric Seidel (no email) 2009-12-15 11:20:56 PST
Thanks!