Bug 131796 - AX: Malformed tables exposing incorrect col and colSpans
Summary: AX: Malformed tables exposing incorrect col and colSpans
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-04-17 09:36 PDT by chris fleizach
Modified: 2014-04-19 18:26 PDT (History)
9 users (show)

See Also:


Attachments
patch (6.36 KB, patch)
2014-04-17 10:01 PDT, chris fleizach
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2014-04-17 09:36:16 PDT
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>
Comment 1 chris fleizach 2014-04-17 09:36:36 PDT
The fix appears to be to use effectiveCols, available on RenderTableCell
Comment 2 chris fleizach 2014-04-17 10:01:38 PDT
Created attachment 229555 [details]
patch
Comment 3 Matthew Hanson 2014-04-17 11:21:53 PDT
<rdar://problem/16437995>
Comment 4 Darin Adler 2014-04-19 14:32:33 PDT
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.
Comment 5 chris fleizach 2014-04-19 18:19:24 PDT
(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
Comment 6 chris fleizach 2014-04-19 18:26:47 PDT
http://trac.webkit.org/changeset/167558