Bug 57463

Changing the title because the current one because it was misleading: When I filed the bug I did not meant that rows/columns should be exposed for ARIA tables (as FF do), but that contents in those rows/columns (aka, the cells) should be exposed in a consistent way bearing in mind how HTML are exposed, that is, having all the a11y objects for the table's cells as direct children for the a11y object for the table. Furthermores, I had no idea whether exposing those rows/columns explicitly would be right or not when I filed the bug, but now that Joanmarie confirmed that they shouldn't be exposed I felt like mandatory to come here and change this bug's title. Sorry for the noise.

Created attachment 87839 [details] Patch proposal + Layout test Attaching patch proposal + skipping test that is now passing

Comment on attachment 87839 [details] Patch proposal + Layout test View in context: https://bugs.webkit.org/attachment.cgi?id=87839&action=review > Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:113 > + !child check is not needed > Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:115 > + if (appendedRows.contains(row)) this check could be in the previous check. there's no need to cast to a AccessibilityTableRow at this point > Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:119 > + unsigned rowCellCount = row->children().size(); Store instead of "store" > Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:123 > + row->setRowIndex((int)columnCount); static_cast also i don't think the rowIndex is the same as the columnCount. The row index should be m_rows.size() > Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:125 > + m_children.append(row->children()); why is all this logic duplicated from addChild. Is there any difference with what you added? > Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:450 > + case GridRole: i believe you removed this in another patch

(In reply to comment #3) > (From update of attachment 87839 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87839&action=review > > > Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:113 > > + > > !child check is not needed Ok > > Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:115 > > + if (appendedRows.contains(row)) > > this check could be in the previous check. there's no need to cast to a AccessibilityTableRow at this point Ok > > Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:119 > > + unsigned rowCellCount = row->children().size(); > > Store instead of "store" Ok, oops. > > Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:123 > > + row->setRowIndex((int)columnCount); > > static_cast > also i don't think the rowIndex is the same as the columnCount. The row index should be m_rows.size() That was a mistake. Thanks for the catch. > > Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:125 > > + m_children.append(row->children()); > > why is all this logic duplicated from addChild. Is there any difference with what you added? The main difference is that in addChild() your adding always rows (as it's claimed by the assertion in the first line), but here in this patch we're adding the row's children instead, since we don't want (for the time being at least) those row objects to be exposed to ATK, but just their children (the cells). That is, we want the a11y objects for the cells to be exposed as direct children of the a11y objects for the tables. No a11y objects for the rows in the middle, for consistency with GTK (and with what it's done for HTML tables). What is true is that perhaps I could refactor code in a better way, maybe through an extra parameter in addChild() so we avoid this code replication. > > Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:450 > > + case GridRole: > > i believe you removed this in another patch True, sorry. Now I'm running out of time but will try to work on fixing this patch asap, if possible before Monday. Thanks for the feedback

Created attachment 87933 [details] Patch proposal + Layout test Attaching a new patch, now avoiding to duplicate so much code as in the previous version. Behaviour for other platform should keep being the same than before the patch, hence only GTK should be affected. Have a nice weekend!

Created attachment 88068 [details] Patch proposal + Layout test Attaching a new patch adding some extra changes in AccessibilityARIAGridCell.cpp I realized would be needed while working in other stuff (trying to unskip some other tests by adding extra support in DRT). Summing up, this patch is like the previous one, but it adds some needed code for ARIA Grid cells to be aware, in GTK, about the fact that rows will be plainly ignored for this platform.

Comment on attachment 88068 [details] Patch proposal + Layout test ... and now I realized (while trying to unskip aria-tables.html) that this patch has still some problems so it shouldn't be reviewed yet. So sorry for the noise. Hope to come back soon with a good enough patch, but in the meantime lets remove the r? flag.

Created attachment 88199 [details] Patch proposal + Layout test Back on business... I think this one is the good one, for a variety of reasons: * It doesn't assume that GTK ignores accessibility for table rows (which is something happening right now, but could/should change in the future). Instead, it adds extra code for the GTK platform so it works as expected _in case_ it's ignoring accessibility for rows. This means that if in the future we decide not to ignore rows anymore, current code in AccessibilityGrid and AccessibilityGridCell should still work fine. * It works fine for different test cases I tried, inspired both in the aria-tables-hierarchy.html and aria-tables.html test cases. * I checked with Accerciser and there's currently no difference, from AT's point of view, between HTML and ARIA tables (which is what this bug is about). * It should keep the same behavior than before for other platforms (I carefully checked this by visually inspecting the diff). Of course, no sw/patch is free of bugs, but I think this patch is worth considering for review anyway... :)

Comment on attachment 88199 [details] Patch proposal + Layout test View in context: https://bugs.webkit.org/attachment.cgi?id=88199&action=review Is it possible to override what GTK returns for parent and children, and check if there's an AX table row in that equation, and if so, either pass on to the parent of the table row, or the children of the table row? I ask because I fear that there's a lot of edge cases that rely on a table row for calculating or retrieving data. A good example is rowIndexRange(). It seems the rowIndex() method is really good for that, but GTK code now has to do a lot of work to get that same information. In the Mac wrapper we do something similar when handling attachments. For children and parents, we check if there's an attachmentView and if there, we do something different. I also think that change may be a lot less code > Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:85 > + I don't think this change needs to be platform dependent. I think the accessibilityIsIgnored() check covers the platform dependentness.

Created attachment 88393 [details] Patch proposal + Layout test (In reply to comment #9) > (From update of attachment 88199 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88199&action=review > > Is it possible to override what GTK returns for parent and children, and > check if there's an AX table row in that equation, and if so, either pass > on to the parent of the table row, or the children of the table row? Problem is that at the moment in GTK accessibility objects for rows should ignore accessibility, thus they should not be part of the accessibility hierarchy, so I'm not sure how to do this by just overriding those functions. Perhaps I'm missing something, or not understanding your suggestion properly, though. > I ask because I fear that there's a lot of edge cases that rely on a table row > for calculating or retrieving data. A good example is rowIndexRange(). > It seems the rowIndex() method is really good for that, but GTK code > now has to do a lot of work to get that same information. Yeah, we need to do that extra calculations in GTK since accessible rows are not added to he hierarchy, so we can't use the API from AccessibilityTableRow. > In the Mac wrapper we do something similar when handling attachments. > For children and parents, we check if there's an attachmentView and > if there, we do something different. I've checked the code in the mac wrapper, but still not sure if it would be a similar case to what happens here: we just want to leave (and handle everything else accordingly) accessibility rows out from the a11y hierarchy, as it's done with regular HTML tables. But as I commented above, perhaps I'm not understanding properly your point, in which case I'd appreciate some further comments from your side, if possible. Thanks in advance. > I also think that change may be a lot less code I'm currently attaching a new patch which, even though it's not perhaps perfect either, it's a lot less code than the previous one, mainly because I removed all the "#if PLATFORM(GTK).. #endif" regions by writing all the changes as if having rows ignoring accessibility was not a matter of the GTK platform only, but something more general, and leaving the accessibilityIsIgnored() check to do the work of making the right decision. To tell the truth, I didn't like very much adding so much platform specific code in the previous patch, but I did it so in order not to touch at all the behavior of other platforms (that is, Mac), as I thought it could be a good trade off. But after reading the last line of your comment, I realized you wouldn't be as much opposed to a more general patch as I initially thought (my fault, I'm the king when it comes to making wrong assumptions) so I'm coming now with just the opposite thing: a patch with no "#if PLATFORM(GTK).. #endif" regions at all. Also, I've run the tests of the Mac port in a MacMini I have at hand, and it seems it doesn't break anything either, so I guess it couldn't be that bad option at the end :-) And I have proof of that... ** BUILD SUCCEEDED ** Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/mario/Source/WebKit/LayoutTests Testing 194 test cases. accessibility ...................................................................................... platform/mac/accessibility ............................................................................................................ 52.30s total testing time all 194 test cases succeeded > > Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:85 > > + > > I don't think this change needs to be platform dependent. I think the accessibilityIsIgnored() check covers the platform dependentness. ^---> This is the "last line of your comment" I was referring to above.

Comment on attachment 88393 [details] Patch proposal + Layout test View in context: https://bugs.webkit.org/attachment.cgi?id=88393&action=review r=me after you address comments. > Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:110 > + addChild(child.get(), appendedRows, columnCount); if the ariaRoleAttribute == RowRole, is it not always isTableRow() = true? > Source/WebCore/accessibility/AccessibilityARIAGridCell.cpp:64 > + return 0; seems like this check is not necessary since when we ask for the parent unignored, we'll check again for accessibilityTable() a few lines below. I would however add a comment why we need to check parentObjectUnignored(), and the parentObjectUnignored() again > Source/WebCore/accessibility/AccessibilityARIAGridCell.cpp:90 > + for (unsigned k = 0; k < siblings.size(); ++k) { don't call .size() in the for loop (that means you'll call a method for every iteration > Source/WebCore/accessibility/AccessibilityARIAGridCell.cpp:93 > + break; i should note that if ARIA grid cells do have span columns, this code will be invalid

Comment on attachment 88393 [details] Patch proposal + Layout test View in context: https://bugs.webkit.org/attachment.cgi?id=88393&action=review I'll wait for you to answer my question above (about the double check) before actually committing anything. Thanks! >> Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:110 >> + addChild(child.get(), appendedRows, columnCount); > > if the ariaRoleAttribute == RowRole, is it not always isTableRow() = true? Well, the definition of isTableRow() is as follows: bool AccessibilityTableRow::isTableRow() const { AccessibilityObject* table = parentTable(); if (!table || !table->isAccessibilityTable()) return false; return true; } So my quick answer is "yes, it should be the same", but still I think I'd rather leave the double check to make sure we're actually doing the right thing, since ariaRoleAttribute() just checks an attribute, while isTableRow() checks that the parent object is a table, and I wonder whether those two methods could end up "disagreeing" under certain circunstances on whether something is a row or not. Besides, I took the idea of the first check in AccessibilityARIAGrid::addChild(), so that's where my doubts come from... So what do you think? Shall we change it or leave it as it is now (double check)? >> Source/WebCore/accessibility/AccessibilityARIAGridCell.cpp:64 >> + return 0; > > seems like this check is not necessary since when we ask for the parent unignored, we'll check again for accessibilityTable() a few lines below. > I would however add a comment why we need to check parentObjectUnignored(), and the parentObjectUnignored() again Yes, you're right. I just tried to keep the same logic than before my patch as much as possible, but I agree that it's not needed to make this check now having the second isAccessibilityTable() check right after getting the unignored parent for the second time. Agreed on adding the comment as well. >> Source/WebCore/accessibility/AccessibilityARIAGridCell.cpp:90 >> + for (unsigned k = 0; k < siblings.size(); ++k) { > > don't call .size() in the for loop (that means you'll call a method for every iteration Opps. Yes, sorry about that. >> Source/WebCore/accessibility/AccessibilityARIAGridCell.cpp:93 >> + break; > > i should note that if ARIA grid cells do have span columns, this code will be invalid Yes, you're right. But in theory ARIA grids do not have span columns, right? Or at least that's what I thought... I'll double check that, just in case

Created attachment 89018 [details] Patch proposal + Layout test Chris, According to the answers to your questions I wrote in my previous patch, this would be the patch I'd be committing now, if you agree.

Comment on attachment 89018 [details] Patch proposal + Layout test r=me

Committed r83450: <http://trac.webkit.org/changeset/83450>