[webkit-reviews] review granted: [Bug 120550] AX: REGRESSION: @title is exposed as AXDescription when label label from contents already exists. : [Attachment 210318] patch

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 120550: AX: REGRESSION: @title is exposed as AXDescription when label label
from contents already exists.
https://bugs.webkit.org/show_bug.cgi?id=120550

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

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=210318&action=review


> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2040
> +	   switch (text.textSource) {
> +	   case VisibleText:
> +	   case ChildrenText:
> +	   case LabelByElementText:
> +	       visibleTextAvailable = true;
> +	   default:
> +	       break;
> +	   }
> +	   
> +	   if (text.textSource == TitleTagText && !visibleTextAvailable)
>	       return text.text;

I believe this is safe just because TitleTagText sources are always appended
"almost" at the end (in AccessibilityNodeObject::helpText() and before maybe
adding a PlaceHolderText source in accessibilityText()). However, if the order
was not always that one (e.g. what if you have some VisibleText source after
the TitleTagText one?) then you might find yourself returning text.text here
because visibleTextAvailable hasn't set to true yet.

Setting r+ anyway because this is Mac specific code and you definitely know
better. Just commenting that in case you might want to consider it in order to
protect against situations that might happen in the future if the assertion
about the order in which TitleTagText source is being appended was no longer
true.