Closed Bug 461252 Opened 13 years ago Closed 13 years ago

First filter in Message Filters should be selected

Categories

(MailNews Core :: Filters, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: beckley, Assigned: beckley)

Details

Attachments

(1 file, 2 obsolete files)

When you open up the the Message Filters window, the first filter in the list gets the focus, but it is not selected.  This makes various buttons and other UI elements disabled until you click on a filter.  It's visually unappealing and a little jarring when you do select a filter and all the UI elements become enabled.  Also, the focus but no selection is a weird state that you can't get back in to because from then on there is always just one filter selected.

It would be better to just always select that first filter.
Attached patch Patch to select first filter (obsolete) — Splinter Review
Pretty self-explanatory.  Adding Bryan as ui-review in case he sees something about focus without selection that I'm missing.
Attachment #344449 - Flags: ui-review?(clarkbw)
Attachment #344449 - Flags: superreview?(bienvenu)
Attachment #344449 - Flags: review?(bienvenu)
Neil might have some thoughts about an easier way to do this...

you could get rid of the local restoredSelection, and instead set selectFirstFilter to false if we add an item to the selection, and then the setting of the selection could look like this:

+  if (selectFirstFilter && list.childNodes.length > 1)
+    list.addItemToSelection(list.childNodes[1]);
I would make setFolder just clear the selection before calling rebuildFilterList and select the first item (if any) afterwards.
I like it!  I'm assuming this is going to select first on first open of the window, changing folder, and creation of first filter.  Though perhaps the last one is just a case of selecting the filter you just created.  

Essentially I'd recommend this window always starts with focus on a filter.  After edit, the edited filter.  After creation the created filter.  And like you're doing the first filter after changing folder or first opening.  Even delete could select the filter above (or below) the deleted filter after delete.
Comment on attachment 344449 [details] [diff] [review]
Patch to select first filter

minusing - Neil's suggestion sounds cleaner to me.
Attachment #344449 - Flags: superreview?(bienvenu)
Attachment #344449 - Flags: superreview-
Attachment #344449 - Flags: review?(bienvenu)
Attachment #344449 - Flags: review-
Attached patch Simpler implementation (obsolete) — Splinter Review
Here's a new patch using Neil's suggestion.
Attachment #344449 - Attachment is obsolete: true
Attachment #344986 - Flags: ui-review?(clarkbw)
Attachment #344986 - Flags: superreview?(bienvenu)
Attachment #344986 - Flags: review?(neil)
Attachment #344449 - Flags: ui-review?(clarkbw)
Comment on attachment 344986 [details] [diff] [review]
Simpler implementation

I don't actually build this file...
Attachment #344986 - Flags: review?(neil) → review?(mkmelin+mozilla)
Comment on attachment 344986 [details] [diff] [review]
Simpler implementation

looks good, thanks!
Attachment #344986 - Flags: superreview?(bienvenu) → superreview+
Attachment #344986 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 344986 [details] [diff] [review]
Simpler implementation

Nit: remove the spaces from the empty line. And while you're at it, ad a dot at the end of the comment sentence.
Minor changes based on Magnus' comments.  Continuing r+sr.  Ready to be checked in.
Attachment #344986 - Attachment is obsolete: true
Attachment #345177 - Flags: superreview+
Attachment #345177 - Flags: review+
Attachment #344986 - Flags: ui-review?(clarkbw)
Keywords: checkin-needed
Comment on attachment 345177 [details] [diff] [review]
Final patch
[Checkin: Comment 11]

http://hg.mozilla.org/comm-central/rev/e81e3f13af0e
Attachment #345177 - Attachment description: Final patch → Final patch [Checkin: Comment 11]
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Comment on attachment 345177 [details] [diff] [review]
Final patch
[Checkin: Comment 11]


PS: Please, use Unix (= LF) eol in the future.
You need to log in before you can comment on or make changes to this bug.