50881 - Clicking link in message does not bring Navigator window to the front

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect, P3)

Description

[hirata masakazu]

M18-2000083008

Comment 1

[lchiang]

Do you currently have a browser window open?  I think this is mscott's.  

Comment 2

[Zach Lipton [:zach]]

Confirming in 20000991520 OS9

Comment 3

[hirata masakazu]

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

Comment 4

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

I see this on the trunk mac builds, but not on linux.

probably a bug for danm, and not mscott.

to reproduce this, open browser window and mail window, and have mail window in
front.  view a message with a html link, click on the link.  the page loads in
the browser window, but the browser window isn't brought to front.

Comment 5

[Scott MacGregor]

this is a dup of an old mac bug. i'll try to find it.

Comment 6

[Koike Kazuhiko]

I see this on Linux build.

Comment 7

[sairuh (rarely reading bugmail)]

still seeing this [2001.03.12.05, linux moz]. nominating.

danm, should this be yours?

Comment 8

[Dan M]

What, is this a case where we actually have an opportunity to focus a window, and 
don't? Um sure, it might be my bug more than somebody else's. I hope Scott can 
find a duplicate bug, but I'll take it in the meantime. In doing so, I'm 
unnominating it from mozilla 0.9, since there's no chance I'll get to it by then 
and it's not "feature complete" work, anyway.

Comment 9

[Dan M]

.

Comment 10

[Koike Kazuhiko]

WFM with Linux build.

Comment 11

[hirata masakazu]

It is WFM in 2001033004trunk for Mac.
Resolving as such since it has been reported to be WFM in Linux build. 

Comment 12

[laurel]

Worksforme with apr4 build, mac OS 9.0, win98. Can't test on linux right now,
assume OK from last comments.

Comment 13

[Koike Kazuhiko]

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

Comment 14

[Koike Kazuhiko]

From bug 76704:

> ------- Additional Comments From [e-mail] 2001-04-19 12:08 -------
>
> It works for me usually first time in a session, but won't work after that.

Comment 15

[hirata masakazu]

adding regression KW.
It was working in 0417 build but not in 0419 build.

Comment 16

[laurel]

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

Comment 17

[Koike Kazuhiko]

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

Comment 18

[scottputterman]

This makes mail pretty hard to use.  It was working until recently on Win32 and
just broke.  I'm changing the severity to major and would like to ask that the
priority be raised and that the target milestone can be brought in (figuring out
what happened and getting it into 0.9 would be even better).

Comment 19

[laurel]

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

Comment 20

[Chris McAfee]

Good God I hate this bug.  Please PLEASE fix this.

Comment 21

[Dan M]

An impressive number of people have dropped by and made personal requests to get 
this bug fixed. Though none of them have come bearing doughnuts, it looks like 
this is my chance to reduce the pain in the world.

The regression was caused in rev 1.267 of nsDocShell.cpp. That change taught 
nsDocShell::SetFocus to set the focus on the next contained frame; before that 
change the method just set the focus to the docshell's window. In this case there 
is no subframe wanting focus, and SetFocus does nothing. This patch fixes the bug 
by reinstating the original "focus the whole docshell" clause in the "no 
subframe" case.

Index: nsDocShell.cpp
===================================================================
RCS file: /cvsroot/mozilla/docshell/base/nsDocShell.cpp,v
retrieving revision 1.277
diff -u -r1.277 nsDocShell.cpp
--- nsDocShell.cpp	2001/04/25 02:04:31	1.277
+++ nsDocShell.cpp	2001/04/27 02:13:32
@@ -2025,6 +2025,14 @@
     nsIFrame* focusFrame = nsnull;
     presShell->GetPrimaryFrameFor(focusContent, &focusFrame);
     esm->ChangeFocus(focusContent, focusFrame, PR_TRUE);
+  } else {
+    nsCOMPtr<nsIScriptGlobalObject> sgo;
+    document->GetScriptGlobalObject(getter_AddRefs(sgo));
+    if (sgo) {
+      nsCOMPtr<nsIDOMWindowInternal> domwin(do_QueryInterface(sgo));
+      if (domwin)
+        domwin->Focus();
+    }
   }
 
   return NS_OK;

Comment 22

[Håkan Waara]

maybe attinasi wants to r=/sr= this

Comment 23

[Marc Attinasi]

I'd be happy to! [s]r=attinasi

Comment 24

[Christopher Blizzard (:blizzard)]

It's important to test to make sure that this doesn't accidentally cause windows
to raise themselves accidentally.  I get so much hate mail about that issue you
wouldn't believe it.

Also, if you want the window to actually raise itself on linux you have to use
nsIWidget::SetFocus(PR_TRUE) to get the window to raise.  There's a default
PR_FALSE arg on that function call now.

Comment 25

[Dan M]

  I'm calling nsIDOMWindowInternal::Focus, which eventually becomes an 
nsIWidget::SetFocus(PR_TRUE). And after some playing, I can't tell that the patch 
causes windows to come to the front any more often than before, except in the one 
case covered by this bug. Specifically, the patch preventing a text field's 
getting focus from activating the window is unaffected.
  The patch seems safe. Unwilling focus maven hyatt likes it. But I'd still like 
to get an OK from either bryner or saari, who know about these things.

Comment 26

[Christopher Blizzard (:blizzard)]

The code itself looks reasonable, no doubt there.  I would say check it in with
but expect that if it does cause problems you will hear about it. :)

Comment 27

[Chris McAfee]

This works great for me on Linux, thanks dan.  Can we check this in?

Comment 28

[Chris McAfee]

Attachment: patch version of danm's fix

Comment 29

[saari (gone)]

See, you can't change anything in this system without side effects...

Anyway, the patch looks reasonable, but please test the living daylights out of
it. Someone with the patch applied, come get me and I'll go through a set of
tests with you for the r=saari.

Comment 30

[Brian Ryner (not reading)]

r=bryner

Comment 31

[Dan M]

patch is in, but trunk only (not on the 0.9 branch).

Comment 32

[Chris McAfee]

verified on win2k comm build, linux mozilla build.

Comment 33

[Dan M]

Reopening. Waterson's fix for bug 82073, checked in this morning, regressed 
this.

Comment 34

[Dan M]

I'll attach a more robust version of the same fix. I kind of thought I should 
have done this in the first place, but went with the simpler one because it left 
Blame with a clear record. But that made a less robust fix, and it was just 
broken. This new version falls back to focusing the entire document in many more 
cases, rather than simply bailing.

Comment 35

[Dan M]

Attachment: "rewrite" of SetFocus; really it's the same but is more likely to hit the fallback

Comment 36

[scottputterman]

I don't have a respin build so I can't see this in action, but I think this is a
0.9.1 stopper. It sounds like you have the fix, so I hope it can get checked in.

Comment 37

[Brian Ryner (not reading)]

r=bryner for danm's last patch

Comment 38

[Dan M]

  'preciate the r= Brian. I wonder if you could do it again. Rod checked in a 
big change to that method yesterday, so the patch is now useless. Attaching 
another one. As before, it's basically just shoving things around; moving 
objects' declaration and creation closer to where they're used and preferring to 
fail over into focusing the whole document, rather than bailing.
  Also retargeting to 0.9.1 at putterman's request.

Comment 39

[Dan M]

Attachment: new "rewrite," like before, but post rods' recent changes

Comment 40

[Brian Ryner (not reading)]

r=bryner

Comment 41

[David Hyatt]

sr=hyatt

Comment 42

[Asa Dotzler [:asa]]

a= [e-mail]

Comment 43

[Dan M]

patch is in; bug goes down once again.

Comment 44

[hirata masakazu]

i have not seen this work in 20010528 and 0529 trunk builds. What happened?

Comment 45

[Asa Dotzler [:asa]]

I can verify thta this is fixed for mozilla win32 build 052904 on win2K.

Comment 46

[laurel]

This is working OK for me with may29 commercial build:
2001-05-29-04 win98
2001-05-29-08 linux rh6.2

Not OK on mac OS 9.0 with 2001-05-29-08. Will double-check on a few machines
before reopening for mac.

Comment 47

[laurel]

Reopening for mac...

Comment 48

[Dan M]

The Mac-specific problem was the same reason as bug 82413; another variant of bug 
82368. It's fixed too, now.

Comment 49

[laurel]

OK mac OS 9.0 using may30 commercial trunk build.