Closed Bug 501308 Opened 13 years ago Closed 13 years ago

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

Categories

(Thunderbird :: Mail Window Front End, defect)

x86
All
defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED
Thunderbird 3.0b4

People

(Reporter: MarcoZ, Assigned: rain1)

References

(Blocks 1 open bug)

Details

(Keywords: access, regression)

Attachments

(2 files, 1 obsolete file)

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.
Flags: blocking-thunderbird3?
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).
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.
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".
Keywords: relnote
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?
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.
(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.
I see this on Mac OS X also.
OS: Windows XP → All
Assignee: nobody → sid.bugzilla
Status: NEW → ASSIGNED
Attached patch patch, v1 (obsolete) — Splinter Review
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?
Attachment #389952 - Flags: ui-review?(clarkbw)
Attachment #389952 - Flags: review?(bugmail)
Attached patch patch, v1.01Splinter Review
oops, missed a few foo.focus() -> focus_foo() conversions.
Attachment #389952 - Attachment is obsolete: true
Attachment #389954 - Flags: ui-review?(clarkbw)
Attachment #389954 - Flags: review?(bugmail)
Attachment #389952 - Flags: ui-review?(clarkbw)
Attachment #389952 - Flags: review?(bugmail)
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?
Attachment #389954 - Flags: review?(bugmail) → review+
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!
Attachment #389954 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 389954 [details] [diff] [review]
patch, v1.01

sorry for the delay, looks good!
http://hg.mozilla.org/comm-central/rev/945b29adc6c5
(forgot to remove an old comment, now redundant: <http://hg.mozilla.org/comm-central/rev/98b20d100186>)
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b4
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.
Flags: blocking-thunderbird3?
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!
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.