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

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.

Comment on attachment 316573 [details] [diff] [review] patch r=me

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.

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

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 on attachment 316573 [details] [diff] [review] patch You can add some comments so we don't make the mistake again.

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.)

does this patch works for you?

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.

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 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.

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.

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

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 ""

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

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?

Aaron, what about another comments?

Oh, I missed those.

(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.

(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()?

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

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.

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.

Comment on attachment 317250 [details] [diff] [review] Address comments a1.9+=damons

documented at http://developer.mozilla.org/en/docs/Accessibility:AT-APIs:MSAA:Interfaces#get_accName

Comment on attachment 317267 [details] [diff] [review] Mochitest r=me

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.