Closed Bug 495390 Opened 11 years ago Closed 11 years ago

If a link has a non-breaking space as screen text, try to fall back to other means like @title

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: MarcoZ, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #380406 - Flags: review?(marco.zehe)
Attachment #380406 - Flags: review?(david.bolter)
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" :)
Attachment #380406 - Flags: review?(marco.zehe) → review+
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 :).
Attached patch patch2 (obsolete) — Splinter Review
with David and Marco points
Attachment #380406 - Attachment is obsolete: true
Attachment #380420 - Flags: review?(david.bolter)
Attachment #380406 - Flags: review?(david.bolter)
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
Attachment #380420 - Flags: review?(david.bolter) → review+
Attached patch patch3Splinter Review
with David nits
Attachment #380420 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.