Closed Bug 322622 Opened 15 years ago Closed 15 years ago

FAYT style folder navigation for new folder picker widget

Categories

(MailNews Core :: Backend, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: neil, Assigned: neil)

Details

(Keywords: fixed1.8.1)

Attachments

(3 files, 1 obsolete file)

Assignee: nobody → neil.parkwaycc.co.uk
Status: NEW → ASSIGNED
Attachment #207922 - Flags: superreview?(jag)
Attachment #207922 - Flags: review?(mconnor)
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 :-(
Attachment #207922 - Flags: superreview?(jag) → superreview+
Attachment #207922 - Flags: review?(mconnor) → review?(enndeakin)
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.
Attachment #207922 - Flags: review?(enndeakin) → review+
Attachment #207922 - Attachment is obsolete: true
Attached patch mailnews changesSplinter Review
Attachment #228942 - Flags: superreview?(mscott)
Comment on attachment 228942 [details] [diff] [review]
mailnews changes

sweet!
Attachment #228942 - Flags: superreview?(mscott) → superreview+
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
What's being done to consider accessibility with a screen reader with the new widget?
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 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.
Attachment #228940 - Flags: approval1.8.1?
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 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!
Attachment #228940 - Flags: approval1.8.1? → approval1.8.1+
(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 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?
Attachment #228942 - Flags: approval-thunderbird2+
Are you guys going to deal with any a11y regressions in Thunderbird 2 that this causes?
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 ...
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.
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.
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.
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
Indeed, JAWS is not reading the focus in the tree dropdown either.
(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.
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.
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.
Keywords: fixed1.8.1
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.