468451 - Images with empty alt attribute no longer get an empty accessible name, but return NULL instead.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

This is a very recent regression and shows up if you run the test_nsIAccessibleImage.html chrome test file:
not ok - wrong name for nonLinkedImageEmptyAlt! got null, expected ""
not ok - wrong name for linkedImageEmptyAlt! got null, expected
""

The empty alt attribute (alt="") tells screen readers that they should not repair the image's accessible name, but treat it as presentational.

This was introduced with the last wave of checkins on Dec 3.

Comment 1

[Marco Zehe (:MarcoZ)]

Pushed disabling of testing these two images:
http://hg.mozilla.org/mozilla-central/rev/a2986966ab87.

As part of the patch to fix this bug, the two commented-out lines should be reenabled.

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 352512 [details] [diff] [review]
patch

conditional r+
As per IRC chat it would be nice to change the comments to say something about an explicitly provided empty name.

>-  if (aName.IsVoid() && hasAltAttrib) {
>+  if (aName.IsEmpty() && hasAltAttrib) {
>     // No accessible name but empty alt attribute is present. This means a name
>     // was provided by author and AT repair of the name isn't allowed.
>-    aName.Truncate();
>+    return NS_OK_EMPTY_NAME;

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

I mean't to add that the "AT repair" comment is confusing I think.

Comment 5

[Marco Zehe (:MarcoZ)]

Comment on attachment 352512 [details] [diff] [review]
patch

Like David said, the comment should be improved. I suggest to merge the comment from the top of the method and the one within the "if" block and put it before the "if" block since this is where the extra processing begins. With that, r=me.

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch2

David and Marco, please look at new comments. Asking additional review from Aaron.

Comment 7

[alexander :surkov (:asurkov)]

regression from bug 467055

Comment 8

[Aaron Leventhal]

I think it should probably be:
(hasAltAttribute || hasAriaLabelAttribute || hasAriaLabelledByAttribute).

In any of those cases the author has used *something* to indicate there is an empty accessible name.

What do you guys think?

If we do that then let's make sure the implementor's guide says that.

Comment 9

[alexander :surkov (:asurkov)]

Yes, I think it's reasonable that any empty attribute used for name calculation should provide empty name if further algorithm doesn't provide not empty name.

Comment 10

[Aaron Leventhal]

Ok, you still have my r+, since that's an easy change to make to this patch.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> Ok, you still have my r+, since that's an easy change to make to this patch.

I would prefer to have separate bug for this even the patch is simple.

Comment 12

[Marco Zehe (:MarcoZ)]

Comment on attachment 352662 [details] [diff] [review]
patch2

Two ortographical nits:

>+   * Result can be string or null. String value means the name was specified and
>+   * AT can't repair the name even if the string is empty (that means empty name
>+   * was provided by author itentionally). In the case of null value AT are free

"itentionally" should be "intentionally" (is missing the n).

>+    // No accessible name but empty 'alt' attribute is present. This means
>+    // an empty name was provided by author intentionally and image is
>+    // presentational in the case if futher name computation algorithm don't
>+    // provide non empty name (see nsIAccessible::name attribute for details).

"futher" should be "further" (misssing r before the t), and "algorithm don't" should be "algorithm doesn't".

Thanks ffor the changes, this makes things much clearer.

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 352662 [details] [diff] [review]
patch2

I'd like to offer some commenting suggestions, building on what Marco said:

>+   * Result can be string or null. String value means the name was specified and
>+   * AT can't repair the name even if the string is empty (that means empty name
>+   * was provided by author itentionally). In the case of null value AT are free
>+   * to compute the name by own algorithms.

Would the following comment work here?

Value can be string or null. A null value indicates that AT may attempt to compute the name. Any string value, including the empty string, should be considered author-intentional, and respected.

>+  if (aName.IsEmpty() && hasAltAttrib) {
>+    // No accessible name but empty 'alt' attribute is present. This means
>+    // an empty name was provided by author intentionally and image is
>+    // presentational in the case if futher name computation algorithm don't
>+    // provide non empty name (see nsIAccessible::name attribute for details).
>+    return NS_OK_EMPTY_NAME;

How about this wording?

An empty alt attribute is used to indicate a decorative image; no further name computation required.

Also I like Aaron's has* additions.

BTW here's a related WCAG 2.0 statement: http://www.w3.org/TR/2008/NOTE-WCAG20-TECHS-20081211/H67

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> (From update of attachment 352662 [details] [diff] [review])
> I'd like to offer some commenting suggestions, building on what Marco said:
> 
> >+   * Result can be string or null. String value means the name was specified and
> >+   * AT can't repair the name even if the string is empty (that means empty name
> >+   * was provided by author itentionally). In the case of null value AT are free
> >+   * to compute the name by own algorithms.
> 
> Would the following comment work here?
> 
> Value can be string or null. A null value indicates that AT may attempt to
> compute the name. Any string value, including the empty string, should be
> considered author-intentional, and respected.

nice.

> >+  if (aName.IsEmpty() && hasAltAttrib) {
> >+    // No accessible name but empty 'alt' attribute is present. This means
> >+    // an empty name was provided by author intentionally and image is
> >+    // presentational in the case if futher name computation algorithm don't
> >+    // provide non empty name (see nsIAccessible::name attribute for details).
> >+    return NS_OK_EMPTY_NAME;
> 
> How about this wording?
> 
> An empty alt attribute is used to indicate a decorative image; no further name
> computation required.

it's opposite wording actually.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #13)

> Also I like Aaron's has* additions.

I filed bug 469449

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #14)

> > >+  if (aName.IsEmpty() && hasAltAttrib) {
> > >+    // No accessible name but empty 'alt' attribute is present. This means
> > >+    // an empty name was provided by author intentionally and image is
> > >+    // presentational in the case if futher name computation algorithm don't
> > >+    // provide non empty name (see nsIAccessible::name attribute for details).
> > >+    return NS_OK_EMPTY_NAME;
> > 
> > How about this wording?
> > 
> > An empty alt attribute is used to indicate a decorative image; no further name
> > computation required.
> 
> it's opposite wording actually.

I think the wording is equivalent, but I'm happy either way :)

Comment 17

[alexander :surkov (:asurkov)]

That's not good :) I changed it a bit. Hope it's ok now.

http://hg.mozilla.org/mozilla-central/rev/160661559c62