[Webkit-unassigned] [Bug 79080] Row and column headers not exposed by WebCore accessibility

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=79080


chris fleizach <cfleizach at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #133916|review?                     |review-
               Flag|                            |




--- Comment #24 from chris fleizach <cfleizach at apple.com>  2012-03-28 17:44:10 PST ---
(From update of attachment 133916)
View in context: https://bugs.webkit.org/attachment.cgi?id=133916&action=review

there's no change logs in this patch.

There's a method in js-pre that allows you to output the purpose of the test. you'll see that in the platform/mac/accessibility tests. I would add those to each of the tests, so it's clear what the test is for (and where to find that purpose)

> b/LayoutTests/accessibility/table-headers-aria-div-grid.html:15
> +        <span role="gRidcell">Cell</span>

strange capitalization here

> b/LayoutTests/accessibility/table-headers-aria-div-grid.html:35
> +        var table  = accessibilityController.focusedElement.childAtIndex(0);

too much whitespace after table

> LayoutTests/accessibility/table-headers-aria-table-grid.html:21
> +        <th role="coluMnheader">Columnheader</th>

strange capitalization here

> LayoutTests/accessibility/table-headers-aria-table-grid.html:26
> +        <th role="coluMnheader">Columnheader</th>

strange capitalization here

> LayoutTests/accessibility/table-headers-aria-table-grid.html:35
> +        var table  = accessibilityController.focusedElement.childAtIndex(0);

too much white space

> LayoutTests/accessibility/table-headers-heuristic-column-only.html:29
> +        var table  = accessibilityController.focusedElement.childAtIndex(0);

spacing

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:118
> +    // 1. If grandparent is a <thead> this is a column header, for both <th> and <td>

I would remove the --- from the comment as well as the empty line. There's nothing extraordinary about this comment

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:122
> +    ASSERT(tr->hasTagName(trTag)); // DOM has element even if not in doc.

place comment before the assert line. also that comment does not explain why you need to assert

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:129
> +    // 2. All <td> elements not inside a <thead> are cells 

comments end in period

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:130
> +    if (currentNode->hasTagName(tdTag))

this is the 2nd time you called hasTagName(tdTag). you should probably cache it

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:135
> +    // 3. Check scope attribute

ditto for this comment block. comments end in period

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:153
> +        return ColumnHeaderRole; // First row, but not first col.

i would put comment above if block

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:155
> +        return RowHeaderRole; // First col, but not first row.

i would put comment before if block

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:164
> +    AccessibilityTable* parentTableAcc = toAccessibilityTable(parentTable());

the name parentTableAcc is strange to me because it abbreviates Accessibility in a non-standard way in webkit. i would just call it parentTable or parentTableObject

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:165
> +    ASSERT(parentTableAcc); // Cannot be null if isTableCell().

comment before Assert

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:171
> +        parentTableAcc->cellForColumnAndRow(nextColInRow, rowRange.first);

does not seem like this needs to be on two lines

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.