AX: iframe within table cell is inaccessible to VoiceOver

[Nan Wang]

Created attachment 256907 [details]
a testcase showing that VO will not step into the cell with iframe

The issue is that the table row has an AXGroup child and not an AXCell as we expect

[Nan Wang]

<rdar://problem/21106945>

[Nan Wang]

Created attachment 256941 [details]
patch

[chris fleizach]

Comment on attachment 256941 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256941&action=review

make sure you add r? if you want a review. you should always do that before adding cq?

> Source/WebCore/ChangeLog:9
> +        Update each table cell's role once the table adds children.

change this comment to one that explains the problem and explains the solution, succinctly.

"When a table cell is created before its parent table determines if it should be ignored or not, the table cell may cache the wrong role.
Fix that allowing each table cell to update its role after the table makes this determination."

> Source/WebCore/accessibility/AccessibilityTable.cpp:413
> +    // Sometimes the cell gets wrong role that it thinks the parent table is

--> change to : 
"Sometimes the cell gets the wrong role initially because it is created before the parent determines whether it is an accessibility table.
Iterate all the cells and allow them to update their roles now that the table knows its status."

> Source/WebCore/accessibility/AccessibilityTable.cpp:417
> +    if (isExposableThroughAccessibility()) {

we already check this at the top of this method, so this is not necessary to call again

> Source/WebCore/accessibility/AccessibilityTable.cpp:418
> +        for (const auto& row : children()) {

you can just integrate through m_rows and then you don't have to check row->roleValue()

> Source/WebCore/accessibility/AccessibilityTable.cpp:422
> +                AccessibilityObject* cellObj = cell.get();

doesn't seem necessary to create this local variable

> LayoutTests/ChangeLog:9
> +        Test with iframe within table cell, expect to get the correct cell role as AXCell.

Generally no comment is needed for a layout test that comes with a bug fix, so i would remove this line

> LayoutTests/accessibility/iframe-within-cell.html:14
> +		document.body.focus();

don't use tabs. only use 4 spaces

> LayoutTests/accessibility/iframe-within-cell.html:17
> +		result.innerText += "Role for iframe cell is:  " + frameRole + " .\n";

you should use either
debug(message)

or 

shouldBe("frame.role", "'AXRole: AXCell'");

instead of appending text to the result div manually

[Nan Wang]

Created attachment 256957 [details]
new patch

fix that based on previous comments

[chris fleizach]

Comment on attachment 256957 [details]
new patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256957&action=review

> Source/WebCore/ChangeLog:10
> +        the table cell may cache the wrong role. Fix that allowing each table cell to update its role

Fix that "by" allowing

> Source/WebCore/accessibility/AccessibilityTable.cpp:418
> +        if (row->roleValue() != RowRole)

no need to check the role value since you're iterating rows and you know all the things are rows

> Source/WebCore/accessibility/AccessibilityTable.cpp:422
> +                cell->updateAccessibilityRole();

is updateAccessibilityRole() a method on AccessibilityObject? if so, then no need to check if AccessibilityTableCell, you can just call it

[Nan Wang]

Created attachment 256999 [details]
patch

[WebKit Commit Bot]

Attachment 256999 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/AccessibilityTable.cpp:420:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nan Wang]

Created attachment 257000 [details]
patch

fix style

[WebKit Commit Bot]

Comment on attachment 257000 [details]
patch

Clearing flags on attachment: 257000

Committed r186974: <http://trac.webkit.org/changeset/186974>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.