Closed Bug 469367 Opened 12 years ago Closed 12 years ago

Add ARIA semantics to those search widget types that don't have a Search button.

Categories

(Firefox :: Disability Access, defect)

defect
Not set
trivial

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: MarcoZ, Assigned: MarcoZ)

References

Details

(Keywords: access, fixed1.9.1, Whiteboard: No risk. Additional ARIA attributes do not affect anything but AT support.)

Attachments

(1 file, 6 obsolete files)

This is to indicate to screen readers that these search widgets perform a search the moment you start typing. Visually, this is indicated via a different placement of the lense (or whatever that icon is).

aria-autocomplete="list" means that typing in this textbox will peerform filtering on a different control.
aria-controls=<ID> then tells which control that is that's being filtered.

Question for Dao who implemented the Search widget: Is there a clever way to get these semantics applied on the XBL level? I could imagine something like this:

1. If the "searchbutton" attribute is not defined, add the aria-autocomplete="list".
2. Add a new attribute "filters" or something like this that, when defined, is being translated to aria-controls=<ID>, where <ID> would be the value provided by "filters".
This is the not so elegant method.
Assignee: nobody → marco.zehe
Status: NEW → ASSIGNED
> 1. If the "searchbutton" attribute is not defined, add the
> aria-autocomplete="list".

This should be trivial.

> 2. Add a new attribute "filters" or something like this that, when defined, is
> being translated to aria-controls=<ID>, where <ID> would be the value provided
> by "filters".

Given that we don't need "filters" for something else, it seems to me that we should just add the aria-controls attribute directly.
(In reply to comment #2)
> > 1. If the "searchbutton" attribute is not defined, add the
> > aria-autocomplete="list".
> 
> This should be trivial.

OK, jusst add or remove the attribute along with the "searchbutton" thingie? Because if the @searchbutton attribute is present, we don't want aria-autocomplete at all in there.

> > 2. Add a new attribute "filters" or something like this that, when defined, is
> > being translated to aria-controls=<ID>, where <ID> would be the value provided
> > by "filters".
> Given that we don't need "filters" for something else, it seems to me that we
> should just add the aria-controls attribute directly.

OK.
(In reply to comment #3)
> (In reply to comment #2)
> > > 1. If the "searchbutton" attribute is not defined, add the
> > > aria-autocomplete="list".
> 
> OK, jusst add or remove the attribute along with the "searchbutton" thingie?

Yep, in the searchButton property setter and in the constructor.
(There's no xbl constructor yet in that binding.)
Attached patch Patch with logic in XBL binding (obsolete) — Splinter Review
Is this how it should be done in the XBL binding?
Attachment #352762 - Attachment is obsolete: true
Attachment #352770 - Flags: review?(dao)
Comment on attachment 352770 [details] [diff] [review]
Patch with logic in XBL binding

>+        <setter><![CDATA[
>+          if (val) {
>+            this.setAttribute('searchbutton', 'true');
>+            this.setAttribute('aria-autocomplete', 'list');
>+          } else {
>+            this.removeAttribute('searchbutton');
>+            this.removeAttribute('aria-autocomplete');
>+          }

This is the wrong way round, and please use double quotes here.

>+      <constructor><![CDATA[
>+        if (!this.hasAttribute('searchbutton')
>+          this.setAttribute('aria-autocomplete', 'list');
>+      ]]></constructor>

This should be:

        if (this.hasAttribute("searchbutton"))
          this.removeAttribute("aria-autocomplete");
        else
          this.setAttribute("aria-autocomplete", "list");

And it's probably better to put the constructor above all other methods.
Attachment #352770 - Flags: review?(dao) → review-
Attached patch Patch 2 (obsolete) — Splinter Review
Addressing Dao's comments.
Attachment #352770 - Attachment is obsolete: true
Attachment #352777 - Flags: review?(dao)
Attachment #352777 - Flags: review?(dao) → review-
Comment on attachment 352777 [details] [diff] [review]
Patch 2

>+      <constructor><![CDATA[
>+        if (this.hasAttribute('searchbutton')

The trailing parenthesis is missing.
(Also, double quotes...)

With that fixed, could you please verify that this works as expected, e.g. in the history sidebar and in the add-ons manager? You can do that better than I.
Fixed, thanks! I'll be in-flight in 90 minutes and will do the testing when I get home on Saturday night. If it works out the way it is intended, will also ask SR as well, since this also has an impact on the comm-central repo.
Attachment #352777 - Attachment is obsolete: true
Attachment #352781 - Flags: review?(dao)
Attachment #352781 - Flags: review?(dao) → review+
Comment on attachment 352781 [details] [diff] [review]
Fix remaining double quotes and missing trailing parenthesis.

This doesn't need sr, but you could ask Enn for another review.
Attachment #352781 - Flags: review?(enndeakin)
Comment on attachment 352781 [details] [diff] [review]
Fix remaining double quotes and missing trailing parenthesis.

This patch adds semantics to the search widget that allows assistive technologies to:
a) recognize that those search textboxes without a search button are supporting AutoCompletion, and
b) use the object attribute "autocomplete='list'" to determine that the filtering happens on a separate control and not within this control's scope.

I tested the patch in both the History sidebar and the Add-Ons manager. In the History sidebar, the STATE_AUTOCOMPLETE flag is set on the textbox's accessible, and the object attribute of autocomplete="list" is set. With the search box in the Add-Ons Manager, neither of these are present, which is correct as per the markup and logic in the XBL binding.
Depends on: 469818
I really don't like the use of aria attributes directly in xul. Maybe I just don't like the hyphenated prefix. Thinking about this though, I could see some value of having a search box with a built-in api to filter the corresponding results list as being useful, rather than having the author have to do it manually. A future feature though, but having, say, a 'results' attribute could be forward-thinking.

Does the aria-controls attribute have to be on the <textbox>? Can it be on the inner <input> element? In that case xbl:inherits="..., aria-controls=results" might be suitable.

Any thoughts?

It would also be good to fix up an issue I didn't notice before, that the searchButton getter should be comparing the attribute to true rather than using hasAttribute. And the patch here should be calling the searchButton getter instead.
(In reply to comment #13)
> I really don't like the use of aria attributes directly in xul.

Well, we do have ARIA attributes in various XUL files in Firefox and other Gecko-based apps. There sometimes is no other way to semantically put something properly for accessibility. Or did you mean to say XBL instead?

> Thinking about this though, I could see some
> value of having a search box with a built-in api to filter the corresponding
> results list as being useful, rather than having the author have to do it
> manually. A future feature though, but having, say, a 'results' attribute could
> be forward-thinking.

But again the semantics must be there to tell a11y that a particular control controls another. So you'd have to have a translation forward to the aria-controls attribute somewhere. :)

> Does the aria-controls attribute have to be on the <textbox>? Can it be on the
> inner <input> element? In that case xbl:inherits="..., aria-controls=results"
> might be suitable.

I'll have to defer to Aaron for this question. We don't create an extra accessible for the inner input element, we create a single accessible for the textbox widget.

> It would also be good to fix up an issue I didn't notice before, that the
> searchButton getter should be comparing the attribute to true rather than using
> hasAttribute. And the patch here should be calling the searchButton getter
> instead.

OK, should I make this change to the getter here, or should we have a separate bug for this?
> Well, we do have ARIA attributes in various XUL files in Firefox and other
> Gecko-based apps. There sometimes is no other way to semantically put something
> properly for accessibility. Or did you mean to say XBL instead?

I did not. I generally consider the use of aria in XUL to be a bug.

> But again the semantics must be there to tell a11y that a particular control
> controls another.

The results attribute would do that as well.

> OK, should I make this change to the getter here, or should we have a separate
> bug for this?

A separate bug would be fine.
Hi Neil, I'd like to know why you consider ARIA in XUL to be a bug. To me it:
1) Improves the user experience for screen reader users without adding much code
2) Shows the flexibility and usefulness of ARIA

Are you suggesting a different approach? If so, would it be just as little code as we are using now?
Attached patch Fix Enn's review comments. (obsolete) — Splinter Review
Is this what you had in mind? Inheriting the @results attribute to the html:input as @aria-controls works.
Attachment #352781 - Attachment is obsolete: true
Attachment #355559 - Flags: review?(enndeakin)
Attachment #352781 - Flags: review?(enndeakin)
I carried over the Mochitest from bug 469818, which it was originally a part of. I needed the refactor for other work, so decided to fold the mochitest into this patch.
Attachment #355559 - Attachment is obsolete: true
Attachment #355588 - Flags: review?(enndeakin)
Attachment #355559 - Flags: review?(enndeakin)
Flags: wanted-firefox3.1?
Whiteboard: No risk. Additional ARIA attributes do not affect anything but AT support.
IMHO we should take the patch that sets aria-controls directly and file a new bug for the "results" attribute. It comes down to whether we can filter trees and richlistboxes automatically. Just disliking the look of "aria-controls" doesn't seem convincing.
(In reply to comment #16)
> Hi Neil, I'd like to know why you consider ARIA in XUL to be a bug.

Because it's better to make stuff accessible without authors having to think about it. But "results" as an alias for "aria-controls", without adding any additional value, doesn't achieve that.
OK, it's probably best for now to just use the original patch (352781) already reviewed since it doesn't involve any extra effort. It would be nice in general though if it was 'controls' and not 'aria-controls'. We can revisit the aria-autocomplete part one day when xbl2 comes along.
Attachment #355588 - Attachment is obsolete: true
Attachment #355588 - Flags: review?(enndeakin)
Hi Neil, do you still want the below addressed?

(In reply to comment #13)
> It would also be good to fix up an issue I didn't notice before, that the
> searchButton getter should be comparing the attribute to true rather than using
> hasAttribute. And the patch here should be calling the searchButton getter
> instead.
Yes, please address that.
Attached patch Patch to go inSplinter Review
Final patch to land once tree reopens.
1. Reverted to using aria-controls.
2. Fixed getter for the @searchbutton to check for value of "true".
3. Constructor uses getter instead of checking for the attribute.
4. Mochitest included for a11y exposure. Note that I will file a follow-up bug for this to test the relations once we have bug 475298 in.
Attachment #359017 - Flags: approval1.9.1?
Pushed in changeset:
http://hg.mozilla.org/mozilla-central/rev/3dfb11111142

Note that surkov's r+ is carried over from bug 469818, of which the mochitest for this bug was originally a part of.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Target Milestone: --- → Firefox 3.2a1
Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090202 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED
Attachment #359017 - Flags: approval1.9.1? → approval1.9.1+
Pushed to m-1.9.1 in changeset:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b5a3ad7350f1
Keywords: fixed1.9.1
Blocks: 482440
You need to log in before you can comment on or make changes to this bug.