[webkit-reviews] review granted: [Bug 115837] AX: VoiceOver is no longer seeing items in poorly formed tables : [Attachment 201123] 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 115837: AX: VoiceOver is no longer seeing items in poorly formed tables
https://bugs.webkit.org/show_bug.cgi?id=115837

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=201123&action=review


> Source/WebCore/accessibility/AccessibilityARIAGridRow.cpp:121
> +    // The parent table might not be the direct ancestor (unfortunately due
to bad programming).

This reference to “bad programming” is vague and confusing. Could you write
something more specific instead?

> Source/WebCore/accessibility/AccessibilityTable.h:91
> +    // isTable is whether it's an AccessibilityTable object.
> +    virtual bool isTable() const { return true; }
> +    // isAccessibilityTable is whether it is exposed as an
AccessibilityTable to the platform.
> +    virtual bool isAccessibilityTable() const;
> +    // isDataTable is whether it is exposed as an AccessibilitTable because
the heuristic
> +    // think this "looks" like a data-based table (instead of a table used
for layout).
> +    virtual bool isDataTable() const;

All three of these virtual functions should say OVERRIDE.

I don’t find the distinction between isTable and isAccessibilityTable is
expressed by the function name. I’d suggest calling the latter function
something else. Something to return to later.

There’s a typo here: AccessibilitTable.

> Source/WebCore/accessibility/AccessibilityTableRow.cpp:106
> +    for (AccessibilityObject* parent = parentObject(); parent; parent =
parent->parentObject()) {

Does this need to skip ignored objects, or does that take care of itself?

> Source/WebCore/accessibility/AccessibilityTableRow.cpp:107
> +	   // If this is a table object, but not an accessibility table, we
shouldn't 

Looks like you were halfway through writing this comment.