322622 - FAYT style folder navigation for new folder picker widget

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement)

Description

[[e-mail]]

See bug 302120 comment 7.

Comment 1

[[e-mail]]

Attachment: In order to support this we need to break out the incremental search routine

Comment 2

[jag (Peter Annema)]

Comment on attachment 207922 [details] [diff] [review]
In order to support this we need to break out the incremental search routine

I like your trick of comparing the first n-1 chars to the last n-1 chars.

I would've used something slightly simpler, e.g.

  if (/^(.)\1*$/.test(incrementalString))
    incrementalString = incrementalString[0];


I know you're just moving code around, but could you put that magic "1000" into a field while you're there? I'm guessing we don't have some LookAndFeel-ish place to get that value from :-(

Comment 3

[Neil Deakin]

Comment on attachment 207922 [details] [diff] [review]
In order to support this we need to break out the incremental search routine

>-         var c = this.currentIndex;
>          if (event.charCode == ' '.charCodeAt(0) && !this.view.selection.single) {
>-           if (!this.view.selection.isSelected(c) || event.ctrlKey) {
>+           var c = this.currentIndex;
>+           if (event.ctrlKey || !this.view.selection.isSelected(c))
>              this.view.selection.toggleSelect(c);
>-           }

Not a big deal, but a similar change wasn't made in the toolkit code.

Comment 4

[[e-mail]]

Attachment: attachment 207922 updated for bitrot and checked in

Comment 5

[[e-mail]]

Attachment: mailnews changes

Comment 6

[Scott MacGregor]

Comment on attachment 228942 [details] [diff] [review]
mailnews changes

sweet!

Comment 7

[[e-mail]]

Fix checked in.

Comment 8

[Aaron Leventhal]

What's being done to consider accessibility with a screen reader with the new widget?

Comment 9

[Scott MacGregor]

Neil, do you think we should try to get these two patches nominated for the branch? I think it's a pretty big usability win, for seamonkey and thunderbird. We might have to explain why the tree.xml change is safe for firefox.

Comment 10

[Scott MacGregor]

Comment on attachment 228940 [details] [diff] [review]
attachment 207922 [details] [diff] [review] updated for bitrot and checked in

This toolkit change to tree.xml refactors key navigation code for the tree into a separate method that others can call.

This change allows the mail code to call keyNavigate on our new folder picker widget. It's a big usability win for Thunderbird and Seamonkey and shouldn't impact Firefox. It's been revied and sr'ed by jag and endeakin.

It's been on the trunk since July 12th.

Comment 11

[Aaron Leventhal]

I don't think it should go into 1.8.1 until we check to see if it's accessible with a screen reader. We now have Thunderbird in a usable state with JAWS, let's not regress that.

In addition, the review requirements clearly state that for new widgets you need to check with the a11y module people. That could be myself or Mark Pilgrim. I don't like the idea of having to find out about new widgets accidentally as I browse the bug database.

Comment 12

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

Comment on attachment 228940 [details] [diff] [review]
attachment 207922 [details] [diff] [review] updated for bitrot and checked in

a=drivers, who likes us three weeks of baking!

Comment 13

[Aaron Leventhal]

(In fact I asked about accessibility in this bug back on July 12 and only got an answer on IRC that no, a11y for screen readers wasn't looked at.)

Comment 14

[Scott MacGregor]

Comment on attachment 228942 [details] [diff] [review]
mailnews changes

great we got approval on the toolkit change, so I'll do an approval for the mail specific change. 

Neil do you want to land both patches on the branch or would you like me to?

Comment 15

[Aaron Leventhal]

Are you guys going to deal with any a11y regressions in Thunderbird 2 that this causes?

Comment 16

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

Scott, I agree with aaron fwiw, and we should make sure that we not regress on the JAWS compliance here. It shouldn't be that hard to do some quick testing and fixing. We approved this assuming that we wouldn't be regressing things ...

Comment 17

[Scott MacGregor]

Hey, Aaron, it's not clear to me how a toolkit patch that refactors a piece of code into a method any one can call blocks accessibility?

For the mailnews patch, there is no new widget here. We're adding FAYT support to an existing widget that shipped in 1.5. From your comment about screen reader support in 1.5 already being in a usable state, it sounds like the widget is already compliant. This is a simple change to make it easier for a keyboard user to select a folder in the advanced search dialog by typing the first letter in the folder name. If you are still concerned, feel free to try it in a trunk build where it has been since early July for either the suite or Thunderbird. You can try which ever one is most convenient.

Comment 18

[Scott MacGregor]

also note that the FAYT code being used is the tree widget FAYT code and has been around and shipping in mozilla products since 2003. Hope that helps.

Comment 19

[Aaron Leventhal]

Are we talking about Ctrl+Shift+F and then dropping down the "Search for messages in" combobox?

I understand what you're talking about now, and it sounds like it should get checked into the branch.

However, I'm seeing some bugs that I hope are already filed.
First, when I bring up Ctrl+Shift+F, focus is lost and I can't regain it.
Second, the advanced search is nowhere in the menus, so it's not discoverable from what I can see.

Comment 20

[Aaron Leventhal]

There are problems with the widget --  this is on an 8/10 Thunderbird 2 build without this patch

- I see focus events being fired but Window-Eyes is not reading items as I arrow to them.
- It could be that the new widget is what's completely eating tab/shift+tab presses in the dialog

Comment 21

[Aaron Leventhal]

Indeed, JAWS is not reading the focus in the tree dropdown either.

Comment 22

[Scott MacGregor]

(In reply to comment #19)
> Are we talking about Ctrl+Shift+F and then dropping down the "Search for
> messages in" combobox?

Yup.

> 
> I understand what you're talking about now, and it sounds like it should get
> checked into the branch.
> 
> However, I'm seeing some bugs that I hope are already filed.
> First, when I bring up Ctrl+Shift+F, focus is lost and I can't regain it.

I don't think that's related to this bug. 

> Second, the advanced search is nowhere in the menus, so it's not discoverable
> from what I can see.

Edit / Find / Search Messages

Thanks for trying this out.

Comment 23

[Aaron Leventhal]

I filed bug 348265 for the screen reader issues in the folder picker widget.

If this FAYT fix can get checked in soon I'll test again. You never know, it could help with some of the other issues.

Comment 24

[Scott MacGregor]

Attachment: both patches merged to the 1.8.1 branch

There were some minor merge conflicts I had to account for on the 1.8.1 branch.

Per aaronl and beltzner, I'll get this landed today.