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.

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

fixed some issues

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.

(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

Verified fixed in Firefox 19.0a1 20121104030714.