WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
131796
AX: Malformed tables exposing incorrect col and colSpans
https://bugs.webkit.org/show_bug.cgi?id=131796
Summary
AX: Malformed tables exposing incorrect col and colSpans
chris fleizach
Reported
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>
Attachments
patch
(6.36 KB, patch)
2014-04-17 10:01 PDT
,
chris fleizach
darin
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
chris fleizach
Comment 1
2014-04-17 09:36:36 PDT
The fix appears to be to use effectiveCols, available on RenderTableCell
chris fleizach
Comment 2
2014-04-17 10:01:38 PDT
Created
attachment 229555
[details]
patch
Matthew Hanson
Comment 3
2014-04-17 11:21:53 PDT
<
rdar://problem/16437995
>
Darin Adler
Comment 4
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.
chris fleizach
Comment 5
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
chris fleizach
Comment 6
2014-04-19 18:26:47 PDT
http://trac.webkit.org/changeset/167558
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug