581932 - Keyboard shortcuts are disabled after deleting a message and next is a thread when message pane has focus

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Onno Ekker [:nONoNonO UTC+1]]

When you remove a message when the message pane has focus and the next message is a part of a collapsed thread, the thread doesn't expand and you can't move to the first message of the thread with your keyboard.

Steps to reproduce:
1. Mail yourself a first test message, a second test message and reply to the second message;
2. Go to menu View | Sort by | Threaded
3. Go to your inbox and select the first message and press F6 until the message pane gets focus;
4. Press delete to delete first message;
5. Press n to go to next message.

Expected results:
Second message should display.

Actual results:
Nothing happens and all (?) keyboard shortcuts (n, p, f, delete) are disabled.

Comment 1

[Ludovic Hirlimann [:Usul]]

I suppose the Error console is empty ?

This looks a bit similar to 558303

Comment 2

[Ludovic Hirlimann [:Usul]]

Onno could you track the regression window ?

Comment 3

[Onno Ekker [:nONoNonO UTC+1]]

Error Console is empty...
In 3.1 this is already broken. In 3.0 keyboard shortcuts are broken too, but F6 still works, so after pressing (Shift-)F6 things work again.

Comment 4

[Wayne Mery (:wsmwk)]

your steps WFM Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2.9pre) Gecko/20100802 Lanikai/3.1.2pre

If you did not think this bug is a regression, I'd seriously say this is a dup - although bug 558303 might not be best, because someone might think N shouldn't work in that situation anyway (summary view). Might be work look at thses bugs - http://bit.ly/bfqg5n ?

xref bug 278386

Comment 5

[Onno Ekker [:nONoNonO UTC+1]]

The thread needs to be collapsed too, so you get stuck at the summary view of the collapsed thread with focus lost somewhere. And I have mail.operate_on_msgs_in_collapsed_threads set to its default true.

Comment 6

[Onno Ekker [:nONoNonO UTC+1]]

Try with f key in step 5 to go to the following message. After pressing f I get stuck with no keys working

Comment 7

[Onno Ekker [:nONoNonO UTC+1]]

Might be a dup of bug 567715

Comment 10

[John David Galt]

In my experience it doesn't matter if I just deleted a message: the 'n' stops working whenever a thread summary is displayed instead of a message.

I wish whoever is making all these useless, gratuitous UI changes such as the thread summary would stop.  Or at least make the default for every new version be to behave exactly like the old one unless explicitly requested by the user.

Comment 11

[esigra]

I can reproduce this bug with Thunderbird 3.1.7 on Gentoo Linux. It does not respond to any keyboard input whatsoever; not shortcuts (not even Shift+F6) and no accelerator keys (like Alt+H for the Help menu).

Comment 13

[Jim Porter (:squib)]

Attachment: FIx this and test it

Here's a pretty straightforward fix with a test. Note that I handle focusing the single message pane differently from the multi-message pane due to <http://mxr.mozilla.org/comm-central/source/mail/base/content/mail3PaneWindowCommands.js#1197> (for folks from the future: function SetFocusMessagePane).

In retrospect, I think I've been bitten by this bug a bunch, which usually resulted in me flailing in vain at my keyboard. :)

Comment 17

[Jim Porter (:squib)]

Attachment: A better fix

I don't know what I was thinking with the earlier patch; I must have just had a case of the dumbs when I wrote it. Here's a functionally identical patch with cleaner code.

Comment 18

[Jim Porter (:squib)]

Attachment: Once more, but this time with feeling

Ack, sorry. I forgot that == binds tighter than the ternary operator. A dash of parentheses fix everything!

Comment 19

[Jim Porter (:squib)]

Last comment for a while: WhichPaneHasFocus() isn't always defined by the time singleMessageDisplay is set, which causes problems. I'm thinking we should move WhichPaneHasFocus somewhere else so that it's defined in time (or implement a similar function in messageDisplay.js). Any ideas on how best to handle this, asuth?

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 554583 [details] [diff] [review]
Once more, but this time with feeling

Yes, I believe there are a number of subtle focus bugs that likely drive many people crazy; thankfully you are a master focus-bug hunter!

Yes, it makes sense to move WhichPaneHasFocus / other things from the global soup onto the class.

mailTabs.js has focus persisting logic in saveFocus/restoreFocus that will get tricked by your transition-only logic not firing when not "this._active".

I think my natural inclination to address this would be to have our persisted state be incapable of expressing an illegal focus state.  Which is to say, when either the "messagepane" or "multimessage" is focused, we might store the focus state as MESSAGE_AREA, and then focus the appropriate thing based on the _singleMessageDisplay flag.  To avoid making things more brittle, when persisting the state, we could check for both in all cases (rather than just the one we expect).

I expect this might entail pushing saveFocus/restoreFocus into FolderDisplayWidget.  saveFocus already has a copy of WhichPaneHasFocus, which maybe will fit into your scheme :)

If you end up simplifying the focus maintenance logic, I don't think we'll need a tab changing test, but if it stays like it is with the patch (transition case and separate persistent cases) I think we need an extra test for that.

Thanks!

Comment 21

[Jim Porter (:squib)]

Attachment: Reorganize things and add tests

This patch reorganizes everything and adds focusedPane as a member of the FolderDisplayWidget. I also added tests for 1) swapping between tabs with various focus states and 2) to check that focus is updated properly when deleting a message and going from single- to multi-message and back.

Maybe it'd be worth doing something like adding an onfocus handler for the messagepanebox to automatically update focus to the appropriate subnode (messagepane or multimessage), since that would simplify some of the special-casing, like SetFocusMessagePane(). But then again, maybe not.

Comment 22

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 555669 [details] [diff] [review]
Reorganize things and add tests

I expect the main problem is ye 'ole multiplexed implementation where we keep around DOM nodes for everything all the time, even if we're not using it.  Probably the simplest fix is to move to the everybody-in-their-own-tab and then our problem is just performance from creating new DOM nodes :)

If I read things correctly, rather than using a magic JS object like I was proposing, you are now using the common ancestor of the two dudes who can be focused as the persisted state and migrating the focus down to the specific/appropriate child as needed.  This is good and makes sense, but it would probably help to add some mention of the strategy to the "focusedPane" documentation block to help clarify what is going on.

The alternative you mention could work.  Assuming this patch lands without explosions, I probably still advocate the transition to a non-multiplexed implementation (which will have its own horrors :).

Comment 23

[Jim Porter (:squib)]

(In reply to Andrew Sutherland (:asuth) from comment #22)
> If I read things correctly, rather than using a magic JS object like I was
> proposing, you are now using the common ancestor of the two dudes who can be
> focused as the persisted state and migrating the focus down to the
> specific/appropriate child as needed.  This is good and makes sense, but it
> would probably help to add some mention of the strategy to the "focusedPane"
> documentation block to help clarify what is going on.

I added a note on this. Technically, we always used the common ancestor, but when I changed things to support F6'ing into the message summary, I superseded that. The way it is now is probably closer to the Right Way than it was before.

> 
> The alternative you mention could work.  Assuming this patch lands without
> explosions, I probably still advocate the transition to a non-multiplexed
> implementation (which will have its own horrors :).

Definitely. This would help with a bunch of other bugs too, like the infamous "tabs don't maintain scroll position" bug.

Comment 24

[Jim Porter (:squib)]

Checked in: http://hg.mozilla.org/comm-central/rev/2b354adda711