Bug 136415

Created attachment 258285 [details] patch

Created attachment 258290 [details] patch fixed the test

so does this patch make the table inaccessible completely? i think what we want to do is make sure that row 2 and column 2 actually appear in the table in the right form (rather than making it inaccessible or making the whole table inaccessible) when i test with the test case, VO cannot navigate to column 2 or row 2. I think we want to make sure that that works rather than marking things ignored

(In reply to comment #3) > so does this patch make the table inaccessible completely? > > i think what we want to do is make sure that row 2 and column 2 actually > appear in the table in the right form (rather than making it inaccessible or > making the whole table inaccessible) > > when i test with the test case, VO cannot navigate to column 2 or row 2. I > think we want to make sure that that works rather than marking things ignored The fix makes the contents in the table accessible. The CSS display style has changed the table hierarchy to be only one row and one cell, and our rows and cells become children of that cell. So which part of the project should I look at, that it modifies the hierarchy based on CSS?

(In reply to comment #4) > (In reply to comment #3) > > so does this patch make the table inaccessible completely? > > > > i think what we want to do is make sure that row 2 and column 2 actually > > appear in the table in the right form (rather than making it inaccessible or > > making the whole table inaccessible) > > > > when i test with the test case, VO cannot navigate to column 2 or row 2. I > > think we want to make sure that that works rather than marking things ignored > > The fix makes the contents in the table accessible. The CSS display style > has changed the table hierarchy to be only one row and one cell, and our > rows and cells become children of that cell. > > So which part of the project should I look at, that it modifies the > hierarchy based on CSS? Not sure, let me see the results of this patch first

(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > so does this patch make the table inaccessible completely? > > > > > > i think what we want to do is make sure that row 2 and column 2 actually > > > appear in the table in the right form (rather than making it inaccessible or > > > making the whole table inaccessible) > > > > > > when i test with the test case, VO cannot navigate to column 2 or row 2. I > > > think we want to make sure that that works rather than marking things ignored > > > > The fix makes the contents in the table accessible. The CSS display style > > has changed the table hierarchy to be only one row and one cell, and our > > rows and cells become children of that cell. > > > > So which part of the project should I look at, that it modifies the > > hierarchy based on CSS? > > Not sure, let me see the results of this patch first Ok, I'll keep investigating.

Another approach, when the AccessibilityTable object is adding children, once we see an invalid row (Row-Cell-Rows) hierarchy, we ignore the invalid row and add the correct rows(the children of cell) instead. Working on the approach and layout test right now.

Created attachment 258318 [details] patch Patch with new approach and fixed the issue that table is exposed as AXGrid instead of AXTable. Updated test expectations as well.

Comment on attachment 258318 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=258318&action=review > Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:97 > + // CSS display style can change the table hierarchy and cause the table to be malformed. how does CSS display change the table hierarchy? it's a strange thing because the DOM looks ok > Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:103 > + if (is<AccessibilityTableRow>(*cellChild)) { should this be more generic? why just table rows? what if we had a group -> group > row would we catch that? is there something specific about this CSS variation that always creates this specific render tree hierarchy? > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:-1921 > - { GridRole, NSAccessibilityGridRole }, what was the bug that this changed with? it seems correct to keep it as TableRole, but wondering how it broke > LayoutTests/accessibility/mac/malformed-table.html:49 > + shouldBe("columnCount", "2"); we need to check cellForColumnAndRow we need get the elements out of the table through it's rows method, it's cells method and children method and verify that the role is correct for each child in those cases

Comment on attachment 258318 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=258318&action=review >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:-1921 >> - { GridRole, NSAccessibilityGridRole }, > > what was the bug that this changed with? it seems correct to keep it as TableRole, but wondering how it broke I think in https://bugs.webkit.org/show_bug.cgi?id=146011, I changed element with "grid" role to use GridRole. Before that "grid" is mapped to TableRole.

Comment on attachment 258318 [details] patch Attachment 258318 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/20603 New failing tests: accessibility/roles-exposed.html

Created attachment 258323 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

(In reply to comment #10) > Comment on attachment 258318 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=258318&action=review > > >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:-1921 > >> - { GridRole, NSAccessibilityGridRole }, > > > > what was the bug that this changed with? it seems correct to keep it as TableRole, but wondering how it broke > > I think in https://bugs.webkit.org/show_bug.cgi?id=146011, I changed element > with "grid" role to use GridRole. Before that "grid" is mapped to TableRole. sad I let this slip through What we want is to map aria "grid" -> WebCore::GridRole, but then map WebCore::GridRole to NSAccessibilityTableRole that way, in the accessibility node inspector it will say "grid" but for the mac platform it says "table"

Created attachment 258382 [details] patch

Comment on attachment 258382 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=258382&action=review > Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:91 > + if (!rowChild->isTableRow() || (rowChild->renderer() && rowChild->renderer()->isAnonymous())) { shouldn't this check be in isTableRow()

Comment on attachment 258382 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=258382&action=review >> Source/WebCore/accessibility/AccessibilityARIAGrid.cpp:91 >> + if (!rowChild->isTableRow() || (rowChild->renderer() && rowChild->renderer()->isAnonymous())) { > > shouldn't this check be in isTableRow() I tested with a case that table with CSS and no ARIA, and that made the table totally inaccessible by returning no rows. I think in AccessibilityTable::addChildren(), is<AccessibilityTableRow> is using the isTableRow() check. And the addChildren() logic is different from AccessibilityARIAGrid::addChildren(), so I'm little hesitate to break that logic.

Created attachment 258521 [details] patch Fixed an issue that when CSS is added to a table with no ARIA roles, it's not getting the correct column and row information.

Comment on attachment 258521 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=258521&action=review > Source/WebCore/ChangeLog:9 > + CSS table display styles sometime can change the hierarchy of a table, making child objects to be anonymous. Apply CSS display styles to tables can end up inserting anonymous RenderTableRows, which is not handled well by the accessibility code, which treats these as the actual rows. We can address this by diving deeper into anonymous nodes and finding the real rows and cells we want. In addition, another thing also causing malformed tables is that "grid" roles are being exposed as AXGrid instead of AXTable. > Source/WebCore/accessibility/AccessibilityTable.cpp:480 > continue; instead of having a continue here and then the other case below, i think it should be if/else which should make it clearer to understand that we have two cases > Source/WebCore/accessibility/AccessibilityTableCell.cpp:88 > + // The RenderTableCell's table() object might be anonymous sometimes. We should handle it gracefully This new code seems to ignore the perils of the comment above. I think we should do a check for the renderTable if it's anonymous, then go up the render->parent() chain until we find a non anonymous RenderTable parent. then do the original logic (and leave the original comment) > Source/WebCore/accessibility/AccessibilityTableRow.cpp:107 > + if (parentTable.node() != nullptr) this should be if (parentTable.node()) break;

Created attachment 258591 [details] patch

Comment on attachment 258591 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=258591&action=review Just some minor comments. Can you upload a patch to address those small comments? Thanks > Source/WebCore/ChangeLog:9 > + Apply CSS display styles to tables can end up inserting anonymous RenderTableRows, which is not handled well by the Apply-> Applying > Source/WebCore/accessibility/AccessibilityTableCell.cpp:97 > + // The RenderTableCell's table() object might be anonymous sometimes. We should handle it gracefully End the sentence with a period > LayoutTests/accessibility/mac/malformed-table.html:39 > + <th>Heading one</th> It looks like there are some tabs in here instead of spaces like the rest of the file

Comment on attachment 258591 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=258591&action=review >> Source/WebCore/accessibility/AccessibilityTableCell.cpp:97 >> + // The RenderTableCell's table() object might be anonymous sometimes. We should handle it gracefully > > End the sentence with a period The sentence is not finished, there's another line below. >> LayoutTests/accessibility/mac/malformed-table.html:39 >> + <th>Heading one</th> > > It looks like there are some tabs in here instead of spaces like the rest of the file Ok

Created attachment 258596 [details] patch

Comment on attachment 258596 [details] patch Clearing flags on attachment: 258596 Committed r188203: <http://trac.webkit.org/changeset/188203>

All reviewed patches have been landed. Closing bug.

<rdar://problem/22026625>

This bug depends on bug 146011.

Created attachment 258749 [details] test case