501308 - Pressing Enter on a message no longer sets focus to the message body

Status: VERIFIED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Marco Zehe (:MarcoZ)]

This is a regression from the recent "open message in tab" landing. When pressing Enter on a message from the messages list, when the message opened in a new window, the focus was set to the message body, and the screen reader could immediately start reading. Now, keyboard focus goes into nowhereland, and one has to press at least 1 other key (F6, Shift+Tab, the behavior doesn't seem to be consistent) to get to the message body.

This will be viewed by end users as a regression.

Comment 1

[Phil Ringnalda (:philor)]

Since up-arrow loads the previous message in the threadpane in the standalone window, I'd say keyboard focus isn't in nowhereland, it's in a window that's in the background (equally strange and terrible, but a little more understandable than nowhere).

Comment 2

[Phil Ringnalda (:philor)]

Err, in a part of the window which isn't visible: I do know the difference between a tab that's the same XUL with most of it hidden and a separate window that's different XUL, really I do.

Comment 3

[Marco Zehe (:MarcoZ)]

Requesting that this be relnoted for b3, with a recommendation to set Tools/Options/Advanced/Reading & Display, "Open messages in" to "A new message window" or "an existing message window".

Comment 4

[Marco Zehe (:MarcoZ)]

Attachment: Patch for feedback

This patch does not actually fix the problem, but I'm putting it up here to ask whether this is the right spot to try and fix the problem. The behavior doesn't change with this patch.

Moreover, when closing the tab with Ctrl+W, focus does not get restored. But this also seems to happen without this patch.

Any feedback anyone?

Comment 5

[David :Bienvenu]

that piece of code is called before the message is loaded, so I wonder if the reason it's not working is that focus is changed somewhere after this.

Comment 6

[Rain [:rain1] (inactive)]

(In reply to comment #5)
> that piece of code is called before the message is loaded, so I wonder if the
> reason it's not working is that focus is changed somewhere after this.

Actually, that line isn't called at all... mail3PaneWindow is guaranteed to be null at that point when Enter's pressed, since we always pass in a tabmail.

Comment 7

[Rafael Ebron (:rebron)]

I see this on Mac OS X also.

Comment 8

[Rain [:rain1] (inactive)]

Attachment: patch, v1

With this patch, we'll persist focus across tab switches. We don't do it across restarts yet though :)

I've modified the existing unit tests to cover focus and its persistence.

Bryan, is this the right thing to do here?

Comment 9

[Rain [:rain1] (inactive)]

Attachment: patch, v1.01

oops, missed a few foo.focus() -> focus_foo() conversions.

Comment 10

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

Comment on attachment 389954 [details] [diff] [review]
patch, v1.01

Great work.  In particular the extensive and well-factored additions to the
mozmill tests are a fantastic addition.


on file: mail/base/content/mailWindowOverlay.js line 2023
>   /**
>    * Save off the tab's currently focused element or window.
>    */
>   saveFocus: function mailTabType_saveFocus(aTab) {

Additional comments on or in this method would be very helpful.

For example, why do we only persist the 'window' if we're dealing with the
message pane?  Presumably the rationale is that because these are basically
re-built every time you change tabs that it is impossible to have a more
precise persistence without doing more legwork to maintain the focus of
specific things in the message header.

For the other case, I'm not sure there is the ability for anything other than
the trees to actually have the focus, but I'm not sure why we do the climbing
logic (which sets things to null if it's neither) rather than just saving off
the element?  I presume there's a reason...


on file: mail/base/content/mailWindowOverlay.js line 2065
>           document.commandDispatcher.focusedWindow = aTab._focusedWindow;

This action is not entirely obvious here.  Can you add a comment that explains
the choice and any deeper rationale, or otherwise add a "copied this from
elsewhere, seems to work" note so if the choice turns out to be problematic we
know it's not for a deep magic reason?

Comment 11

[Marco Zehe (:MarcoZ)]

I just ran with this patch for a while and have the following feedback:

1. It fixes the bug!
2. It makes the focus behave very consistently across tabs.
3. It partially fixes bug 468497, in that it fires focus events when different elements are focused within different tabs (for example the folderTree in one, the threadTree in another. Only if the threadTree is focused in both tabs does the accessibility view not update yet.

Good work!

Comment 12

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 389954 [details] [diff] [review]
patch, v1.01

sorry for the delay, looks good!

Comment 13

[Rain [:rain1] (inactive)]

http://hg.mozilla.org/comm-central/rev/945b29adc6c5
(forgot to remove an old comment, now redundant: <http://hg.mozilla.org/comm-central/rev/98b20d100186>)

Comment 14

[Rain [:rain1] (inactive)]

Looks like this patch caused test failures on the trunk MozMill test box. I suspect it's because the focus code is different on trunk than on 1.9.1.

Is #ifdefing code acceptable here? mailWindowOverlay.js is currently free of the preprocessor curse.

Comment 15

[James Teh [:Jamie]]

Verified fixed in: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3pre) Gecko/20090730 Shredder/3.0b4pre
Thanks!