Bug 136415 – AX: CSS table display styles can cause malformed, inaccessible AXTables to be exposed to the AX tree

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[James Craig]

AX: CSS table display styles can cause malformed, inaccessible AXTables to be exposed to the AX tree

<rdar://problem/18188694>

[Nan Wang]

Created attachment 258285 [details]
patch

[Nan Wang]

Created attachment 258290 [details]
patch

fixed the test

[chris fleizach]

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

[Nan Wang]

(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?

[chris fleizach]

(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

[Nan Wang]

(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.

[Nan Wang]

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.

[Nan Wang]

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.

[chris fleizach]

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

[Nan Wang]

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.

[Build Bot]

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

[Build Bot]

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

[chris fleizach]

(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"

[Nan Wang]

Created attachment 258382 [details]
patch

[chris fleizach]

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()

[Nan Wang]

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.

[Nan Wang]

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.

[chris fleizach]

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;

[Nan Wang]

Created attachment 258591 [details]
patch

[chris fleizach]

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

[Nan Wang]

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

[Nan Wang]

Created attachment 258596 [details]
patch

[WebKit Commit Bot]

Comment on attachment 258596 [details]
patch

Clearing flags on attachment: 258596

Committed r188203: <http://trac.webkit.org/changeset/188203>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[James Craig]

<rdar://problem/22026625>

[James Craig]

This bug depends on bug 146011.

[James Craig]

Created attachment 258749 [details]
test case