516680 - [faceted search] Add keyboard navigation capabilities to new search

Status: VERIFIED FIXED

(Thunderbird :: Search, defect, P2)

Description

[Marco Zehe (:MarcoZ)]

The new search UI is completely keyboard inaccessible, breaking not only screen reader users, but anyone who primarily uses the keyboard to operate Thunderbird. This will be perceived as a MASSIVE regression by end users who are used to a working search facility.

Steps to reproduce:
1. Open Shredder.
2. Navigate to the "Search Everywhere" field using the keyboard. This still works like it did before the new search landed.
3. Enter a search term.
4. Press ENTER. A new tab opens with the new search UI.
5. Tab around the UI.

Result: The only focusable items are the search field, the tab bar, and the two checkboxes "From" and "To". None of the other checkboxes, nor the actual list of search results are reachable using the keyboard. For a keyboard user, there is no way to open any of the search results without having to use the mouse. For screen reader users, there is no way to examine the search results before deciding to open one.

Comment 1

[David Ascher (:davida)]

I've thought a bit about that, will post some thoughts later.

Comment 2

[Wayne Mery (:wsmwk)]

clarifying subject - now that we now have N+1 ways to search :)

Comment 3

[David Ascher (:davida)]

I think we'll need to do this in two phases, with the second one probably not blocking-tb3.

phase 1:

  - add tabindex to the right DOM elements, so that tabbing works
  - come up with some CSS for :focus elements that doesn't disrupt the layout, but likely just changes colors, or underlines, or something.
  - move code that's currently in click handler blocks into methods, and add keypress events for VK_ENTER/VK_RETURN/(VK_SPACE?) to trigger those methods (along w/ the click events).

early testing shows that this will give us _some keyboard nav_, which achieves an important accessibility goal.  

A second phase would be to add what i'd call geometric keyboard nav, so that people can navigate between the two columns, for example -- it's hard to do that in a way that works across all keyboard layouts.  I'm not sure whether it's safe to assume that locales imply keyboard layouts.  I'm sure that using the up/down/left/right arrows will be controversial.  Hence, no clear solution there, and no time to do it for RC1.

I'll put up a patch to do phase 1 soon.  This won't have l10n impact.

MarcoZ: from an a11y POV, do you care if it's ENTER/RETURN, SPACE, or both?

Comment 4

[David Ascher (:davida)]

may as well take it for now.

Comment 5

[David Ascher (:davida)]

Attachment: patch v1

First, bryan, if you could give this a try, and let me know what you think. Then phil will tell me how i did it all wrong.

Comment 6

[Phil Ringnalda (:philor)]

Nah, it's XBL+HTML - I won't have any idea about right from wrong, so I'll just nod sagely with an oddly vacant expression on my face.

Comment 7

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 401267 [details] [diff] [review]
patch v1

works pretty well for me.  I think you missed the "show all as list" button but with that fixed ui-r+

Comment 8

[Marco Zehe (:MarcoZ)]

(In reply to comment #3)
> MarcoZ: from an a11y POV, do you care if it's ENTER/RETURN, SPACE, or both?

Well, for a list of messages displayed as Search results, ENTER is the more consistent choice because ENTER also opens messages from the main messages list.

Comment 9

[David Ascher (:davida)]

Marco: ok. You'd agree that for toggling checkboxes and selecting "facet values" (like "add _this_ person to the list of constraints", SPACE is the common shortcut (it's what web pages do, AFAICT).

Thinking about it a bit, it seems that SPACE is for clicks that modify an existing page, but ENTER is for navigation.  That would mean that "show all as list" would require an ENTER, but "sort by date" would require a SPACE.

hmm...

Comment 10

[Marco Zehe (:MarcoZ)]

(In reply to comment #9)
> Marco: ok. You'd agree that for toggling checkboxes and selecting "facet
> values" (like "add _this_ person to the list of constraints", SPACE is the
> common shortcut (it's what web pages do, AFAICT).

Correct.

> Thinking about it a bit, it seems that SPACE is for clicks that modify an
> existing page, but ENTER is for navigation.  That would mean that "show all as
> list" would require an ENTER, but "sort by date" would require a SPACE.

Is "Show as List" a button? If so, SPACE is usually also used to activate buttons. Links and list items are usually activated by pressing Enter.

Comment 11

[Phil Ringnalda (:philor)]

Comment on attachment 401267 [details] [diff] [review]
patch v1

>+        subject.onkeypress = function(aEvent) {

What is this "subject" of which you speak (other than "undefined, so it breaks the results display")?

Comment 12

[Bryan Clark (DevTools PM) [:clarkbw]]

hmm, is that patch assuming that bug 516261 landed when it's just sitting in checkin-needed limbo?

Comment 13

[Phil Ringnalda (:philor)]

Comment on attachment 401267 [details] [diff] [review]
patch v1

Fun. I don't think I've ever even come remotely close to being too quick to get to a review before.

Comment 14

[David Ascher (:davida)]

I'd like to not regress accessibility if we can avoid it, so approving blocking-tb3.

Comment 15

[Phil Ringnalda (:philor)]

(In reply to comment #10)
> Is "Show as List" a button?

MarcoZ: a "fun" question, as yours usually are :). Visually, it's some black text with a transparent background which when moused over gets a blue background with rounded corners, so I'd say visually it's 55% button, 45% link. For a screen reader, it's an <html:span> with an onclick attribute. What does it sound to you like it is? From my memory of using screen readers on HTML, you know what controls are by hearing "button: show all as list" or "link: show all as list" so what do you hear from <html:span onclick="doSomething();">Show all as list</html:span>?

davida: is there a reason you're making buttons out of spans, other than not wanting to have any of the native appearance of buttons?

Comment 16

[David Ascher (:davida)]

philor: nope, it's all about appearance.  I'm fine w/ making them <button>s if that helps the screenreaders.

Comment 17

[Marco Zehe (:MarcoZ)]

(In reply to comment #15)
> For a screen reader, it's an <html:span> with an onclick attribute. What does
> it sound to you like it is? From my memory of using screen readers on HTML, you
> know what controls are by hearing "button: show all as list" or "link: show all
> as list" so what do you hear from <html:span onclick="doSomething();">Show all
> as list</html:span>?

It would say something like "Show as list, clickable".

Well, thanks to WAI-ARIA, we can still make these HTML:span elements appear to screen readers as buttons, but retain the visual effects Davida is seeking: For each thing that would conceptually be a button, like this "Show als list", add:

role="button"

besides the onclick handler to the attributes of the relevant elements. That will let screen readers know that this conceptually is a button.

The only other thing that needs to be made sure is that SPACE will actually activate the button, as is done with regular UI elements.

Comment 18

[David Ascher (:davida)]

Attachment: debitrotted patch

debitrotted, and added keyboard handling for 'show all as list', which i'd missed.

Comment 19

[Phil Ringnalda (:philor)]

(In reply to comment #9)
> Thinking about it a bit, it seems that SPACE is for clicks that modify an
> existing page, but ENTER is for navigation.  That would mean that "show all as
> list" would require an ENTER, but "sort by date" would require a SPACE.

Not really, no, because you don't know what "some random thing in a web page" will actually do. Best practices aside, whether it's a link or a button or a checkbox, it still might call a script that updates the page in place, or load a new page (or a new page that looks just like the old page with a part changed), or open a new page in a new window/tab. So keyboard nav is based on what a control *is* rather than what it does: if it's a link, you activate it with enter no matter what effect it has, and you know whether hitting space is going to scroll the page or activate the focused control based on whether focus is on a form control that'll activate, or a link that won't.

Buttons get a little complicated since on Windows and Linux, in HTML or XUL, you can activate a button with either enter or space, but on OS X in HTML you can active a button with either enter or space, but in XUL and native dialogs, there's a difference between the focused button (which has an outline of blue or dark grey) which you active with space, and the default button (which has the whole button either blue or dark grey) which you activate with enter.

It matters which one a thing is because when you have a link-thing focused, you expect that space will scroll the page, but when you have a button-thing focused, you expect that space will activate it (and you may expect that enter will activate it, or you may expect that enter will activate some other button, but we can ignore that case since we don't have a default button, and Mac users shouldn't be focusing a button and then expecting that space will *not* activate the button, since they should have their HTML glasses on and realize that there's no default button around).

So, first we have to decide which are link-things and which are button-things. I would assert that all the blue text in the sidebar, that becomes underlined when you focus it, is unquestionably a link-thing, as are the do-not-enter symbols associated with them, since they don't have any button-like appearance when hovered, focused, or activated, the timeline-toggle image (right now, since it doesn't have any button-like appearance, though perhaps it should since it's a pretty button-like behavior), and the blue text of the subjects, while the "List all 10", "Show all as list", "relevance" and "date" are buttons, based on their :hover appearance (which they need to have for :focus, too).

Once we're agreed on what spans are links, and what spans are buttons, we need to put role="link" and role="button" on them, so Marco knows what we decided, and make sure that the button ones activate on both enter and space, and for space preventDefault on the event so that the page doesn't scroll, and make sure that the link ones activate on enter, and don't do anything other than allow the page to scroll on space.

Then all we have to do is deal with the big blue boxy elephant in the room. How does someone who doesn't use a mouse limit their results by date? Does the timeline, which I can use to limit by some random month, or by a particular month if my results have matches in at least every other month of a year, so I don't have to guess what month a totally unlabelled bar "um, maybe four or five widths from the start of the year" represents, even *exist*, as something completely unusable, to Marco?

Comment 20

[Phil Ringnalda (:philor)]

All that typing made me forget why I was talking about preventDefault - clicking the timeline toggle shows and hides the timeline, but pressing space with it focused "clicks" the "Show more hits" button and doesn't prevent default, so you scroll down to some random spot in the middle of the page and wonder wtf happened.

Comment 21

[Phil Ringnalda (:philor)]

Comment on attachment 403127 [details] [diff] [review]
debitrotted patch

And by all that, I mean "r-, another iteration please."

Comment 22

[David Ascher (:davida)]

Roger on all that.  I'll do a next rev after string freeze.  As to the timeline localization, I suspect what I'd like is to hear from MarcoZ what it's like to deal w/ Google's timeline: 

http://www.google.ca/search?q=accessibility&hl=en&sa=G&tbo=1&output=search&tbs=tl:1

I suspect we won't be able to get to full a11y of the timeline for TB3, but Google's timeline seems to provide a possible path.

Comment 23

[David Ascher (:davida)]

I think that with the latest changes to the faceting sidebar, the blue things on the left (facet values), which bring up menus when clicked, are now button-things.  I'll implement w/ that understanding, and can revisit if I'm wrong.

Comment 24

[David Ascher (:davida)]

Attachment: WIP

Attaching WIP, as I'm not sure how much time I'll have over the next few days.  I'm pretty sure this is quite a bit better than the last one, and also sure that I haven't deal with all of the many conditions (role attributes, consistently doing the right thing w/ space vs. enter, etc.)

Phil, if you have a few minutes to help me identify the remaining TODOs, that'd be great.

Comment 25

[Mark Banner (:standard8)]

Phil, David is busy at the moment, would you be able to take this on?

Comment 26

[Phil Ringnalda (:philor)]

That depends, can you get me fired from my job before the code freeze? If not, then no.

Comment 27

[Mark Banner (:standard8)]

Blake, any chance you can take this?

Comment 28

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #27)
> Blake, any chance you can take this?

If Phil could identify the TODOs, then I could probably do something with it, but based on my non-progress with bug 516680, I don't think I could take it all on my own.

Comment 29

[Phil Ringnalda (:philor)]

TODO:

* Don't totally break the "List all n" buttons for facets
* Don't totally break including/excluding facets
* Don't activate the blue underlined text of the subject of a result on space, those are links
* Include the "More >>" button at the bottom of the results in tabbing, make it activate on space
* Don't use an undeclared var "label" in "<method name="show">"

Can't promise that's all of them, since it's hard to tell what will be behind the broken doors.

Comment 30

[Phil Ringnalda (:philor)]

Only thing I hate more than midair warnings is the absence of midair warnings.

Comment 31

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch fixing the suggested things.

(In reply to comment #29)
> * Don't totally break the "List all n" buttons for facets

Fixed.  It also activates on space, like the other buttons.

> * Don't totally break including/excluding facets

Fixed.

> * Don't activate the blue underlined text of the subject of a result on space,
> those are links

Done.  They now activate with return.

> * Include the "More >>" button at the bottom of the results in tabbing, make
> it activate on space

Done.

> * Don't use an undeclared var "label" in "<method name="show">"

Done.

Other things I've found, but might put in another patch, if you're okay with that:
1) The "None" tag facet is still broken.  It should be fixable, but I wanted to throw this up tonight, to show progress, and get some preliminary feedback.
2) We get an ugly-ish error when we hit space when we're on a subject.

Thanks,
Blake.

Comment 32

[Phil Ringnalda (:philor)]

The none facet thing is bug 515650, but I bet asuth would be willing to share :)

Comment 33

[Phil Ringnalda (:philor)]

And I'm pretty sure the SpaceFit from SpaceHit is just something screwy when you change from running -1.9.1 to -central, since I've seen it with the threadpane too, and it goes away when you close+reopen. Probably fastload or some other thing that I've never quite understood.

So, this seems to work the right way everywhere, and I think we're down to two things:

* Everything that's acting like a link which isn't an <a> needs a |role="link"| and everything that's acting like a button which isn't a <button> needs a |role="button"|

* Rather than hardcoding |event.charCode == 32|, mxr says we want |event.charCode == KeyEvent.DOM_VK_SPACE|

Comment 34

[Blake Winton (:bwinton) (:☕️)]

Attachment: A better patch, with more roles, and stuff.

(In reply to comment #33)
> * Everything that's acting like a link which isn't an <a> needs a |role="link"|
> and everything that's acting like a button which isn't a <button> needs a
> |role="button"|

Fixed, I think.  At least, I fixed everything I could think of.

> * Rather than hardcoding |event.charCode == 32|, mxr says we want
> |event.charCode == KeyEvent.DOM_VK_SPACE|

Done.

Thanks,
Blake.

Comment 35

[Phil Ringnalda (:philor)]

Comment on attachment 409734 [details] [diff] [review]
A better patch, with more roles, and stuff.

I'd be delighted if we wind up with enough time for MarcoZ to look at a tryserver build, but it all looks right to me, so r=philor

Comment 36

[Blake Winton (:bwinton) (:☕️)]

So the tryserver builds have completed:
Linux - http://s3.mozillamessaging.com/build/try-server/2009-11-02_13:[e-mail]-gloda-a11y/[e-mail]2

Mac - http://s3.mozillamessaging.com/build/try-server/2009-11-02_13:[e-mail]-gloda-a11y/[e-mail]

Windows is reporting a failure, but here are the links, just in case:

Windows - http://s3.mozillamessaging.com/build/try-server/2009-11-02_13:[e-mail]-gloda-a11y/[e-mail]

Windows Installer - http://s3.mozillamessaging.com/build/try-server/2009-11-02_13:[e-mail]-gloda-a11y/[e-mail]

Thanks,
Blake.

Comment 37

[Marco Zehe (:MarcoZ)]

Something's wrong with that Windows build. When I enter a search term, my inbox blanks out, there's no way to get it back, and no search tab is opened.

I suspect this has to do with possible incompatibilities between Thunderbird and the mozilla-central build system. The try server does not build against the 1.9.1 branch.

In other words, I can't really test it since the search doesn't seem to work in this try build.

Comment 38

[Mark Banner (:standard8)]

(In reply to comment #37)
> I suspect this has to do with possible incompatibilities between Thunderbird
> and the mozilla-central build system. The try server does not build against the
> 1.9.1 branch.

The try server can build 1.9.1 branch but not by default.

> In other words, I can't really test it since the search doesn't seem to work in
> this try build.

I've fired off a new set of builds and I'll keep an eye on them.

Comment 39

[Mark Banner (:standard8)]

New Windows builds now available:

http://s3.mozillamessaging.com/build/try-server/2009-11-03_01:[e-mail]-gloda-a11y-v2/[e-mail]
http://s3.mozillamessaging.com/build/try-server/2009-11-03_01:[e-mail]-gloda-a11y-v2/[e-mail]

Comment 40

[Marco Zehe (:MarcoZ)]

This try-server build worked for me! Here are my findings:

First, this is already a *huge* improvement over the current regular nightlies.

Now for the nits:
After performing a search, when tabbing through the newly opened tab, the checkboxes are read correctly.
Next, you get a couple of tab stops that point to links in which the search term has been found. In my case the term I used was found in 3 folders.
All these tab stops land on "section" elements, not on the actual links to the folders. The links to the folders are one level down, and to the next sibling in the hierarchy. So, the general vicinity is correct, the tab stop is just on an element whose focus speaking doesn't make sense.

Continuing to tab, there is one button the focus lands on which does not have a label. It's the one immediately preceeding the "Open as list" button in the tab order.

And that's all I found! :)

Comment 41

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, hopefully with MarcoZ's nits fixed.

Okay, Marco, I believe your nits have been fixed, at least as much as they can be without adding new strings.

(Try server builds against comm-1.9.1 are submitted, and should be running any minute now.)

Thanks,
Blake.

Comment 42

[Blake Winton (:bwinton) (:☕️)]

And here are the try-server builds:

Linux:
http://s3.mozillamessaging.com/build/try-server/2009-11-03_08:[e-mail]-gloda-a11y/[e-mail]2

MacOS:
http://s3.mozillamessaging.com/build/try-server/2009-11-03_08:[e-mail]-gloda-a11y/[e-mail]

Windows:
http://s3.mozillamessaging.com/build/try-server/2009-11-03_08:[e-mail]-gloda-a11y/[e-mail]
http://s3.mozillamessaging.com/build/try-server/2009-11-03_08:[e-mail]-gloda-a11y/[e-mail]

Thanks,
Blake.

Comment 43

[Marco Zehe (:MarcoZ)]

I just tested this newest try-server build, and we're good to go! The button that opens the graph showing the search results overtime is still not labelled, but since this opens an inaccessible part of the UI that builds on Canvas, we can go without that. The others  that were previously not speaking are now speaking nicely! Good work!

Please file a follow-up bug for the graph part, and please also relnote that that button should not be pressed by a screen reader user.

Comment 44

[Blake Winton (:bwinton) (:☕️)]

Pushed as:
http://hg.mozilla.org/releases/comm-1.9.1/rev/8a9dfdd70b98
http://hg.mozilla.org/comm-central/rev/51460ab23791

Comment 45

[Blake Winton (:bwinton) (:☕️)]

And the follow-up bug is bug 526311.

Comment 46

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.6pre) Gecko/20091104 Shredder/3.0pre