Bug 130386 – AX: Bug in AccessibilityNodeObject::helpText

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Dominic Mazzoni]

Discovered by [e-mail] in Blink (Chromium) code: https://code.google.com/p/chromium/issues/detail?id=352771
The bug was introduced here: https://bugs.webkit.org/show_bug.cgi?id=99502

In this snippet, getAttribute should be curr->getAttribute.

1403    for (Node* curr = node(); curr; curr = curr->parentNode()) {
1404	        const AtomicString& summary = getAttribute(summaryAttr);
1405	        if (!summary.isEmpty())
1406	            textOrder.append(AccessibilityText(summary, SummaryText));
1407	       
1408	        // The title attribute should be used as help text unless it is already being used as descriptive text.
1409	        const AtomicString& title = getAttribute(titleAttr);
1410	        if (!title.isEmpty())
1411	            textOrder.append(AccessibilityText(title, TitleTagText));

[chris fleizach]

good catch

[Radar WebKit Bug Importer]

<rdar://problem/16357555>

[James Craig]

Created attachment 228161 [details]
patch

Might need some better examples in the test, and/or commented in the source. I can't really determine why we're ever getting helpText from the parentNode, so I'm not sure this is an effective test.

[chris fleizach]

(In reply to comment #0)
> Discovered by [e-mail] in Blink (Chromium) code: https://code.google.com/p/chromium/issues/detail?id=352771
> The bug was introduced here: https://bugs.webkit.org/show_bug.cgi?id=99502
> 
> In this snippet, getAttribute should be curr->getAttribute.
> 
> 1403    for (Node* curr = node(); curr; curr = curr->parentNode()) {
> 1404            const AtomicString& summary = getAttribute(summaryAttr);
> 1405            if (!summary.isEmpty())
> 1406                textOrder.append(AccessibilityText(summary, SummaryText));
> 1407           
> 1408            // The title attribute should be used as help text unless it is already being used as descriptive text.
> 1409            const AtomicString& title = getAttribute(titleAttr);
> 1410            if (!title.isEmpty())
> 1411                textOrder.append(AccessibilityText(title, TitleTagText));

(In reply to comment #3)
> Created an attachment (id=228161) [details]
> patch
> 
> Might need some better examples in the test, and/or commented in the source. I can't really determine why we're ever getting helpText from the parentNode, so I'm not sure this is an effective test.

One thing that's held me up on this patch is whether we actually want to do this behavior. I am not completely sure we do

[James Craig]

Created attachment 228353 [details]
patch

Okay, given that this clearly never worked and we don't know why it ever should, I'm pulling out the loop.

[chris fleizach]

Comment on attachment 228353 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1397
> +    Node* node = this->node();

I don't think you even need to get node anymore. i don't see it being used

[James Craig]

Comment on attachment 228353 [details]
patch

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

>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1397
>> +    Node* node = this->node();
> 
> I don't think you even need to get node anymore. i don't see it being used

It's used on the next few lines.

 1399    if (!node)
 1399        return;
 1400    if (node->isElementNode()) {

[chris fleizach]

Comment on attachment 228353 [details]
patch

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

>>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1397
>>> +    Node* node = this->node();
>> 
>> I don't think you even need to get node anymore. i don't see it being used
> 
> It's used on the next few lines.
> 
>  1399    if (!node)
>  1399        return;
>  1400    if (node->isElementNode()) {

but it doesn't seem like we need it after that

[James Craig]

Comment on attachment 228353 [details]
patch

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

>>>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1397
>>>> +    Node* node = this->node();
>>> 
>>> I don't think you even need to get node anymore. i don't see it being used
>> 
>> It's used on the next few lines.
>> 
>>  1399    if (!node)
>>  1399        return;
>>  1400    if (node->isElementNode()) {
> 
> but it doesn't seem like we need it after that

I need it for the conditional. What change are you suggesting? Combining it all on one line like this?

if (this->() && this->node->isElementNode()) {

[James Craig]

Comment on attachment 228353 [details]
patch

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

>>>>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1397
>>>>> +    Node* node = this->node();
>>>> 
>>>> I don't think you even need to get node anymore. i don't see it being used
>>> 
>>> It's used on the next few lines.
>>> 
>>>  1399    if (!node)
>>>  1399        return;
>>>  1400    if (node->isElementNode()) {
>> 
>> but it doesn't seem like we need it after that
> 
> I need it for the conditional. What change are you suggesting? Combining it all on one line like this?
> 
> if (this->() && this->node->isElementNode()) {

if (this->node() && this->node()->isElementNode()) {

[chris fleizach]

(In reply to comment #10)
> (From update of attachment 228353 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228353&action=review
> 
> >>>>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1397
> >>>>> +    Node* node = this->node();
> >>>> 
> >>>> I don't think you even need to get node anymore. i don't see it being used
> >>> 
> >>> It's used on the next few lines.
> >>> 
> >>>  1399    if (!node)
> >>>  1399        return;
> >>>  1400    if (node->isElementNode()) {
> >> 
> >> but it doesn't seem like we need it after that
> > 
> > I need it for the conditional. What change are you suggesting? Combining it all on one line like this?
> > 
> > if (this->() && this->node->isElementNode()) {
> 
> if (this->node() && this->node()->isElementNode()) {

Yea it looks like we don't need  this anymore

[James Craig]

Comment on attachment 228353 [details]
patch

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

>>>>>>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1397
>>>>>>> +    Node* node = this->node();
>>>>>> 
>>>>>> I don't think you even need to get node anymore. i don't see it being used
>>>>> 
>>>>> It's used on the next few lines.
>>>>> 
>>>>>  1399    if (!node)
>>>>>  1399        return;
>>>>>  1400    if (node->isElementNode()) {
>>>> 
>>>> but it doesn't seem like we need it after that
>>> 
>>> I need it for the conditional. What change are you suggesting? Combining it all on one line like this?
>>> 
>>> if (this->() && this->node->isElementNode()) {
>> 
>> if (this->node() && this->node()->isElementNode()) {
> 
> Yea it looks like we don't need  this anymore

Don't we want to ensure the node exists and is an element node before attempting to to call these attribute methods on it?

[James Craig]

Created attachment 228945 [details]
patch with review feedback

Stylistic change based on review feedback. Avoids extra conditional here (and simplifies code) for the element case even if it causes slightly more execution (conditionals embedded in getAttribute, etc) in the less common, non-element case.

[chris fleizach]

Comment on attachment 228945 [details]
patch with review feedback

thanks!

[WebKit Commit Bot]

Comment on attachment 228945 [details]
patch with review feedback

Clearing flags on attachment: 228945

Committed r167054: <http://trac.webkit.org/changeset/167054>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Alberto Lopez Perez]

The test accessibility/help-text.html introduced here by r167054 is failing on platform GTK ( see bug #131496 )

[chris fleizach]

(In reply to comment #17)
> The test accessibility/help-text.html introduced here by r167054 is failing on platform GTK ( see bug #131496 )

Maybe we need a GTK expectation file for differences?