AX: REGRESSION: @title is exposed as AXDescription when label label from contents already exists.

[James Craig]

AX: REGRESSION: @title is exposed as AXDescription when label label from contents already exists.

Presumably this is not traversing the contents correctly (considering the element to have no label from contents) and therefore exposing the @title value as AXDescription instead of the default AXHelp.

[Radar WebKit Bug Importer]

<rdar://problem/14882968>

[James Craig]

Created attachment 210162 [details]
test case demonstrating bug

[James Craig]

Looks like it also exposes the contents as both AXDescription and AXHelp, which should never happen, and results in extremely redundant speech for VoiceOver.

[chris fleizach]

Created attachment 210318 [details]
patch

[Mario Sanchez Prada]

Comment on attachment 210318 [details]
patch

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.

[chris fleizach]

(In reply to comment #5)
> (From update of attachment 210318 [details])
> 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.

Yes this is part of the contract of the accessibilityText method. You rely on the order of the text being appended in importance

Thanks

[WebKit Commit Bot]

Comment on attachment 210318 [details]
patch

Clearing flags on attachment: 210318

Committed r155022: <http://trac.webkit.org/changeset/155022>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.