[webkit-reviews] review granted: [Bug 57463] [GTK] ARIA tables not exposing cells as HTML tables do : [Attachment 88393] Patch proposal + Layout test

[bugzilla-daemon at webkit.org]

chris fleizach <cfleizach at apple.com> has granted Mario Sanchez Prada
<msanchez at igalia.com>'s request for review:
Bug 57463: [GTK] ARIA tables not exposing cells as HTML tables do
https://bugs.webkit.org/show_bug.cgi?id=57463

Attachment 88393: Patch proposal + Layout test
https://bugs.webkit.org/attachment.cgi?id=88393&action=review

------- Additional Comments from chris fleizach <cfleizach at apple.com>
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