[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 #131192|review?                     |review-
               Flag|                            |




--- Comment #9 from chris fleizach <cfleizach at apple.com>  2012-03-12 10:25:04 PST ---
(From update of attachment 131192)
View in context: https://bugs.webkit.org/attachment.cgi?id=131192&action=review

> LayoutTests/ChangeLog:9
> +        * accessibility/table-headers.html: Added.

These tests, test too many things and are unclear in their results. 
Dumping the whole AX tree makes it impossible to figure out what's happening in the test.
You should make specific calls to verify that specific information is correct.
Also, i think there should be a separate test per <table> because they test different things. It will make it much easier to debug future problems

What platforms does this test actually work on?

> Source/WebCore/ChangeLog:5
> +

they are exposed just fine on the Mac, through AXColumnHeaderUIElements and AXRowHeaderUIElements. This change log needs more information as to what platform its affecting and what is actually missing.

> Source/WebCore/accessibility/AccessibilityObject.h:351
> +    // accessible table interface.

this comment could be on one line

> Source/WebCore/accessibility/AccessibilityObject.h:355
>      virtual bool isDataTable() const { return false; }

as far as i can tell, isDataTable() does not need to be on AccessibilityObject. I see it only used within AccessibilityTable. we can move this method to AccessibilityTable. 
The double negative comment is hard to understand. 
The comment should read "Returns true if heuristics determine the HTML table appears to be exposing a grid of data, and not used for layout purposes."

> Source/WebCore/accessibility/AccessibilityObject.h:358
> +    // Return true if something is a cell, a rowheader or a columnheader

comments need periods

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1737
> +        return false;

checking m_renderer isn't important here. there's nothing in this method that uses m_renderer. ariaRoleAttribute() takes care of that

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1739
> +    AccessibilityRole ariaRole = ariaRoleAttribute();

this can all be made into a one line function

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3289
> +{

i don't think you need this method.
bool AccessibilityObject::isLandmark() const
already exists

> Source/WebCore/accessibility/AccessibilityRenderObject.h:275
> +    

both of these do not need to be on the render object. they should be on AccessibilityObject

> Source/WebCore/accessibility/AccessibilityTable.cpp:86
>          return false;

this line makes it seem like to me that something can still be a table even if it's role is a landmark role. that seems wrong

> Source/WebCore/accessibility/AccessibilityTable.h:91
> +    // Return true if it is viable to expose an a11y table interface for

a11y is not a word. please use a real word.

> Source/WebCore/accessibility/AccessibilityTable.h:94
>      bool isTableExposableThroughAccessibility() const;

this comment is wrong. this is a private method to determine if the table should be exposed as a table interface. isAccessibilityTable() returns the result of this method (which is stored in a bool)

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:-98
> -    return CellRole;

There is a lot of work that is being performed below. the result should be cached whether it's a Cell, ColumnHeader or RowHeader, or if it should ask AccessibilityRenderObject::roleValue() for the answer so that this does not need to be calculated every time

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:101
> +    static_cast<HTMLTableCellElement*>(node());

this line doesn't need to be on two lines

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:103
> +        ASSERT_NOT_REACHED();

this seems like a useless check. when would cellElement ever == 0? i suppose only if node() was nil, so it seems like you should just check if node() == 0 and return

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:134
> +    // based on the position of the <th> in the table

comments should end with periods.

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:152
> +         static_cast<AccessibilityTable*>(parentTable());

you should use toAccessibilityTable() instead of this and this can be on one line

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:164
> +    Element* nextCellElt = static_cast<Element*>(nextCellInRow->node());     

what's an "Elt"

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:165
> +    if (!nextCellElt) {

why are you casting to Element? hasTagName exists on node()

> Source/WebCore/accessibility/AccessibilityTableCell.h:44
> +    // Return true if either table cell or column/row header

this should be a full sentence

> Tools/Scripts/prepare-ChangeLog:65
> +use lib "Tools/Scripts/";

I doubt this fix requires a change to prepare-Changelog. if it does, it should be in a separate patch

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