Use title attribute for image names when alt attribute is explicitly empty

RESOLVED FIXED

Status

()

enhancement
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Jamie, Assigned: surkov)

Tracking

({dev-doc-complete})

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

Reporter

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041707 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041707 Minefield/3.0pre

Although the alt attribute on an image is meant to provide a text alternative, the title attribute may also provide useful information. If the alt attribute is unspecified and the title attribute is specified, Gecko exposes the title attribute as the name of the image to accessibility APIs. If both are specified, Gecko exposes the alt attribute as the name and the title as the description. If the image is solely for visual layout and has no actual meaning (e.g. spacers), the alt attribute can explicitly be specified as an empty string, which indicates that the image should be disregarded for non-visual purposes. Unfortunately, there are some web sites which explicitly set the alt attribute to an empty string but set the title to something useful. In these cases, the title is disregarded; nothing is exposed for either name or description. Despite the fact that this is bad design on the part of the web sites, it would be better if Gecko could expose the title as the name in cases where alt is explicitly an empty string but the title is specified.

Reproducible: Always
Reporter

Updated

11 years ago
Version: unspecified → Trunk
Reporter

Comment 1

11 years ago
Posted file Test case.
This sample provides different combinations of alt and title attribute specification. In the last case (empty alt with title), neither name or description is exposed via accessibility APIs.
Assignee

Comment 2

11 years ago
Posted patch patch (obsolete) — Splinter Review
Assignee: aaronleventhal → surkov.alexander
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #316573 - Flags: review?(marco.zehe)
Assignee

Updated

11 years ago
Attachment #316573 - Flags: approval1.9?

Comment 4

11 years ago
Comment on attachment 316573 [details] [diff] [review]
patch

With this patch we no longer SetIsVoid() if there is nothing. Why that change? It's what tells a screen reader it's okay to try to come up with their own alt text (e.g. via the file name of the image, or the title of the page pointed to, etc.).

We put that in on request of ATs.
Attachment #316573 - Flags: approval1.9? → review-
Assignee

Comment 5

11 years ago
Comment on attachment 316573 [details] [diff] [review]
patch

(In reply to comment #4)
> (From update of attachment 316573 [details] [diff] [review])
> With this patch we no longer SetIsVoid() if there is nothing. Why that change?
> It's what tells a screen reader it's okay to try to come up with their own alt
> text (e.g. via the file name of the image, or the title of the page pointed to,
> etc.).
> 
> We put that in on request of ATs.
> 

If I get correctly then we can follow two ways:
1) use SetIsVoid/IsVoid
2) use Truncate/IsEmpty

Almost everywhere we use Truncate/IsEmpty() and we don't use IsVoid (excepting of CAccessibleAction::descripton which seems to be error). Possibly earlier we used the first approach but eventually things have been changed.

rerequesting
Attachment #316573 - Flags: review- → review?(aaronleventhal)

Comment 6

11 years ago
A void result only matters for get_accName(). We accidentally removed the IsVoid() check from get_accName() in bug 421650.

That check should not be removed, because for accessible name, NULL means something. For an img it means the difference between no alt text and alt="".

Please return that piece of nsAccessibleWrap it keep the SetIsVoid() when there is no alt, title, aria-labelledby etc.

Comment 7

11 years ago
Comment on attachment 316573 [details] [diff] [review]
patch

You can add some comments so we don't make the mistake again.
Attachment #316573 - Flags: review?(aaronleventhal) → review-

Comment 8

11 years ago
More info:

alt="" means the author says this is a decorative image or is redundant with the text, so don't speak anything for it.

No alt attribute means the author did not specify an alt attribute, and the browser or AT may repair (create an accessible name from the image or link URL, the title of the page pointed to, etc.)
Assignee

Comment 9

11 years ago
Posted patch patch2Splinter Review
does this patch works for you?
Attachment #316573 - Attachment is obsolete: true
Attachment #316965 - Flags: review?(aaronleventhal)
Assignee

Comment 10

11 years ago
This bug is invalid per comment #8 but we could morph it to make things clearer.
(In reply to comment #8)
> alt="" means the author says this is a decorative image or is redundant with
> the text, so don't speak anything for it.

Yes, but if there is also a title present, which is not "", shouldn't we be using that instead? Meaning: If the author for some reason did put "" for alt, but also provided a title, shouldn't we be using that instead, and only if that's not present, fall back to our current mechanism indicating that this is *really* a decorative image?

For example, JAWS does its own magic with Jamie's 4th list item: It sees that ALT is "", and looks for the title all by itself to provide that. So even though alt is "", JAWS finds the title and gives me useful information.
This Mochitest demonstrates the intended behaviour. The desired end result is that the last two tests also pass, not fail.

Comment 13

11 years ago
Ok, I should have said, if neither alt nor title are present as attributes, use SetIsVoid() so that screen reader knows they weren't set and can do it's own repair.

Comment 14

11 years ago
Comment on attachment 316965 [details] [diff] [review]
patch2

We should still be using SetIsVoid() and we should fix nsAccessibleWrap::get_accName() to check for the is void condition as we used to.

We should also take Marco's suggestion and still use title if alt="" (a rare condition).

We can SetIsVoid() on the name if the alt attribute is not present, and the name is still empty after trying aria-labelledby and title.
Attachment #316965 - Flags: review?(aaronleventhal) → review-

Updated

11 years ago
Attachment #316976 - Flags: review?(surkov.alexander)
Assignee

Comment 16

11 years ago
I'm concert setIsVoid/IsVoid(): why do you need *pszName = ::SysAllocStringLen(name.get(), name.Length()); for null pointer? Is it major difference with *pszName = NULL; that is used by AT?
Comment on attachment 316976 [details] [diff] [review]
Polish the logic, allow title if alt="", reinstate the legitimate NULL-name case, allow aria-labelledby w/o role since it's a global property

Passes all tests, including the accessibleName being NULL for images with no alt, and "" for images with alt="" and no title.
Attachment #316976 - Flags: review?(marco.zehe) → review+

Comment 18

11 years ago
I tested as well using Window-Eyes. We don't regress anything there.

Comment 19

11 years ago
To answer comment 16:
- this is just reverting to what was there before, we removed it without thinking
- NULL vs. "" is how the screen reader can tell the difference between
<img src="foo">   // no alt supplied, return NULL for accName, AT can guess
and
<img src="foo" alt="">     // author says this a decorative image, return ""
Assignee

Comment 20

11 years ago
Comment on attachment 316976 [details] [diff] [review]
Polish the logic, allow title if alt="", reinstate the legitimate NULL-name case, allow aria-labelledby w/o role since it's a global property


> /* wstring getName (); */
> NS_IMETHODIMP nsHTMLImageAccessible::GetName(nsAString& aName)
> {
>+  aName.Truncate();
>   nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
>-  if (!content) {
>-    return NS_ERROR_FAILURE;  // Node has been shut down
>-  }

I would suggest to use IsDefunct() here

>-      aName.SetIsVoid(PR_TRUE); // No alt or title
>+    if (aName.IsEmpty()) { // No name from alt or aria-labelledby
>+      content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::title, aName);
>+      if (!hasAltAttrib && aName.IsEmpty()) { 
>+        // Still no accessible name and no alt attribute is present.
>+        // SetIsVoid() is different from empty string -- this means a name was not 
>+        // provided by author and AT repair of the name is allowed.
>+        aName.SetIsVoid(PR_TRUE);

It doesn't make sense to call it with PR_TRUE because string is empty.

>+  nsAutoString name;
>+  if (NS_FAILED(xpAccessible->GetName(name)))
>+    return E_FAIL;

Use GetHRESULT() here
   
>+  if (name.IsVoid()) {
>+    // Valid return value for the name:
>+    // The name was not provided, e.g. no alt attribute for an image.

nit: you could use example from GetHTMLName() where SetIsVoid is used too but please mention nsHTMLImageAcc class where we can get more information about that.

the rest is ok, r=me
Attachment #316976 - Flags: review?(surkov.alexander) → review+
Assignee

Comment 21

11 years ago
ah, one thing, please replace IsVoid check on IsEmpty in CAccessibleAction::get_description since you are here.
Aaron, do you want to put a patch first addressing Surkov's comment, or should I request approval on the current patch?

Comment 23

11 years ago
Attachment #317248 - Flags: review?(surkov.alexander)
Assignee

Comment 24

11 years ago
Aaron, what about another comments?

Comment 25

11 years ago
Oh, I missed those.

Comment 26

11 years ago
(In reply to comment #20)
> (From update of attachment 316976 [details] [diff] [review])
> 
> > /* wstring getName (); */
> > NS_IMETHODIMP nsHTMLImageAccessible::GetName(nsAString& aName)
> > {
> >+  aName.Truncate();
> >   nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));
> >-  if (!content) {
> >-    return NS_ERROR_FAILURE;  // Node has been shut down
> >-  }
> 
> I would suggest to use IsDefunct() here

I prefer not to, since I need to QI to nsIContent anyway. I'd rather be sure that I have the interface I want and check if defunct all at the same time.

> >-      aName.SetIsVoid(PR_TRUE); // No alt or title
> >+    if (aName.IsEmpty()) { // No name from alt or aria-labelledby
> >+      content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::title, aName);
> >+      if (!hasAltAttrib && aName.IsEmpty()) { 
> >+        // Still no accessible name and no alt attribute is present.
> >+        // SetIsVoid() is different from empty string -- this means a name was not 
> >+        // provided by author and AT repair of the name is allowed.
> >+        aName.SetIsVoid(PR_TRUE);
> 
> It doesn't make sense to call it with PR_TRUE because string is empty.

That's not true. If we do SetIsVoid(PR_FALSE) that will make it not void, which is already is, and indicate there was an alt=""

> 
> >+  nsAutoString name;
> >+  if (NS_FAILED(xpAccessible->GetName(name)))
> >+    return E_FAIL;
> 
> Use GetHRESULT() here

Ok.

> 
> >+  if (name.IsVoid()) {
> >+    // Valid return value for the name:
> >+    // The name was not provided, e.g. no alt attribute for an image.
> 
> nit: you could use example from GetHTMLName() where SetIsVoid is used too but
> please mention nsHTMLImageAcc class where we can get more information about
> that.

Added comment.

Comment 27

11 years ago
Attachment #316976 - Attachment is obsolete: true
Attachment #317248 - Attachment is obsolete: true
Attachment #317250 - Flags: review?(surkov.alexander)
Attachment #317248 - Flags: review?(surkov.alexander)
Assignee

Comment 28

11 years ago
(In reply to comment #26)

> > I would suggest to use IsDefunct() here
> 
> I prefer not to, since I need to QI to nsIContent anyway. I'd rather be sure
> that I have the interface I want and check if defunct all at the same time.

The idea is to use IsDefunct everywhere and override it when it's needed. we decided that sometime ago, I don't remember in which bug. In this case we don't need check nsIContent on nsnull because it's not document or attribute. But if you want in anyway then would you like to override IsDefunct()?

Comment 29

11 years ago
I'll change it if you want. Can you r+ and I'll check in with your IsDefunct() change.
Assignee

Updated

11 years ago
Attachment #317250 - Flags: review?(surkov.alexander) → review+
Posted patch MochitestSplinter Review
Adding checks for alt="" and alt="" with title present. All others are already tested with existing Mochitests from bug 429659, and these tests still pass.
Attachment #316971 - Attachment is obsolete: true
Attachment #317267 - Flags: review?(surkov.alexander)
Comment on attachment 317250 [details] [diff] [review]
Address comments

Benefits: Return a correct accessible name if the author chose to put an alt="" and a meaningful title on an img tag. This is present in the wild sometimes, and we should make sure we get correct info.
Some screen readers like JAWS and Window-Eyes already apply such magic for this scenario, and this change would make sure others like Orca and NVDA don't have to go through hoops to get the same info.

Risk: We already have Mochitests for images that are not breaking from this change. Therefore the patch has been tested and has shown it is reliable.
Attachment #317250 - Flags: approval1.9?
Comment on attachment 317250 [details] [diff] [review]
Address comments

a1.9+=damons
Attachment #317250 - Flags: approval1.9? → approval1.9+
Assignee

Comment 34

11 years ago
Comment on attachment 317267 [details] [diff] [review]
Mochitest

r=me
Attachment #317267 - Flags: review?(surkov.alexander) → review+

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Checking in accessible/tests/mochitest/test_nsIAccessibleImage.html;
/cvsroot/mozilla/accessible/tests/mochitest/test_nsIAccessibleImage.html,v  <--  test_nsIAccessibleImage.html
new revision: 1.2; previous revision: 1.1
done

This completes the bugfix.
You need to log in before you can comment on or make changes to this bug.