Closed Bug 468451 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Disability Access APIs, defect, major)

x86
Windows Vista
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: MarcoZ, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access, regression)

Attachments

(1 file, 1 obsolete file)

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.
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.
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
Attachment #352512 - Flags: review?(marco.zehe)
Attachment #352512 - Flags: review?(david.bolter)
Attachment #352512 - Flags: review?(david.bolter) → review+
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;
I mean't to add that the "AT repair" comment is confusing I think.
Attachment #352512 - Flags: review?(marco.zehe) → review+
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.
Attached patch patch2Splinter Review
David and Marco, please look at new comments. Asking additional review from Aaron.
Attachment #352512 - Attachment is obsolete: true
Attachment #352662 - Flags: review?(aaronleventhal)
regression from bug 467055
Blocks: 467055, namea11y
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.
Attachment #352662 - Flags: review?(aaronleventhal) → review+
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.
Ok, you still have my r+, since that's an easy change to make to this patch.
(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 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 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
(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.
(In reply to comment #13)

> Also I like Aaron's has* additions.

I filed bug 469449
(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 :)
That's not good :) I changed it a bit. Hope it's ok now.

http://hg.mozilla.org/mozilla-central/rev/160661559c62
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.