[webkit-reviews] review granted: [Bug 136714] AX: in an aria-labelledby computation, do not traverse into elements whose nameFrom value does not include 'contents' : [Attachment 238767] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 136714: AX: in an aria-labelledby computation, do not traverse into
elements whose nameFrom value does not include 'contents'
https://bugs.webkit.org/show_bug.cgi?id=136714

Attachment 238767: patch
https://bugs.webkit.org/attachment.cgi?id=238767&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=238767&action=review


> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1645
> +static void appendStringBuildTextToName(String& text, StringBuilder&
builder)

This should take const String& text. Also the StringBuilder should be the first
argument. Also not sure that "append string build text to name" is the best
name for this helper function. I think it should probably just be called
"append name" or perhaps "append name to string builder".

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1670
> +	       String axName = accessibleNameForNode(child->node());
> +	       appendStringBuildTextToName(axName, builder);

This would read better without the local variable.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1864
> +	   String valueDesc = axObject->valueDescription();
> +	   if (!valueDesc.isEmpty())
> +	       return valueDesc;

Please don’t abbreviate “description” as “desc” in WebKit code.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1875
> +    }
>      else

WebKit style puts the } on the same line as the else.

> Source/WebCore/accessibility/AccessibilityObject.cpp:292
> +bool AccessibilityObject::accessibleNameDerivesFromContent() const

This has some long case lists in it. Do we have test coverage for all these
cases?