This was originally brought up in NVDA ticket 156 <http://www.nvda-project.org/ticket/156> and is basically a link with a non-breaking space, a title attribute and a background image for sighted users. Question: If we encounter an accessible name of \xa0, which is a non-breaking space, should we treat it as if the string was empty and fall back to other means like title etc.? My gutt feeling is yes, to give our users the best experience and shield them a bit from this bad design. Thoughts?

Speaking as a pedant, this is bad design and we should not have to compensate for it. Speaking as a user, I think we should try to "repair" it if we can, as it will make such cases useable and it should not have any negative effects.

Comment on attachment 380406 [details] [diff] [review] patch > //////////////////////////////////////////////////////////////////////////////// >+// nsWhitespaceChecker. >+ >+/** >+ * Helper class allows to check if the given string consists of whitespace >+ * charackters. In constast to nsWhitespaceTokenizer it takes into account "characters" :) >+ * non-breaking space (0xa0). >+ */ >+class nsWhitespaceChecker I think I would prefer some static util methods over a new class. I know we've gone with classes for this kind of thing elsewhere but I wonder about performance. What are your thoughts here? >- aName = name; >+ if (!name.IsEmpty()) { >+ nsWhitespaceChecker checker(name); >+ if (!checker.isWhitespaceString()) >+ aName = name; Maybe this could condense to something like: if (!name.IsEmpty() && !nsTextUtils::isWhitespace(name)) aName = name; Or if (nsTextUtils::isGoodName(name)) // isGoodName combines IsEmpty and IsWhitespace checks aName = name; The patch looks correct though.

Comment on attachment 380406 [details] [diff] [review] patch >+ * charackters. In constast to nsWhitespaceTokenizer it takes into account and "contrast" :)

Comment on attachment 380406 [details] [diff] [review] patch >+ aChar == '\r'|| aChar == '\t' || aChar == 0xa0; Nit: Missing space before the first || in this line. Also a question: Is it sufficient to test this one testcase where you have two non-breaking whitespaces and a "real" space, or should we add one more that has a single non-breaking whitespace character? r=me with the nit fixed and the question answered or another testcase added.

(In reply to comment #5) > Also a question: Is it sufficient to test this one testcase where you have two > non-breaking whitespaces and a "real" space, or should we add one more that has > a single non-breaking whitespace character? I think it's enough. This complex test includes your case. If non-braking space and space works together then single non-breaking space will work as well. It's similar to & operand :).

with David and Marco points

Comment on attachment 380420 [details] [diff] [review] patch2 Thanks, only 2 nits left: > #define NS_OK_NO_NAME_CLAUSE_HANDLED \ > NS_ERROR_GENERATE_SUCCESS(NS_ERROR_MODULE_GENERAL, 0x24) > >+ Why the extra line? >+ * only. In contast to nsWhitespaceTokenizer class it takes into account "contrast" r=me

with David nits

Comment on attachment 380423 [details] [diff] [review] patch3 One more: >+ * Returns true if the given sting is empty or contains whitespace symbols "string".

(In reply to comment #10) > (From update of attachment 380423 [details] [diff] [review]) > One more: > >+ * Returns true if the given sting is empty or contains whitespace symbols > > "string". thanks, updated locally

checked in, http://hg.mozilla.org/mozilla-central/rev/2f859373bedc

Verified fixed in: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090614 Minefield/3.6a1pre.

See comment #13. Thanks Jamie for confirmation!