Closed
Bug 461252
Opened 16 years ago
Closed 16 years ago
First filter in Message Filters should be selected
Categories
(MailNews Core :: Filters, defect)
MailNews Core
Filters
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9.1b2
People
(Reporter: beckley, Assigned: beckley)
Details
Attachments
(1 file, 2 obsolete files)
1002 bytes,
patch
|
beckley
:
review+
beckley
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•16 years ago
|
||
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)
Comment 2•16 years ago
|
||
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]);
Comment 3•16 years ago
|
||
I would make setFolder just clear the selection before calling rebuildFilterList and select the first item (if any) afterwards.
Comment 4•16 years ago
|
||
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 5•16 years ago
|
||
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-
Assignee | ||
Comment 6•16 years ago
|
||
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 7•16 years ago
|
||
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 8•16 years ago
|
||
Comment on attachment 344986 [details] [diff] [review] Simpler implementation looks good, thanks!
Attachment #344986 -
Flags: superreview?(bienvenu) → superreview+
Updated•16 years ago
|
Attachment #344986 -
Flags: review?(mkmelin+mozilla) → review+
Comment 9•16 years ago
|
||
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.
Assignee | ||
Comment 10•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Comment 11•16 years ago
|
||
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]
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Comment 12•16 years ago
|
||
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.
Description
•