Closed Bug 511989 Opened 13 years ago Closed 12 years ago

Port Bug 461252 - First filter in Message Filters should be selected

Categories

(SeaMonkey :: MailNews: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0

People

(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)

Details

(Keywords: fixed-seamonkey2.0)

Attachments

(1 file, 3 obsolete files)

Attached patch proposed patch (obsolete) — Splinter Review
Until now when opening Message Filters, the first filter is neither selected nor focussed. Thus the action buttons and keys that require a filter to be selected (e.g. Edit/Return or toggle/Space) are disabled. When opening that dialog you probably want to work on filters so that's where the focus should be (the cursor keys can then be used to directly select a different filter as well).

I moved the assignment of gFilterTree.view up and removed updateButtons() there because the assignment now needs to be done before selectServer() is called which calls setServer() which in turn calls updateButtons() (which also accesses gFilterTree.view). Let me know if I overlooked any undesired implications that might have.
Attachment #395959 - Flags: review?(mnyromyr)
I just noticed that with my initial patch updateButtons() wouldn't be called anymore if the second firstItem check failed. Suggestion:

    if (firstItem)
        selectServer(firstItem);
    else
        updateButtons();
Attachment #395959 - Flags: review?(mnyromyr) → review-
Comment on attachment 395959 [details] [diff] [review]
proposed patch

I wonder if anybody tested this from a accessibility point of view?

Imagine having multiple accounts. Open the dialog. An account gets chosen, the focus jumps into the tree view. Hit Alt-f to focus the account list without opening. Try to use the up and down arrow keys to select another account...
As it is, it's extremely arduous to use and violates user expections.
Attached patch patch v2 (obsolete) — Splinter Review
(In reply to comment #2)
> Imagine having multiple accounts. Open the dialog. An account gets chosen, the
> focus jumps into the tree view. Hit Alt-f to focus the account list without
> opening. Try to use the up and down arrow keys to select another account...
> As it is, it's extremely arduous to use and violates user expections.

Good point. Let's limit this to the opening case then. I kept the other changes as per my previous comments.
Attachment #395959 - Attachment is obsolete: true
Attachment #396131 - Flags: review?(mnyromyr)
Comment on attachment 396131 [details] [diff] [review]
patch v2

>+    if (gFilterTreeView.rowCount) {
>+        gFilterTreeView.selection.select(0);
>+        gFilterTree.focus();
>     }

Please follow SM MailNews bracing style,
see <https://wiki.mozilla.org/index.php?title=SeaMonkey:MailNews:CodingStyle>.

r=me with that.
Attachment #396131 - Flags: review?(mnyromyr) → review+
Attached patch patch v2a (obsolete) — Splinter Review
Attachment #396131 - Attachment is obsolete: true
Attachment #396734 - Flags: superreview?(neil)
Attachment #396734 - Flags: review+
Comment on attachment 396734 [details] [diff] [review]
patch v2a

>+    if (firstItem)
>         selectServer(firstItem);
>+    else
>+        updateButtons();
What if the initial server has no filters?

>+    // Select and focus the first item in the list, if there is one.
>+    if (gFilterTreeView.rowCount)
>+    {
>+        gFilterTreeView.selection.select(0);
Offhand it seems to me that selectServer should do this.

>+        gFilterTree.focus();
I think we probably always want to do this on startup, as we currently have no focus in this window, although the server list is obviously a strong contender.
Attached patch patch v3Splinter Review
(In reply to comment #6)
> What if the initial server has no filters?

As discussed via IRC, updateButtons is still called transitively (see comment 0).

> >+    // Select and focus the first item in the list, if there is one.
> >+    if (gFilterTreeView.rowCount)
> >+    {
> >+        gFilterTreeView.selection.select(0);
> Offhand it seems to me that selectServer should do this.

I misunderstood Karsten. In fact focus and select can and should be looked at individually, focus should only be done when the window opens, select can happen any time.

> >+        gFilterTree.focus();
> I think we probably always want to do this on startup, as we currently have no
> focus in this window, although the server list is obviously a strong contender.

OK.
Attachment #396734 - Attachment is obsolete: true
Attachment #397900 - Flags: superreview?(neil)
Attachment #397900 - Flags: review?(mnyromyr)
Attachment #396734 - Flags: superreview?(neil)
Attachment #397900 - Flags: superreview?(neil) → superreview+
Attachment #397900 - Flags: review?(mnyromyr) → review+
Comment on attachment 397900 [details] [diff] [review]
patch v3

Carrying over r+ again based on IRC discussion.
Keywords: checkin-needed
Attachment #397900 - Flags: approval-seamonkey2.0?
Attachment #397900 - Flags: approval-seamonkey2.0? → approval-seamonkey2.0+
http://hg.mozilla.org/comm-central/rev/1d0fdd6efb2d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0
Adding fixed-seamonkey2.0 keyword so the queries for approved but not fixed bugs don't catch that bug.
You need to log in before you can comment on or make changes to this bug.