203386 - CTRL+TAB navigation in three-pane stops at QuickSearch

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, enhancement)

Description

[Mike Cowperthwaite]

Navigating around the 3-pane with Ctrl-Tab (or Shift-Ctrl-Tab) appears to 'move 
quickly from pane to pane.'  However, navigation with this key makes four 
different stops; I would expect three.

Actual results:  Starting from the folder tree, Ctrl-Tab moves to QuickSearch 
field, Message Headers list, Message Body, and back to Folder tree.

Expected results: QuickSearch would not have been a stop, as it is part of the 
headers pane.

Comment 1

[Mike Cowperthwaite]

Investigating this bug, I found it is quite simple to fix.  However, the 
original code was clearly intended to make a stop at the QuickSearch box -- 
internally, the window actually has four panes.

To me, the QuickSearch thing looks like a toolbar

I also noticed an additional symptom:

As you use Ctrl-Tab or Shift-Ctrl-Tab to move around the window, when the focus 
shifts into the message pane, the "focus ring" is not cleared from the Thread or 
Folder pane which the focus just left.

This is due to code that's setting the focus on the "MessagePaneFrame" -- which 
is |window.content| -- rather than on the pane itself.  Even tho there is some 
code in place in FocusRingUpdate_Mail() that looks like it's supposed to cover 
for that, it fails, and I don't know why.  I've got a quick hack that addresses 
this nicely.  I can think of a slightly smoother, if more involved, bit of code 
that accomplishes the same thing; but I don't know exactly why it's failing in 
the first place.

Comment 2

[Mike Cowperthwaite]

Attachment: Patch diff -u against 1.7-0310

This patch changes the cycling order for Ctrl-Tab, Ctrl-Shift-Tab, F6, Ctrl-F6
to leave the search pane out of the possible targets; and it implements the
quick hack to address the focus-ring problem.

Comment 3

[[e-mail]]

I implemented Ctrl+Tab to the quick search widget because that's what was
requested - see bug 102711 and related bugs... as to the focus ring, I see it in
Classic but I don't see it in my own theme, perhaps it's a theme issue?

Comment 4

[[e-mail]]

Ah, I think it's a style system issue - it doesn't always update the style for
.focusring:focus > .tree-rows > .tree-bodybox though I have no idea why, I have
noticed that sometimes the :focus effect fails on menulists.

Comment 5

[Mike Cowperthwaite]

I think making the Search bar a stopping point in pane-cycling is just a 
not-well-thought-out requirement:
 - The search bar visually appears to be a toolbar, not a separate pane.
 - Unlike any of the major three panes, there is a hotkey to get you to the 
Search bar (Alt-S, or Alt-I for that matter).

However, I'll change this bug to an rfe.


Regarding the focus-ring issue:  I'm using the Pinball theme (which is based on 
Modern), for whatever that's worth; I see it in Modern too.  With all three 
panes open, just using Tab to move around does not display the problem, whether 
or not there's a message displayed.

See bug 227646; in the anomalous state described there, the focus ring is still 
in place on the Thread pane, and apparently the keyboard events are directed 
there as well, but the highlight on the list is rendered as if it did not have 
focus.  Maybe related, maybe distinct.

I've already looked for a separate bug on the focus-ring issue. (Maybe related 
to bug 205576?)  I'll open a separate one for it, later...

Comment 6

[[e-mail]]

The focus ring issue may well be related to bug 205576.

CC'ing mscott in case he feels that quicksearch can be removed from Ctrl+Tab/F6
sequences as it has Alt+S shortcut and regular Tab still works.

Comment 7

[Scott MacGregor]

"CC'ing mscott in case he feels that quicksearch can be removed from Ctrl+Tab/F6
sequences as it has Alt+S shortcut and regular Tab still works."

I'm fine with removing the quick search box from th ctrl-tab navigation.

I see the Views menu list is already not part of that list (but it is part of
the tab list) so I think it is just an oversite that the quick search bar is
part of the list.

Comment 8

[[e-mail]]

Comment on attachment 143730 [details] [diff] [review]
Patch   diff -u  against 1.7-0310

>+    // Reverse traversal: Message -> Thread -> Folder -> Message
>+    if ((focusedElement == threadTree || focusedElement == searchInput) && !IsFolderPaneCollapsed())
So... should we still be special-casing searchInput? Perhaps we need a new
approach that will do the right thing for all of the search bar controls.

>+    // Bug 203386: hack to get FocusRingUpdate_Mail() to correctly clear previous element's ring
>+    GetMessagePane().focus();       // first focus on a pane element
>+    GetMessagePaneFrame().focus();  // then focus on the main window's 'content' frame
I wonder if bryner can give me a few pointers here...

Comment 9

[Mike Cowperthwaite]

(In reply to comment #8)
> So... should we still be special-casing searchInput? Perhaps we need a new
> approach that will do the right thing for all of the search bar controls.

It's being special cased because WhichPaneHasFocus() indicates the (internal, 
structural, actual) pane.  If we don't check, then the wrong thing will be done 
when the focus is on the search bar.

It does work for all the search-bar controls, because WhichPaneHasFocus() always 
returns the same control for a given pane.

However, looking at the uses of that function -- at least, those within the same 
file -- I think it could be tweaked to return the same control for both 
search-bar and thread-pane, and skip the special-case logic elsewhere.

Comment 10

[[e-mail]]

Actually it might be better to tweak WhichPaneHasFocus() to return null except
for the main three panes, like it used to do before bug 102711. Also I don't
think WhichPaneHasFocus() is used in any other file.

Comment 11

[Mike Cowperthwaite]

(In reply to comment #10 [Neil])
> Actually it might be better to tweak WhichPaneHasFocus() to return null except
> for the main three panes, like it used to do before bug 102711. Also I don't
> think WhichPaneHasFocus() is used in any other file.

(Nine months later...) I tried this.  If WhichPaneHasFocus() returns null, 
FocusRingUpdate_Mail() breaks as follows: when the focus is on the message pane, 
and is shifted to the search input field, the message pane's focus ring is not 
erased.  (Why the same thing doesn't happen when the focus is on the thread pane 
or the folder pane, I don't know.)  This behavior is already broken, tho, if the 
focus is shifted to the MailView dropdown -- that case *already* returns null.

In TB, FocusRingUpdate_Mail() is still there, but I don't know if it's being 
called -- at any rate, the panes aren't being drawn with focus rings in TB 0.9. 
(Is that by design or a bug?  Scott?)  

This means that the focus on the message pane appears as no focus anywhere, 
unless some specific subcomponent of the message pane (selection in a header, 
link in the body) has a visible focus indicator.

Comment 12

[[e-mail]]

OK, so to fix that we'd need to tweak FocusRingUpdate_Mail() to accept nulls
from WhichPaneHasFocus(); removing the if (!currentFocusedElement) return; and
adding an if (currentFocusedElement) before the setAttribute should work.

Comment 13

[Mike Cowperthwaite]

Attachment: diff -u against Moz 1.8a5-1122

Updated patch: Ctrl-Tab navigation is limited to the three main panes, and two
minor focus-ring issues are fixed.

WhichPaneHasFocus() returns null if focus is not on one of the three main
pains. 
SwitchPaneFocus() handles the null by overriding 'focusedElement', simplifying
the actual pane-switching logic a bit.
FocusRingUpdate_Mail() handles the null case correctly, fixing the broken
behavior mentioned in comment 11.
SetFocusMessagePane() still has the hack to fix the focus-ring problem in
comment 1.

This patch works in 1.8a5; the same patch works in Thunderbird, but as
mentioned, the focus-ring is not displayed in TB, so I don't know whether those
two minor fixes actually are useful there.

Comment 14

[[e-mail]]

Comment on attachment 167337 [details] [diff] [review]
diff -u against Moz 1.8a5-1122

>+  // Altho internally this is actually a four-pane window, it is presented as
Nit: I don't like "abbreviations" in comments

>+  if (focusedElement == null)       // focus not on one of the main three panes?
>+    focusedElement = threadTree;    // treat as if on thread tree
Out of interest, how do these lines affect navigation?

>+    if ((focusedElement == threadTree) && !IsFolderPaneCollapsed())
Nit: don't need the extra ()s as per the existing code.

>+    // Bug 203386: hack to get FocusRingUpdate_Mail() to correctly clear previous element's ring
>+    GetMessagePane().focus();       // first focus on a pane element
>+    GetMessagePaneFrame().focus();  // then focus on the main window's 'content' frame
This must be a bug in the ESM, although that's rather hairy code :-/

Comment 15

[Mike Cowperthwaite]

Attachment: Same patch, with parentheses fixed

> Out of interest, how do these lines affect navigation?

If you don't override the searchbar case explicitly as 'threadpane', the focus
will shift from searchbar *to* threadpane, for both the forward and reverse
cases.	And because the other two panes can be hidden, it makes the most sense
for the 'threadpane' to remain the 'default' clause after the two explicit 'IF'
checks.

Comment 16

[[e-mail]]

Comment on attachment 167349 [details] [diff] [review]
Same patch, with parentheses fixed

r=me but investigations I've made show that your comment in SetFocusMessagePane
is wrong, so it will need to be changed to "// XXX hack to clear the focus on
the previous element" before it gets checked in. (It doesn't appear to have
been the ESM's fault either, but I found it to difficult to debug.)

Comment 17

[Mike Cowperthwaite]

Comment on attachment 167349 [details] [diff] [review]
Same patch, with parentheses fixed

Scott, as noted in comment 13, this same code change works for TB, but I'm only
looking for suite approval until I know whether the lack of a focus ring in TB
is intentional.

Comment 18

[Scott MacGregor]

Comment on attachment 167349 [details] [diff] [review]
Same patch, with parentheses fixed

yeah the focus ring removal is intentional in thunderbird mike.

Comment 19

[Mike Cowperthwaite]

Neil, would you do the honors of checking this in (with the comment fix, and the 
traditional spelling if you like)?

Comment 20

[[e-mail]]

Fix checked in with traditional spelling :-P