Bug 62335

The problem is how rows of an ARIA grid are determined. The mentioned bug was strangely and wrongly only adding children 1 level deep in the hierarchy. This means that it doesn't look any farther for table rows.

Created attachment 96501 [details] patch

Comment on attachment 96501 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=96501&action=review r=me with the unsigned => size_t changes. > Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:119 > + unsigned length = children.size(); This should be size_t instead of unsigned. > Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:120 > + for (unsigned i = 0; i < length; ++i) unsigned => size_t

fix the leopard test http://trac.webkit.org/changeset/88843

I see a GTK test is failing as well. May need Mario's help to fix that one AXRole: table AXRole: table AXRole: table cell +AXRole: unknown AXRole: table cell -AXRole: table cell -AXRole: table cell +AXRole: unknown Test passed

rdar://9600922. Also affects the desktop

It seems like the test is still faling on Leopard. http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r88877%20(33189)/results.html Could you look into it?

(In reply to comment #9) > It seems like the test is still faling on Leopard. > http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r88877%20(33189)/results.html > > Could you look into it? looking into why my skipped list change didn't work

Fixed for real http://trac.webkit.org/changeset/88887

(In reply to comment #6) > I see a GTK test is failing as well. May need Mario's help to fix that one > > AXRole: table > AXRole: table > AXRole: table cell > +AXRole: unknown > AXRole: table cell > -AXRole: table cell > -AXRole: table cell > +AXRole: unknown > Test passed It seems the problem in GTK with this patch is that the addChild() method expects a row object with the cells as its children, and doesn't care initially whether that row object is ignoring a11y or not: it will just consider that later on, when deciding whether to add the row object itself to the hierarchy, or just the raw list of children: bool AccessibilityARIAGrid::addChild(AccessibilityObject* child, HashSet<AccessibilityObject*>& appendedRows, unsigned& columnCount) { if (!child || !child->isTableRow() || child->ariaRoleAttribute() != RowRole) return false; [...] // Try adding the row if it's not ignoring accessibility, // otherwise add its children (the cells) as the grid's children. if (!row->accessibilityIsIgnored()) m_children.append(row); else m_children.append(row->children()); appendedRows.add(row); return true; } Thus, when we do the following in AccessibilityARIAGrid::addChildren()... // The children of this non-row will contain all non-ignored elements (recursing to find them). // This allows the table to dive arbitrarily deep to find the rows. AccessibilityChildrenVector children = child->children(); unsigned length = children.size(); for (unsigned i = 0; i < length; ++i) addChild(children[i].get(), appendedRows, columnCount); } ...nothing is being added to the hierarchy in GTK, because navigating through this AccessibilityChildrenVector, as Chris explained in the comment, will mean navigating through non-ignored elements only, which in GTK means just the cells, as rows won't be exposed in that platform (yest, weird, but at the moment it's like that, will change in the future, though [1][2]). So, the problem I see with this patch, from GTK's selfish point of view, is that it assumes that rows are not being ignored, which is right for the Mac, but not for GTK at this point. My proposal to keep this regression fixed while still keep all the parties happy, is just to combine the code before and after this patch with #if - #endif regions, doing something like this: #if PLATFORM(GTK) // Do not navigate children through the Accessibility children vector to let addChild() check // the result of accessibilityIsIgnored() and make the proper decision. AccessibilityObject* grandChild = 0; for (grandChild = child->firstChild(); grandChild; grandChild = grandChild->nextSibling()) addChild(grandChild, appendedRows, columnCount); #else // The children of this non-row will contain all non-ignored elements (recursing to find them). // This allows the table to dive arbitrarily deep to find the rows. AccessibilityChildrenVector children = child->children(); size_t length = children.size(); for (size_t i = 0; i < length; ++i) addChild(children[i].get(), appendedRows, columnCount); #endif Why I propose this ugly thing? Well, because after all this hack is needed because of a very specific requirement in GTK (not exposing rows) that is needed at the moment, but that was already decided it will change in the future (see ATK hackfest agenda and conclusions in [1] and [2]), so when that happens it will be just a matter of removing this #if PLATFORM(GTK) - #endif region and just leaving the generic one Chris wrote, and that should work for all the parties. As this seems to be a too-specific thing for GTK, I will file a new bug and propose this patch there, keeping the test skipped while it's not fixed. [1] https://live.gnome.org/Hackfests/ATK2011/Agenda [2] https://live.gnome.org/Hackfests/ATK2011/Agenda/table

(In reply to comment #12) > [...] > As this seems to be a too-specific thing for GTK, I will file a new bug and propose this patch there, keeping the test skipped while it's not fixed. Filed bug 62718 for tracking down this issue. Test skipped in the meanwhile: http://trac.webkit.org/changeset/88919