Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(33)

Issue 914233002: Use effective columns when cells with colspan are present. (Closed)

Created:
5 years, 10 months ago by k.czech
Modified:
5 years, 10 months ago
Reviewers:
dmazzoni
CC:
blink-reviews, aboxhall
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Use effective columns when cells with colspan are present. Colspan brings some mess when cells for particular coordinates are returned. Effective column's conversion should be used to make sure we operate on exact column. Let's consider a very simple test case: <table><tr><td colspan="10"></td><th></th></tr></table>. Currently cellForColumnAndRow(1, 0) returns a first cell not the second. Basically columnIndexRange produces wrong information {0, 10} instead of {0, 1} which is wrong. Effective columns are fixing this issue. BUG=457687 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190160 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190442

Patch Set 1 #

Patch Set 2 : Just updating expectation result #

Total comments: 1

Patch Set 3 : Fixed typo and added effective col's calculation to column's headerObject method #

Patch Set 4 : Second attempt to land this patch #

Unified diffs Side-by-side diffs Delta from patch set Stats (+96 lines, -5 lines) Patch
A LayoutTests/accessibility/table-cells-with-colspan.html View 1 chunk +64 lines, -0 lines 0 comments Download
A LayoutTests/accessibility/table-cells-with-colspan-expected.txt View 1 1 chunk +27 lines, -0 lines 0 comments Download
M Source/modules/accessibility/AXTableCell.cpp View 1 chunk +2 lines, -2 lines 0 comments Download
M Source/modules/accessibility/AXTableColumn.cpp View 1 2 2 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 19 (5 generated)
k.czech
5 years, 10 months ago (2015-02-11 16:22:25 UTC) #2
dmazzoni
lgtm https://codereview.chromium.org/914233002/diff/20001/Source/modules/accessibility/AXTableCell.cpp File Source/modules/accessibility/AXTableCell.cpp (right): https://codereview.chromium.org/914233002/diff/20001/Source/modules/accessibility/AXTableCell.cpp#newcode199 Source/modules/accessibility/AXTableCell.cpp:199: columnRange.first = cell->table()->colToEffCol(cell->col()); Do we need to null-check ...
5 years, 10 months ago (2015-02-13 07:07:34 UTC) #3
k.czech
On 2015/02/13 07:07:34, dmazzoni wrote: > lgtm > > https://codereview.chromium.org/914233002/diff/20001/Source/modules/accessibility/AXTableCell.cpp > File Source/modules/accessibility/AXTableCell.cpp (right): > ...
5 years, 10 months ago (2015-02-13 09:29:14 UTC) #4
k.czech
On 2015/02/13 09:29:14, k.czech wrote: > On 2015/02/13 07:07:34, dmazzoni wrote: > > lgtm > ...
5 years, 10 months ago (2015-02-13 12:03:18 UTC) #6
dmazzoni
lgtm
5 years, 10 months ago (2015-02-13 16:05:46 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/914233002/40001
5 years, 10 months ago (2015-02-13 16:06:01 UTC) #9
commit-bot: I haz the power
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=190160
5 years, 10 months ago (2015-02-13 18:51:38 UTC) #10
Stephen White
This is breaking content_browsertests DumpAccessibilityTreeTest.AccessibilityTableSpans on Mac, e.g.: [ RUN ] DumpAccessibilityTreeTest.AccessibilityTableSpans [40968:1799:0214/083528:4954423442682:WARNING:browser_accessibility_manager_mac.mm(139)] Unknown accessibility ...
5 years, 10 months ago (2015-02-14 17:36:27 UTC) #11
k.czech
On 2015/02/14 17:36:27, Stephen White wrote: > This is breaking content_browsertests > DumpAccessibilityTreeTest.AccessibilityTableSpans on Mac, ...
5 years, 10 months ago (2015-02-18 12:38:43 UTC) #13
k.czech
On 2015/02/18 12:38:43, k.czech wrote: > On 2015/02/14 17:36:27, Stephen White wrote: > > This ...
5 years, 10 months ago (2015-02-18 12:40:51 UTC) #14
k.czech
On 2015/02/18 12:40:51, k.czech wrote: > On 2015/02/18 12:38:43, k.czech wrote: > > On 2015/02/14 ...
5 years, 10 months ago (2015-02-18 13:30:37 UTC) #15
dmazzoni
lgtm OK to land again.
5 years, 10 months ago (2015-02-18 16:46:57 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/914233002/60001
5 years, 10 months ago (2015-02-18 16:47:32 UTC) #18
commit-bot: I haz the power
5 years, 10 months ago (2015-02-18 19:21:38 UTC) #19
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190442

Powered by Google App Engine
This is Rietveld 408576698