201242 - When minimize and then restore, two carets appear in a mail compose window

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[rubydoo123]

I am using a trunk build from 20030407 on win 2K
1. open a new mail compose window, caret is in the To: field, set focus in the
compose window
2. start entering some text, then minimize the window
3. restore the window

Expected: I expected focus to remain within the body of the message.
Actual: there was a cursor I-beam in the body of the message where I had stopped
typing. There was also an I-beam in the two field. At this point when I started
typing, focus was in the To: field instead of in the body of the message.

Comment 1

[leon.zhang]

neil, can you pay attention to this bug? :)
btw, this bug may be related to bug 195798.

Comment 2

[[e-mail]]

Just doing some quick debugging, after minimizing the compose window its focus
controller believes that both the To: and the body have focus (there is a
separate body focus flag), which is obviously wrong :-(

BTW, using Mozilla's Window menu to restore the compose window works (because it
sees that the body should have focus and restores it correctly).

Comment 3

[[e-mail]]

It's worse than that, I can duplicate the problem in the browser:
1. Focus the location bar
2. Click in the body (but NOT in an input or textarea)
3. Minimize and restore the browser

Actual results: focus returns to the location bar

Expected results: focus returns to the previously focused frame

ere, one of yours?

Comment 4

[Ere Maijala (slow)]

As just a shot in the dark, this could be related to bug 201996. 

Comment 5

[Ere Maijala (slow)]

My shot was a miss. Anyway, I think I found the problem in focus controller.
Patch coming up soon.

Comment 6

[Ere Maijala (slow)]

Attachment: Proposed patch

When focusing a window only (no element), explicitly set the focused element to
null to keep the previous element and previous window synchronized.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 121439 [details] [diff] [review]
Proposed patch

Looks good to me (Hyvalta nayttaa) sr=jst, but I think bryner should have a
look at this one before it's checked in.

Comment 8

[[e-mail]]

Comment on attachment 121439 [details] [diff] [review]
Proposed patch

>+        SetFocusedElement(nsnull);
>         SetFocusedWindow(domWindow);
>         if (mCurrentElement) {
This just looks wrong... but it's probably the right idea, there's an
mCurrentElement = later that might need to be SetFocusedElement instead.

Comment 9

[Brian Ryner (not reading)]

Comment on attachment 121439 [details] [diff] [review]
Proposed patch

Hm, no, I don't think this is quite right.  You're forcing mCurrentElement to
null, which sort of makes the following if (mCurrentElement) moot.  Can you
describe in more detail what's happening to cause this?

Comment 10

[Ere Maijala (slow)]

Attachment: Patch v2

Thanks for catching that. Here's a fixed patch. Here is the deal: if we don't
have a suitable element to focus, we must use SetFocusedElement() so that
mPreviousElement is updated. Otherwise we'll end up in a situation where
mPreviousElement points to an element that isn't a child of mPreviousWindow and
RewindFocusState() messes it up.

Comment 11

[Brian Ryner (not reading)]

Attachment: alternate patch

I think this one is more correct... it keeps mPreviousELement from becoming
incorrect in the first place.

Comment 12

[Ere Maijala (slow)]

The alternate patch doesn't work for me. The focus is always lost if I minimize
and restore a window where the focus was on an element. 

Comment 13

[Brian Ryner (not reading)]

Ok, after debugging some more, I think ere's approach is probably the right one.
 I'd prefer if we just set mPreviousElement directly though, instead of calling
SetFocusedElement(), so that we don't call UpdateCommands() twice.

Comment 14

[Brian Ryner (not reading)]

Attachment: how about this one

ere, does this look ok?

Comment 15

[Ere Maijala (slow)]

Looks better and works better. My only concern is that this sets
mPreviousElement to nsnull instead of cycling mCurrentElement to it. Apparently
at the moment we don't have a situation where we would need mPreviousElement
right after Focus(), so this doesn't seem to cause any harm, but wouldn't it be
ok to call SetFocusedElement(nsnull) and just remove UpdateCommands() from Focus()?

Comment 16

[Ere Maijala (slow)]

Attachment: Bryner's patch with an additional check

I found out that we just have to guard against setting mPreviousElement to null
in SetFocusedElement(). For example when minimizing the window by clicking its
button on the taskbar. This causes Blur() when focus is suppressed, which
causes us not to call SetFocusedElement() there. With these additional checks
everything I tried worked fine. And these checks are the same as in
SetFocusedWindow(). Bryner, what do you think?

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 121820 [details] [diff] [review]
Bryner's patch with an additional check

sr=jst

Comment 18

[Ere Maijala (slow)]

Comment on attachment 121820 [details] [diff] [review]
Bryner's patch with an additional check

Seeking approval. This is a fairly simple patch to fix a fairly high-visibility
focus problem at least in compose and navigator windows.

Comment 19

[(not reading, please use [e-mail] instead)]

Comment on attachment 121820 [details] [diff] [review]
Bryner's patch with an additional check

a=sspitzer

Comment 20

[Ere Maijala (slow)]

Fix checked in to trunk.

Comment 21

[(not reading, please use [e-mail] instead)]

*** Bug 147077 has been marked as a duplicate of this bug. ***

Comment 22

[sairuh (rarely reading bugmail)]

related bugs for OS X, which are still problems with bryner's test build:

bug 203827: have 0 caret with every other new msg window
bug 124929: restoring a minimized window loses focus

Comment 23

[sairuh (rarely reading bugmail)]

with bug 124929 fixed, this looks fixed on OS X now.