[Webkit-unassigned] [Bug 147001] AX: iframe within table cell is inaccessible to VoiceOver

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=147001

chris fleizach <cfleizach at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #256941|commit-queue?               |review-, commit-queue-
              Flags|                            |

--- Comment #3 from chris fleizach <cfleizach at apple.com> ---
Comment on attachment 256941
  --> https://bugs.webkit.org/attachment.cgi?id=256941
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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150717/d4d074d6/attachment.html>