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

Status: VERIFIED FIXED

(Firefox :: Disability Access, defect)

Description

[Marco Zehe (:MarcoZ)]

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

Comment 1

[Marco Zehe (:MarcoZ)]

Attachment: Patch that manually applies the attributes to each individual instance of the search textbox.

This is the not so elegant method.

Comment 2

[Dão Gottwald [::dao]]

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

Comment 3

[Marco Zehe (:MarcoZ)]

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

Comment 4

[Dão Gottwald [::dao]]

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

Comment 5

[Dão Gottwald [::dao]]

(There's no xbl constructor yet in that binding.)

Comment 6

[Marco Zehe (:MarcoZ)]

Attachment: Patch with logic in XBL binding

Is this how it should be done in the XBL binding?

Comment 7

[Dão Gottwald [::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.

Comment 8

[Marco Zehe (:MarcoZ)]

Attachment: Patch 2

Addressing Dao's comments.

Comment 9

[Dão Gottwald [::dao]]

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.

Comment 10

[Marco Zehe (:MarcoZ)]

Attachment: Fix remaining double quotes and missing trailing parenthesis.

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.

Comment 11

[Dão Gottwald [::dao]]

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.

Comment 12

[Marco Zehe (:MarcoZ)]

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.

Comment 13

[Neil Deakin]

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.

Comment 14

[Marco Zehe (:MarcoZ)]

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

Comment 15

[Neil Deakin]

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

Comment 16

[Aaron Leventhal]

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?

Comment 17

[Marco Zehe (:MarcoZ)]

Attachment: Fix Enn's review comments.

Is this what you had in mind? Inheriting the @results attribute to the html:input as @aria-controls works.

Comment 18

[Marco Zehe (:MarcoZ)]

Attachment: Same patch, but containing a11y mochitest

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.

Comment 19

[Dão Gottwald [::dao]]

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.

Comment 20

[Dão Gottwald [::dao]]

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

Comment 21

[Neil Deakin]

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.

Comment 22

[Marco Zehe (:MarcoZ)]

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.

Comment 23

[Dão Gottwald [::dao]]

Yes, please address that.

Comment 24

[Marco Zehe (:MarcoZ)]

Attachment: Patch to go in

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.

Comment 25

[Marco Zehe (:MarcoZ)]

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.

Comment 26

[Marco Zehe (:MarcoZ)]

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

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 359017 [details] [diff] [review]
Patch to go in

a191=beltzner

Comment 28

[Marco Zehe (:MarcoZ)]

Pushed to m-1.9.1 in changeset:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b5a3ad7350f1