Bug 96412

Summary: title attribute is not exposed as the AXDescription on AXImage when there is no other fallback content
Product: WebKit Reporter: chris fleizach <cfleizach>
Component: AccessibilityAssignee: chris fleizach <cfleizach>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dmazzoni, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
webkit-ews: commit-queue-
patch
jhoneycutt: review+
patch for landing
none
patch for landing none

Description chris fleizach 2012-09-11 11:29:07 PDT
The bug here is that the title attribute does not become the label on AXImage if there is no alt tag.
Comment 1 chris fleizach 2012-09-11 12:03:21 PDT
Created attachment 163417 [details]
patch
Comment 2 Early Warning System Bot 2012-09-11 12:10:14 PDT
Comment on attachment 163417 [details]
patch

Attachment 163417 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13826335
Comment 3 Early Warning System Bot 2012-09-11 12:10:44 PDT
Comment on attachment 163417 [details]
patch

Attachment 163417 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13819431
Comment 4 WebKit Review Bot 2012-09-11 12:11:00 PDT
Comment on attachment 163417 [details]
patch

Attachment 163417 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13832041
Comment 5 chris fleizach 2012-09-11 12:13:02 PDT
Created attachment 163418 [details]
patch
Comment 6 Jon Honeycutt 2012-09-11 12:24:22 PDT
Comment on attachment 163418 [details]
patch

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

> LayoutTests/accessibility/img-fallsback-to-title.html:12
> +    <img title="test1" height="100" width="100">
> +    <img alt="alt" title="test2" height="100" width="100">
> +    <div role="img" title="test3" width="100" height="100">test</div>
> +    <div role="img" alt="alt" title="test4" width="100" height="100">test</div>

Please add a test for the empty alt attribute case if we don't already have one.

r=me
Comment 7 Darin Adler 2012-09-11 13:27:29 PDT
Comment on attachment 163418 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1508
> -    Node* node = m_renderer->node();
>      if (isImage() || isInputImage() || isNativeImage() || isCanvas()) {
> -        if (node && node->isHTMLElement()) {
> -            const AtomicString& alt = toHTMLElement(node)->getAttribute(altAttr);
> -            if (alt.isEmpty())
> -                return String();
> +
> +        // Images should use alt as long as the attribute is present, even if empty.
> +        // Otherwise, it should fallback to other methods, like the title attribute.
> +        const AtomicString& alt = getAttribute(altAttr);
> +        if (!alt.isNull())
>              return alt;
> -        }
>      }

I understand the change from isEmpty to isNull. I don’t understand the removal of the HTMLElement check, nor does the test case seem to cover this.
Comment 8 Dominic Mazzoni 2012-09-11 14:10:16 PDT
Comment on attachment 163418 [details]
patch

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

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1508
>>      }
> 
> I understand the change from isEmpty to isNull. I don’t understand the removal of the HTMLElement check, nor does the test case seem to cover this.

AccessibilityObject::getAttribute already does the HTMLElement check
Comment 9 chris fleizach 2012-09-11 14:28:30 PDT
(In reply to comment #6)
> (From update of attachment 163418 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163418&action=review
> 
> > LayoutTests/accessibility/img-fallsback-to-title.html:12
> > +    <img title="test1" height="100" width="100">
> > +    <img alt="alt" title="test2" height="100" width="100">
> > +    <div role="img" title="test3" width="100" height="100">test</div>
> > +    <div role="img" alt="alt" title="test4" width="100" height="100">test</div>
> 
> Please add a test for the empty alt attribute case if we don't already have one.
> 
> r=me

Will add. Thanks
Comment 10 chris fleizach 2012-09-11 14:46:17 PDT
Created attachment 163447 [details]
patch for landing
Comment 11 chris fleizach 2012-09-11 14:47:21 PDT
Created attachment 163448 [details]
patch for landing
Comment 12 chris fleizach 2012-09-11 15:00:58 PDT
http://trac.webkit.org/changeset/128227
Comment 13 chris fleizach 2012-09-11 15:01:08 PDT
rdar://12061421