Use effective columns when cells with colspan are present.

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

[k.czech, 2015-02-11 16:22:24]

k.czech@samsung.com changed reviewers:
+ dmazzoni@chromium.org

[dmazzoni, 2015-02-13 07:07:34]

lgtm

https://codereview.chromium.org/914233002/diff/20001/Source/modules/accessibi...
File Source/modules/accessibility/AXTableCell.cpp (right):

https://codereview.chromium.org/914233002/diff/20001/Source/modules/accessibi...
Source/modules/accessibility/AXTableCell.cpp:199: columnRange.first =
cell->table()->colToEffCol(cell->col());
Do we need to null-check any of these?

Please look through the code. For example, if cell->table() is guaranteed to be
non-null then this is fine.

[k.czech, 2015-02-13 09:29:14]

On 2015/02/13 07:07:34, dmazzoni wrote:
> lgtm
> 
>
https://codereview.chromium.org/914233002/diff/20001/Source/modules/accessibi...
> File Source/modules/accessibility/AXTableCell.cpp (right):
> 
>
https://codereview.chromium.org/914233002/diff/20001/Source/modules/accessibi...
> Source/modules/accessibility/AXTableCell.cpp:199: columnRange.first =
> cell->table()->colToEffCol(cell->col());
> Do we need to null-check any of these?
> 
> Please look through the code. For example, if cell->table() is guaranteed to
be
> non-null then this is fine.

Yes it's guaranteed to be non-null, otherwise toLayoutTable(...) will cause a
crash.

[k.czech, 2015-02-13 11:54:33]

New patchsets have been uploaded after l-g-t-m from dmazzoni@chromium.org

[k.czech, 2015-02-13 12:03:18]

On 2015/02/13 09:29:14, k.czech wrote:
> On 2015/02/13 07:07:34, dmazzoni wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/914233002/diff/20001/Source/modules/accessibi...
> > File Source/modules/accessibility/AXTableCell.cpp (right):
> > 
> >
>
https://codereview.chromium.org/914233002/diff/20001/Source/modules/accessibi...
> > Source/modules/accessibility/AXTableCell.cpp:199: columnRange.first =
> > cell->table()->colToEffCol(cell->col());
> > Do we need to null-check any of these?
> > 
> > Please look through the code. For example, if cell->table() is guaranteed to
> be
> > non-null then this is fine.
> 
> Yes it's guaranteed to be non-null, otherwise toLayoutTable(...) will cause a
> crash.

Fixed one typo, and added effective col's calculation to headerObjectForSection.
Could you take a final look.

[dmazzoni, 2015-02-13 16:05:46]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-02-13 16:05:46]

lgtm

[commit-bot: I haz the power, 2015-02-13 16:06:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/914233002/40001

[commit-bot: I haz the power, 2015-02-13 18:51:38]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190160

[Stephen White, 2015-02-14 17:36:27]

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 event: 9
[40968:1799:0214/083528:4954446947327:WARNING:browser_accessibility_manager_mac.mm(139)]
Unknown accessibility event: 9
Testing:
/Volumes/data/b/build/slave/Mac10_8_Tests/build/src/content/test/data/accessibility/html/table-spans.html
../../content/browser/accessibility/dump_accessibility_browsertest_base.cc:208:
Failure
Value of: is_different
Actual: true
Expected: false
* Line Expected
- ---- --------
1 AXWebArea AXTitle='Table example with rowspan and colspan'
2 ++AXTable
3 ++++AXRow AXIndex='0'
4 ++++++AXCell AXColumnIndexRange={"len":1,"loc":0}
AXRowIndexRange={"len":2,"loc":0}
5 ++++++++AXStaticText AXValue='AD'
*    6 ++++++AXCell AXColumnIndexRange={"len":2,"loc":1}
AXRowIndexRange={"len":1,"loc":0}
7 ++++++++AXStaticText AXValue='BC'
8 ++++AXRow AXIndex='1'
*    9 ++++++AXCell AXColumnIndexRange={"len":2,"loc":1}
AXRowIndexRange={"len":1,"loc":1}
10 ++++++++AXStaticText AXValue='EF'
11 ++++AXColumn AXIndex='0'
12 ++++AXColumn AXIndex='1'
13 ++++AXGroup
14 <-- End-of-file -->

Actual
------
AXWebArea AXTitle='Table example with rowspan and colspan'
++AXTable
++++AXRow AXIndex='0'
++++++AXCell AXColumnIndexRange={"len":1,"loc":0}
AXRowIndexRange={"len":2,"loc":0}
++++++++AXStaticText AXValue='AD'
++++++AXCell AXColumnIndexRange={"len":1,"loc":1}
AXRowIndexRange={"len":1,"loc":0}
++++++++AXStaticText AXValue='BC'
++++AXRow AXIndex='1'
++++++AXCell AXColumnIndexRange={"len":1,"loc":1}
AXRowIndexRange={"len":1,"loc":1}
++++++++AXStaticText AXValue='EF'
++++AXColumn AXIndex='0'
++++AXColumn AXIndex='1'
++++AXGroup
<-- End-of-file -->
[  FAILED  ] DumpAccessibilityTreeTest.AccessibilityTableSpans, where TypeParam
=  and GetParam() =  (299 ms)

http://build.chromium.org/p/chromium.webkit/builders/Mac10.8%20Tests/builds/1...

Reverted in https://src.chromium.org/viewvc/blink?revision=190183&view=revision
.

[k.czech, 2015-02-18 12:35:15]

New patchsets have been uploaded after l-g-t-m from dmazzoni@chromium.org

[k.czech, 2015-02-18 12:38:43]

On 2015/02/14 17:36:27, Stephen White wrote:
> 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 event: 9
>
[40968:1799:0214/083528:4954446947327:WARNING:browser_accessibility_manager_mac.mm(139)]
> Unknown accessibility event: 9
> Testing:
>
/Volumes/data/b/build/slave/Mac10_8_Tests/build/src/content/test/data/accessibility/html/table-spans.html
>
../../content/browser/accessibility/dump_accessibility_browsertest_base.cc:208:
> Failure
> Value of: is_different
> Actual: true
> Expected: false
> * Line Expected
> - ---- --------
> 1 AXWebArea AXTitle='Table example with rowspan and colspan'
> 2 ++AXTable
> 3 ++++AXRow AXIndex='0'
> 4 ++++++AXCell AXColumnIndexRange={"len":1,"loc":0}
> AXRowIndexRange={"len":2,"loc":0}
> 5 ++++++++AXStaticText AXValue='AD'
> *    6 ++++++AXCell AXColumnIndexRange={"len":2,"loc":1}
> AXRowIndexRange={"len":1,"loc":0}
> 7 ++++++++AXStaticText AXValue='BC'
> 8 ++++AXRow AXIndex='1'
> *    9 ++++++AXCell AXColumnIndexRange={"len":2,"loc":1}
> AXRowIndexRange={"len":1,"loc":1}
> 10 ++++++++AXStaticText AXValue='EF'
> 11 ++++AXColumn AXIndex='0'
> 12 ++++AXColumn AXIndex='1'
> 13 ++++AXGroup
> 14 <-- End-of-file -->
> 
> Actual
> ------
> AXWebArea AXTitle='Table example with rowspan and colspan'
> ++AXTable
> ++++AXRow AXIndex='0'
> ++++++AXCell AXColumnIndexRange={"len":1,"loc":0}
> AXRowIndexRange={"len":2,"loc":0}
> ++++++++AXStaticText AXValue='AD'
> ++++++AXCell AXColumnIndexRange={"len":1,"loc":1}
> AXRowIndexRange={"len":1,"loc":0}
> ++++++++AXStaticText AXValue='BC'
> ++++AXRow AXIndex='1'
> ++++++AXCell AXColumnIndexRange={"len":1,"loc":1}
> AXRowIndexRange={"len":1,"loc":1}
> ++++++++AXStaticText AXValue='EF'
> ++++AXColumn AXIndex='0'
> ++++AXColumn AXIndex='1'
> ++++AXGroup
> <-- End-of-file -->
> [  FAILED  ] DumpAccessibilityTreeTest.AccessibilityTableSpans, where
TypeParam
> =  and GetParam() =  (299 ms)
> 
>
http://build.chromium.org/p/chromium.webkit/builders/Mac10.8%20Tests/builds/1...
> 
> Reverted in
https://src.chromium.org/viewvc/blink?revision=190183&view=revision
> .

I already disable this test. This patch can land again.

[k.czech, 2015-02-18 12:40:51]

On 2015/02/18 12:38:43, k.czech wrote:
> On 2015/02/14 17:36:27, Stephen White wrote:
> > 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 event: 9
> >
>
[40968:1799:0214/083528:4954446947327:WARNING:browser_accessibility_manager_mac.mm(139)]
> > Unknown accessibility event: 9
> > Testing:
> >
>
/Volumes/data/b/build/slave/Mac10_8_Tests/build/src/content/test/data/accessibility/html/table-spans.html
> >
>
../../content/browser/accessibility/dump_accessibility_browsertest_base.cc:208:
> > Failure
> > Value of: is_different
> > Actual: true
> > Expected: false
> > * Line Expected
> > - ---- --------
> > 1 AXWebArea AXTitle='Table example with rowspan and colspan'
> > 2 ++AXTable
> > 3 ++++AXRow AXIndex='0'
> > 4 ++++++AXCell AXColumnIndexRange={"len":1,"loc":0}
> > AXRowIndexRange={"len":2,"loc":0}
> > 5 ++++++++AXStaticText AXValue='AD'
> > *    6 ++++++AXCell AXColumnIndexRange={"len":2,"loc":1}
> > AXRowIndexRange={"len":1,"loc":0}
> > 7 ++++++++AXStaticText AXValue='BC'
> > 8 ++++AXRow AXIndex='1'
> > *    9 ++++++AXCell AXColumnIndexRange={"len":2,"loc":1}
> > AXRowIndexRange={"len":1,"loc":1}
> > 10 ++++++++AXStaticText AXValue='EF'
> > 11 ++++AXColumn AXIndex='0'
> > 12 ++++AXColumn AXIndex='1'
> > 13 ++++AXGroup
> > 14 <-- End-of-file -->
> > 
> > Actual
> > ------
> > AXWebArea AXTitle='Table example with rowspan and colspan'
> > ++AXTable
> > ++++AXRow AXIndex='0'
> > ++++++AXCell AXColumnIndexRange={"len":1,"loc":0}
> > AXRowIndexRange={"len":2,"loc":0}
> > ++++++++AXStaticText AXValue='AD'
> > ++++++AXCell AXColumnIndexRange={"len":1,"loc":1}
> > AXRowIndexRange={"len":1,"loc":0}
> > ++++++++AXStaticText AXValue='BC'
> > ++++AXRow AXIndex='1'
> > ++++++AXCell AXColumnIndexRange={"len":1,"loc":1}
> > AXRowIndexRange={"len":1,"loc":1}
> > ++++++++AXStaticText AXValue='EF'
> > ++++AXColumn AXIndex='0'
> > ++++AXColumn AXIndex='1'
> > ++++AXGroup
> > <-- End-of-file -->
> > [  FAILED  ] DumpAccessibilityTreeTest.AccessibilityTableSpans, where
> TypeParam
> > =  and GetParam() =  (299 ms)
> > 
> >
>
http://build.chromium.org/p/chromium.webkit/builders/Mac10.8%20Tests/builds/1...
> > 
> > Reverted in
> https://src.chromium.org/viewvc/blink?revision=190183&view=revision
> > .
> 
> I already disable this test. This patch can land again.

Disabled test: https://codereview.chromium.org/929023002/

[k.czech, 2015-02-18 13:30:37]

On 2015/02/18 12:40:51, k.czech wrote:
> On 2015/02/18 12:38:43, k.czech wrote:
> > On 2015/02/14 17:36:27, Stephen White wrote:
> > > 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 event: 9
> > >
> >
>
[40968:1799:0214/083528:4954446947327:WARNING:browser_accessibility_manager_mac.mm(139)]
> > > Unknown accessibility event: 9
> > > Testing:
> > >
> >
>
/Volumes/data/b/build/slave/Mac10_8_Tests/build/src/content/test/data/accessibility/html/table-spans.html
> > >
> >
>
../../content/browser/accessibility/dump_accessibility_browsertest_base.cc:208:
> > > Failure
> > > Value of: is_different
> > > Actual: true
> > > Expected: false
> > > * Line Expected
> > > - ---- --------
> > > 1 AXWebArea AXTitle='Table example with rowspan and colspan'
> > > 2 ++AXTable
> > > 3 ++++AXRow AXIndex='0'
> > > 4 ++++++AXCell AXColumnIndexRange={"len":1,"loc":0}
> > > AXRowIndexRange={"len":2,"loc":0}
> > > 5 ++++++++AXStaticText AXValue='AD'
> > > *    6 ++++++AXCell AXColumnIndexRange={"len":2,"loc":1}
> > > AXRowIndexRange={"len":1,"loc":0}
> > > 7 ++++++++AXStaticText AXValue='BC'
> > > 8 ++++AXRow AXIndex='1'
> > > *    9 ++++++AXCell AXColumnIndexRange={"len":2,"loc":1}
> > > AXRowIndexRange={"len":1,"loc":1}
> > > 10 ++++++++AXStaticText AXValue='EF'
> > > 11 ++++AXColumn AXIndex='0'
> > > 12 ++++AXColumn AXIndex='1'
> > > 13 ++++AXGroup
> > > 14 <-- End-of-file -->
> > > 
> > > Actual
> > > ------
> > > AXWebArea AXTitle='Table example with rowspan and colspan'
> > > ++AXTable
> > > ++++AXRow AXIndex='0'
> > > ++++++AXCell AXColumnIndexRange={"len":1,"loc":0}
> > > AXRowIndexRange={"len":2,"loc":0}
> > > ++++++++AXStaticText AXValue='AD'
> > > ++++++AXCell AXColumnIndexRange={"len":1,"loc":1}
> > > AXRowIndexRange={"len":1,"loc":0}
> > > ++++++++AXStaticText AXValue='BC'
> > > ++++AXRow AXIndex='1'
> > > ++++++AXCell AXColumnIndexRange={"len":1,"loc":1}
> > > AXRowIndexRange={"len":1,"loc":1}
> > > ++++++++AXStaticText AXValue='EF'
> > > ++++AXColumn AXIndex='0'
> > > ++++AXColumn AXIndex='1'
> > > ++++AXGroup
> > > <-- End-of-file -->
> > > [  FAILED  ] DumpAccessibilityTreeTest.AccessibilityTableSpans, where
> > TypeParam
> > > =  and GetParam() =  (299 ms)
> > > 
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/Mac10.8%20Tests/builds/1...
> > > 
> > > Reverted in
> > https://src.chromium.org/viewvc/blink?revision=190183&view=revision
> > > .
> > 
> > I already disable this test. This patch can land again.
> 
> Disabled test: https://codereview.chromium.org/929023002/

@Dominic: any chance to re-land this patch ?

[dmazzoni, 2015-02-18 16:46:57]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-02-18 16:46:57]

lgtm

OK to land again.

[commit-bot: I haz the power, 2015-02-18 16:47:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/914233002/60001

[commit-bot: I haz the power, 2015-02-18 19:21:38]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190442