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

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, enhancement)

Description

[Jens Hatlak (:InvisibleSmiley)]

Attachment: proposed patch

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.

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

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();

Comment 2

[Karsten Düsterloh]

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.

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2

(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.

Comment 4

[Karsten Düsterloh]

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.

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2a

Comment 6

[[e-mail]]

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.

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v3

(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.

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 397900 [details] [diff] [review]
patch v3

Carrying over r+ again based on IRC discussion.

Comment 9

[Stefan [:stefanh]]

http://hg.mozilla.org/comm-central/rev/1d0fdd6efb2d

Comment 10

[Robert Kaiser]

Adding fixed-seamonkey2.0 keyword so the queries for approved but not fixed bugs don't catch that bug.