Bug 115837

Summary: AX: VoiceOver is no longer seeing items in poorly formed tables
Product: WebKit Reporter: chris fleizach <cfleizach>
Component: AccessibilityAssignee: chris fleizach <cfleizach>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, commit-queue, dmazzoni, jdiggs, mario, thorton
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch darin: review+

Description chris fleizach 2013-05-08 17:41:12 PDT
If an ARIA table has intermediate nodes between it and its rows, then the table does not vend its rows or children correctly.
Comment 1 chris fleizach 2013-05-08 17:41:31 PDT
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>
Comment 2 chris fleizach 2013-05-08 17:51:24 PDT
Created attachment 201123 [details]
patch
Comment 3 chris fleizach 2013-05-08 17:51:41 PDT
Adding Tim to help with review
Comment 4 chris fleizach 2013-05-08 17:51:56 PDT
rdar://13687511
Comment 5 Darin Adler 2013-05-09 09:56:19 PDT
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.
Comment 6 chris fleizach 2013-05-09 17:36:03 PDT
(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.
Comment 7 chris fleizach 2013-05-09 17:57:16 PDT
http://trac.webkit.org/changeset/149858