[webkit-reviews] review granted: [Bug 200559] Accessibility client cannot navigate to internal links targets on iOS. : [Attachment 375884] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andres Gonzalez
<andresg_22 at apple.com>'s request for review:
Bug 200559: Accessibility client cannot navigate to internal links targets on
iOS.
https://bugs.webkit.org/show_bug.cgi?id=200559

Attachment 375884: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 375884
  --> https://bugs.webkit.org/attachment.cgi?id=375884
Patch

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

Looks good.

> Source/WebCore/accessibility/AccessibilityObject.cpp:545
> +    return WebCore::firstAccessibleObjectFromNode(node, [] (const
AccessibilityObject& acc) {
> +	   return !acc.accessibilityIsIgnored();
> +    });

WebKit coding style says we should use words, not abbreviations, for variable
names. So the name here could be "object", "accessibilityObject", "candidate",
or some other word, but shouldn’t be "acc".

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1816
> +    /* AccessibilityObject::linkedUIElements may return an object that is
> +	exposed in other platforms but not on iOS, i.e., grouping or structure
> +	elements like <div> or <p>. Thus find the next accessible object that
is
> +	exposed on iOS.
> +	*/

WebKit coding style calls for "//" comments, not "/*" comments.

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1820
> +    if (auto linked =
firstAccessibleObjectFromNode(linkedElements[0]->node(), [] (const
AccessibilityObject& acc) {
> +	   return acc.wrapper().isAccessibilityElement;
> +    }))
> +	   return linked->wrapper();

Same issue with variable named "acc".

Formatting here makes this hard to read. Might want to rearrange to avoid that.
One way to do it is:

    auto linkedElement =
firstAccessibleObjectFromNode(linkedElements[0]->node(), [] (const
AccessibilityObject& object) {
	return object.wrapper().isAccessibilityElement;
    });
    return linkedElement ? linkedElement->wrapper() : nullptr;

But I’m sure there are other ways to sidestep it too.