Bug 131796 – AX: Malformed tables exposing incorrect col and colSpans

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

[chris fleizach]

Check out the table in 
http://www.blindbargains.com/6thmode.php?m=10831 
with VoiceOver

Navigating through those elements, the last column is skipped, because colspans are malformed by the author

<rdar//problem/16437995>

[chris fleizach]

The fix appears to be to use effectiveCols, available on RenderTableCell

[chris fleizach]

Created attachment 229555 [details]
patch

[Matthew Hanson]

<rdar://problem/16437995>

[Darin Adler]

Comment on attachment 229555 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229555&action=review

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:251
>      RenderTableCell* renderCell = toRenderTableCell(m_renderer);

I’d suggest using a reference rather than a pointer for this local variable. And I would name it cell. I don’t think a “render” prefix adds anything.

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:256
> +    RenderTable* table = renderCell->table();
> +    if (!table)
> +        return;

Can this ever be null? In practice I don’t think it can, so the code above doesn’t matter. I was going to criticize sometimes returning col/colSpan directly from the element, but I’m not sure it matters.

> Source/WebCore/accessibility/AccessibilityTableCell.cpp:258
> +    columnRange.first = table->colToEffCol(columnRange.first);

I think it’s strange to use columnRange.first here instead of renderCell->col(). Makes the code harder to read for me.

[chris fleizach]

(In reply to comment #4)
> (From update of attachment 229555 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229555&action=review
> 
> > Source/WebCore/accessibility/AccessibilityTableCell.cpp:251
> >      RenderTableCell* renderCell = toRenderTableCell(m_renderer);
> 
> I’d suggest using a reference rather than a pointer for this local variable. And I would name it cell. I don’t think a “render” prefix adds anything.
> 
> > Source/WebCore/accessibility/AccessibilityTableCell.cpp:256
> > +    RenderTable* table = renderCell->table();
> > +    if (!table)
> > +        return;
> 
> Can this ever be null? In practice I don’t think it can, so the code above doesn’t matter. I was going to criticize sometimes returning col/colSpan directly from the element, but I’m not sure it matters.
> 
> > Source/WebCore/accessibility/AccessibilityTableCell.cpp:258
> > +    columnRange.first = table->colToEffCol(columnRange.first);
> 
> I think it’s strange to use columnRange.first here instead of renderCell->col(). Makes the code harder to read for me.

Thanks. I'll make all these changes

[chris fleizach]

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