[webkit-reviews] review granted: [Bug 179068] AX: search predicate returns containing group for plain text instead of text element : [Attachment 326217] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Doug Russell <d_russell at apple.com>'s
request for review:
Bug 179068: AX: search predicate returns containing group for plain text
instead of text element
https://bugs.webkit.org/show_bug.cgi?id=179068

Attachment 326217: patch

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




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

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:806
> +    if (ariaRoleAttribute() == AccessibilityRole::StaticText)
> +	   return true;
> +
> +    if (is<RenderText>(*m_renderer))
> +	   return true;
> +
> +    if (isTextControl())
> +	   return true;

It might make the line a little long, but I think a horizontal version of this
function that uses || might be clearer than a string of if statements where
it’s hard to spot which are "return true" and which are "return false".
Something roughly like this:

    return isARIAStaticText() || is<RenderText>(m_renderer) || isTextControl();

Needs a little rearranging and better helper functions, but something to think
about when writing future code.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3567
>      if (!m_renderer)
>	   return false;
> -    
> +
> +    if (!canHavePlainText())
> +	   return false;

No need to check m_renderer for null in both hasPlainText and canHavePlainText.