FAYT style folder navigation for new folder picker widget

RESOLVED FIXED

Status

MailNews Core
Backend
--
enhancement
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

({fixed1.8.1})

Trunk
fixed1.8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

12 years ago
See bug 302120 comment 7.
(Assignee)

Comment 1

12 years ago
Created attachment 207922 [details] [diff] [review]
In order to support this we need to break out the incremental search routine
Assignee: nobody → neil.parkwaycc.co.uk
Status: NEW → ASSIGNED
Attachment #207922 - Flags: superreview?(jag)
Attachment #207922 - Flags: review?(mconnor)

Comment 2

12 years ago
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+
(Assignee)

Updated

12 years ago
Attachment #207922 - Flags: review?(mconnor) → review?(enndeakin)

Comment 3

12 years ago
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+
(Assignee)

Comment 4

12 years ago
Created attachment 228940 [details] [diff] [review]
attachment 207922 [details] [diff] [review] updated for bitrot and checked in
Attachment #207922 - Attachment is obsolete: true
(Assignee)

Comment 5

12 years ago
Created attachment 228942 [details] [diff] [review]
mailnews changes
Attachment #228942 - Flags: superreview?(mscott)

Comment 6

12 years ago
Comment on attachment 228942 [details] [diff] [review]
mailnews changes

sweet!
Attachment #228942 - Flags: superreview?(mscott) → superreview+
(Assignee)

Comment 7

12 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 8

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

Comment 9

11 years ago
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

11 years ago
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?

Comment 11

11 years ago
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+

Comment 13

11 years ago
(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

11 years ago
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+

Comment 15

11 years ago
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 ...

Comment 17

11 years ago
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

11 years ago
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

11 years ago
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

11 years ago
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

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

Comment 22

11 years ago
(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

11 years ago
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

11 years ago
Created attachment 235458 [details] [diff] [review]
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.

Updated

11 years ago
Keywords: fixed1.8.1
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.