[webkit-reviews] review granted: [Bug 231993] AX: Using aria-activedescendant to set the active cell within a grid does not convey focus movement : [Attachment 465853] Patch

[bugzilla-daemon at webkit.org]

Tyler Wilcock <tyler_w at apple.com> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 231993: AX: Using aria-activedescendant to set the active cell within a
grid does not convey focus movement
https://bugs.webkit.org/show_bug.cgi?id=231993

Attachment 465853: Patch

https://bugs.webkit.org/attachment.cgi?id=465853&action=review




--- Comment #5 from Tyler Wilcock <tyler_w at apple.com> ---
Comment on attachment 465853
  --> https://bugs.webkit.org/attachment.cgi?id=465853
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:2843
> +    for (auto cell : cells()) {

The type of cell is RefPtr<AXCoreObject>, and the codestyle we've been
following recently uses RefPtr (no <>) over auto.

> Source/WebCore/accessibility/AccessibilityObject.cpp:2848
> +    if (auto activeDesc = activeDescendant()) {

`activeDescendant` returns a pointer, so this should be auto*. Also, I think
WebKit coding style prefers no abbreviations, so this line could maybe be:

if (auto* activeDescendant = this->activeDescendant()) {

> LayoutTests/accessibility/mac/grid-selected-cells.html:12
> +    <tr ca="tr" role="row" aria-level="1" id="row1">

Is this `ca` attribute necessary? Same question for the other uses in this
file.

> LayoutTests/accessibility/mac/grid-selected-cells.html:41
> +	       output += "Received notification: " + notification + "\n";

You can use JS template literals here:

output += `Received notification: ${notification}\n`;