Bug 115837 – AX: VoiceOver is no longer seeing items in poorly formed tables

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

[chris fleizach]

If an ARIA table has intermediate nodes between it and its rows, then the table does not vend its rows or children correctly.

[chris fleizach]

Like this kind of table

<div role="grid" id="grid">
  <div role="application">
     <div role="application">
         <div role="row" id="row1">
            <div role="gridcell">1</div>
         </div>
         <div role="text">a</div>
         <div role="row" id="row2">
            <div role="gridcell">1</div>
         </div>
     </div>
  </div>
</div>

[chris fleizach]

Created attachment 201123 [details]
patch

[chris fleizach]

Adding Tim to help with review

[chris fleizach]

rdar://13687511

[Darin Adler]

Comment on attachment 201123 [details]
patch

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.

[chris fleizach]

(In reply to comment #5)
> (From update of attachment 201123 [details])
> 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?

Fixed

> 
> > 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.
>
Fixed
 
> 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.

Agreed. I'll revisit in another bug

> 
> 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?
> 

That takes care of itself by asking if it's an accessibilityTable. parentObjectUnignored() will do the same thing as this method (run up the parent chain), but has the added cost of calling accessibilityIsIgnored() on every object.

> > 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.

Thanks fixed.

[chris fleizach]

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