Closed Bug 761853 Opened 8 years ago Closed 7 years ago

ARIA grid with rowgroup breaks table row/col counting and coordinates

Categories

(Core :: Disability Access APIs, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla18

People

(Reporter: Jamie, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(2 files, 1 obsolete file)

Attached file Test case.
In an ARIA grid which is *not* based on an HTML table, using role="rowgroup" breaks table row and column counting and coordinates.

Reported in NVDA issue ticket: http://www.nvda-project.org/ticket/2412

Steps to reproduce:
1. Open the attached test case.
2. Retrieve the accessible for the second grid (under the heading "g2").
3. Query the table for its row count; e.g. IAccessibleTable2::nRows.
Expected: 4
Actual: 1
4. Retrieve the accessible for the first column in the second row (containing "r1c1").
5. Query the cell for its row index; e.g. IAccessibleTableCell::rowIndex.
Expected: 1
Actual: 0

Note that the first grid (under the heading "g1") works correctly.

There is a test case in bug 525909 containing a rowgroup which works correctly. However, that test case is based on an HTML table. It seems this breaks if you don't use an HTML table as the base for the grid.
Blocks: aria
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #631657 - Flags: review?(trev.saunders)
Comment on attachment 631657 [details] [diff] [review]
patch

Review of attachment 631657 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/generic/ARIAGridAccessible.cpp
@@ +982,2 @@
>    if (!table)
>      return NS_OK;

removed
Attached patch patch2Splinter Review
fixed some issues
Attachment #631657 - Attachment is obsolete: true
Attachment #631657 - Flags: review?(trev.saunders)
Attachment #631905 - Flags: review?(trev.saunders)
Comment on attachment 631905 [details] [diff] [review]
patch2

>+    PRUint32 result = mFilterFunc(child);
>+    if (!(result & filters::eSkipSubtree)) {
>+      IteratorState* childState = new IteratorState(child, mState);
>       mState = childState;
>     }
>+
>+    if (result & filters::eMatch)
>+      return child;

wouldn't it make more sense to check if its a match first?

>+  // Look for rows inside rowgroup.
>+  if (role == roles::SECTION)
>+    return eSkip;

technically shouldn't we only do this one level deep, so you don't have two layers of rowgroups or something?

>--- /dev/null
>+++ b/accessible/src/generic/ARIAGridAccessible-inl.h
>@@ -0,0 +1,52 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla 

nit, vim mode line too please


Also I think I'm fine with this, but I tend to think a better approach would be to get rid of the eFlatNav bit of AccIterator as oyu do, but keep the filter funcs return bool, and add a new iterator RowIterator that will look at the kids of rowgroups for rows.
Attachment #631905 - Flags: review?(trev.saunders) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> Comment on attachment 631905 [details] [diff] [review]
> patch2
> 
> >+    PRUint32 result = mFilterFunc(child);
> >+    if (!(result & filters::eSkipSubtree)) {
> >+      IteratorState* childState = new IteratorState(child, mState);
> >       mState = childState;
> >     }
> >+
> >+    if (result & filters::eMatch)
> >+      return child;
> 
> wouldn't it make more sense to check if its a match first?

seems reasonable

> >+  // Look for rows inside rowgroup.
> >+  if (role == roles::SECTION)
> >+    return eSkip;
> 
> technically shouldn't we only do this one level deep, so you don't have two
> layers of rowgroups or something?

it's valid but that but not sure how to fit it into existing logic. it shouldn't hurt actually.

> Also I think I'm fine with this, but I tend to think a better approach would
> be to get rid of the eFlatNav bit of AccIterator as oyu do, but keep the
> filter funcs return bool, and add a new iterator RowIterator that will look
> at the kids of rowgroups for rows.

it seems less flexible taking into account we have more hierarchical iterators, GetSelectable for example.
> 
> > Also I think I'm fine with this, but I tend to think a better approach would
> > be to get rid of the eFlatNav bit of AccIterator as oyu do, but keep the
> > filter funcs return bool, and add a new iterator RowIterator that will look
> > at the kids of rowgroups for rows.
> 
> it seems less flexible taking into account we have more hierarchical
> iterators, GetSelectable for example.

I'm not sure I see the flexibility thing, but I can see it being a little redundant.  On the other hand it should be faster, and takes care of the issue of only looking for rows and rows as direct kids of rowgroups nicely.
(In reply to Trevor Saunders (:tbsaunde) from comment #6)

> I'm not sure I see the flexibility thing, but I can see it being a little
> redundant.  On the other hand it should be faster, and takes care of the
> issue of only looking for rows and rows as direct kids of rowgroups nicely.

I agree. If you don't mind then let's get back to it one day. I'm not sure what I like more.
This is giving me:

$ hg up -r tip
abort: case-folding collision between accessible/src/base/filters.cpp and accessible/src/base/Filters.cpp

Non-case sensitive filesystem OSes appear not to like having files hg renamed like this.

Please can someone back this out, since I cannot use my mercurial repo...
renamed to AccFilters, later I'll rename it to Filters, this should work, https://hg.mozilla.org/integration/mozilla-inbound/rev/a1cd7986d295
https://hg.mozilla.org/mozilla-central/rev/a1cd7986d295
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Verified fixed in Firefox 19.0a1 20121104030714.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.