109081 - [windows only] some times the cached compose window pops up, with previous subject

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect, P1)

Description

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

[windows only] some times the cached compose window pops up, with previous 
subject

I've got a screen shot.  in the screen shot, focus is in the editor (although 
you can't tell).

see http://bugzilla.mozilla.org/show_bug.cgi?id=104989#c12

here's what's going on:

somehow, there's a focus change, and on win32 (double check this) we'll show a 
hidden window on focus change.  

I think the way to fix this problem is to move the focus to the subject area, 
before hiding.

on a related note, why aren't we clearing the subject on hiding?

Comment 1

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

Attachment: screen shot, caret is blinking in editor

to reproduce this, add user_pref("mail.compose.max_recycled_windows", 1); to
your prefs.js

it doesn't happen often, but both putterman and I have seen it.

Comment 2

[scottputterman]

if you could find a fix for this, I wouldn't be against getting this into 0.9.6

Comment 3

[jay garcia]

Win98SE 200110803

Font in the cached compose window looks rather strange as it looks sorta
compressed as if typing in TimesRoman in an HTML compose window. The received
message looks normal tho

Comment 4

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

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

Comment 5

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

ducarroz, if you haven't found a way to fix this (by setting the focus to
subject, or to the outer window), we might want to look into extending the xul
window, or base window, interface to have a boolean attribute.

we'd set this boolean to true before hiding, and set it to false after showing.

then, in the window code that shows a window when focus happens, we'd check the
boolean, and if true, skip that code.

it's a hack.  you should talk to who ever owns the window code (danm?)

Comment 6

[Jean-Francois Ducarroz]

The problem is that I cannot write a hack if I cannot reproduce it! Seth, Scott
are you using the turbo mode?

Comment 7

[scottputterman]

No, I don't use turbo mode.  I've never seen this happen on my machine at home
but it happens to me once a day at work.  I don't know this for sure, but I
think it happens when I lock my computer so that I have to enter a password to
get back in.  But I'm not sure about that.

Comment 8

[Scott MacGregor]

No I'm not using turbo mode. It just happens randomly for me if I use the
compose window a lot. 

Comment 9

[Jean-Francois Ducarroz]

good news, I finally get this problem on My PC using a Mozilla debug build
without turbo and my PC wasn't lock. I looked at the console messages but did
not find any clue why this window popup about 1 hour later!

The bad news is that my build include the patch for bug 110775 which I was
hopping would solve this problem too.

I think I would need to add a onfocus event handler to the window and detect
unwanted focus change and eventually shutdown for real the window!

Comment 10

[Jean-Francois Ducarroz]

I was able to reproduce a similary problem when running Mozilla under VC++
dubugger. All I have to do is to set a break point in the code right after we
hide the window. That cause the debugger to set the focus on the compose window
which has been hidden. As we always set the visibility to 1 when a window get
focus, the compose window popup back. I am not 100% sure it the same case than
the one reported in this bug but it's the only one I found to reproduce the problem.

After looking at various part of the code from the time we receive a WM_ACTIVATE
event to the time the compose window get the onFocus handler, I found only two
ways to solve this problem:

1)Once I detect the window get reopen (by getting the focus) when it should not,
I close it. The problem, I cannot avoid the window to show up for a fraction of
a second!

2) As this problem occurs rarely, I can just force the window to execute the
reopen code which make it be like a brand new one and not a bogus one you cannot
do much with it.

I will attach a patch which implement both solutions but only the first one is
activated.

Comment 11

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v1

Comment 12

[scottputterman]

This happens to me at least once a day so I think we should avoid solution #2 if
possible.

Comment 13

[varada]

Comment on attachment 59418 [details] [diff] [review]
Proposed fix, v1

r=varada

Comment 14

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

I have reservations about this patch.

why can't we ignore the focus event when we're hidden?

Comment 15

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

I think the problem is the cached window still has focus, when we hide it.
if we switched focus back another window, before we hid it, I claim this 
wouldn't happen.

nsWindow::Show(nsWindow * const 0x0496bb14, int 1) line 1435
DocumentViewerImpl::Show(DocumentViewerImpl * const 0x049a3f30) line 1562
nsDocShell::SetVisibility(nsDocShell * const 0x03f0d5a4, int 1) line 2650
nsXULWindow::SetVisibility(nsXULWindow * const 0x03f08224, int 1) line 581
nsChromeTreeOwner::SetVisibility(nsChromeTreeOwner * const 0x03f11e14, int 1) 
line 293
GlobalWindowImpl::Focus(GlobalWindowImpl * const 0x03f11614) line 1849
nsWebShellWindow::HandleEvent(nsGUIEvent * 0x0012f700) line 579
nsWindow::DispatchEvent(nsWindow * const 0x03f0f6c4, nsGUIEvent * 0x0012f700, 
nsEventStatus & nsEventStatus_eIgnore) line 845 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f700) line 866
nsWindow::DispatchFocus(unsigned int 105, int 1) line 4613 + 15 bytes
nsWindow::ProcessMessage(unsigned int 7, unsigned int 0, long 0, long * 
0x0012fafc) line 3497 + 23 bytes
nsWindow::WindowProc(HWND__ * 0x000e0692, unsigned int 7, unsigned int 0, long 
0) line 1113 + 27 bytes
USER32! 77e13eb0()
USER32! 77e1591b()
USER32! 77e1595d()
NTDLL! 77f9fb83()
USER32! 77e169a7()
USER32! 77e13eb0()
USER32! 77e16469()
USER32! 77e1a6f8()
nsWindow::WindowProc(HWND__ * 0x000e0692, unsigned int 6, unsigned int 1, long 
0) line 1120 + 31 bytes
USER32! 77e13eb0()
USER32! 77e1591b()
USER32! 77e1595d()
NTDLL! 77f9fb83()
USER32! 77e191df()
nsAppShell::Run(nsAppShell * const 0x005470e0) line 112 + 24 bytes
nsAppShellService::Run(nsAppShellService * const 0x00547480) line 303
main1(int 2, char * * 0x004816e0, nsISupports * 0x00000000) line 1302 + 32 bytes
main(int 2, char * * 0x004816e0) line 1632 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e87903

Comment 16

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

in nsWebShellWindow::HandleEvent() we do this:

nsCOMPtr<nsIDOMWindowInternal> focusedWindow;
focusController->GetFocusedWindow(getter_AddRefs(focusedWindow));
if (focusedWindow) {
  focusController->SetSuppressFocus(PR_TRUE, "Activation Suppression");
  domWindow->Focus();
}

can we make the focus controller return null, if the window focused window is 
our hidden compose window?

adding saari and danm.  maybe they'll have other idea on how we can avoid this 
problem.

are there other windows in mozilla that are hidden (or off screen?) that 
already do this?

Comment 17

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

Attachment: hack idea

Comment 18

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

well, that's not working.

another possible idea is to put focus somewhere else (I'm wondering if it is 
because the text areas still have focus?) or bring another window to the 
front / focus, before hiding that window.

Comment 19

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

I've got a way to help reproduce this, without the debugger.

I'm working on why that causes the window to show up.

Index: compose/resources/content/MsgComposeCommands.js
===================================================================
RCS file: /cvsroot/mozilla/mailnews/compose/resources/content/MsgComposeCommands
.js,v
retrieving revision 1.232
diff -u -w -r1.232 MsgComposeCommands.js
--- MsgComposeCommands.js       2001/11/21 23:43:26     1.232
+++ MsgComposeCommands.js       2001/11/30 03:30:28
@@ -176,6 +176,9 @@
     SetContentAndBodyAsUnmodified();
     disableEditableFields()
     ReleaseGlobalVariables();
+
+    var foo = document.getElementById("toolbar_button_box");
+    foo.focus();
        },

        onReopen: function(params) {

Comment 20

[Dan M]

  Drive-by comment, without having immersed myself in the issue: focusing a
window has always made it visible, and I think there is code that relies on this
behaviour. On the other hand, and this may be heresy, it may not be strictly
necessary for focus to behave like that. It has to raise the window to the top,
sure, but I don't think that the subject of invisible windows is really covered
under the topic of correct script behaviour.
  So there might be something we could do there, though I suspect it would be
hard because it would break unexpected things. Better somehow keep the invisible
cache window from gaining focus.
  And yes there are other hidden windows that don't have this problem.
EvilHiddenWindow, for instance. It avoids becoming visible by staying the heck
out of the focus fray. Easier for it, having never been visible and containing
very little script.
  There is something that keeps a memory of the previously focused window and
could restore that somewhere, isn't there? saari? Or maybe there's some script
somewhere that wants a composer window for whatever reason, looks in the window
list, realizes we already have one and pings it?

Comment 21

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

I tried out the following hack, and it seemed to work, as it might provide a 
way to solve this:

in nsMsgComposeService.cpp 

ShowCachedComposeWindow(), I set the title of the window to "HACK" if we were 
hiding the window.

in nsEventStateManager.cpp

if(focusedWindow) {
  if (title == "HACK")
   break;
  focusedWindow->Focus();

nsWebShellWindow.cpp 

if (focusedWindow) {
  if (title == "HACK")
    break;
  focusController->SetSuppressFocus(PR_TRUE, "Activation Suppression");
  domWindow->Focus();

nsWindow.cpp

NS_METHOD nsWindow::SetFocus(PRBool aRaise)
{
    if (!mIsVisible)
      return NS_OK;

the "HACK" as title trick was because I didn't want to modify 
nsIDOMWindowInternal.

the SetFocus() was the heresy that danm mentioned:  if not visible, don't show 
on focus.
 
that was enough to prevent the window from coming up, even when I force focus 
(see http://bugzilla.mozilla.org/show_bug.cgi?id=109081#c19)

but that might not be the right approach, but maybe it will get us to the right 
approach.

I'm hoping there is some proper interface that that we can add a boolean 
for "hidden on purpose" and when "hidden on purpose" we drop the focus event.

saari / damn, any other ideas?

Comment 22

[Jean-Francois Ducarroz]

I wonder if Disabling the window would help?

Also I found that child windows (modal dialogs open on top of the compose
window) like the send progress or the IMAP logging dialog still alive despite
they have been close. They are invisible but you can see them hanging around
using the VC++ tool Spy++. I wonder why and if that could some how trigger a
focus event on the parent window when they are really destroyed.

Comment 23

[Jean-Francois Ducarroz]

I think I might have an acceptable solution. When I hide the compose, I disable
the window as well, that should normally prevent it to receive any focus event
but that's not enough to avoid it to show up while debugging therefore I
modified GlobalWindowImpl::Focus() to not proceed in case the window is
diseable. In order to do that, I had to add a new interface to nsIWidget:
IsEnable(), like we have already IsVisible.

There is not reason we should accept set the focus on a disable window as the OS
will not pass keyboard or mouse events. Therefore I thing it's the right thing
to do and that should fix this problem. I'll attach a patch soon. Any comment?

Comment 24

[Jean-Francois Ducarroz]

Attachment: Proposed fix v2, widget part

Comment 25

[Jean-Francois Ducarroz]

Attachment: Proposed fix v2, nsGlobalWindow.cpp part

Comment 26

[Jean-Francois Ducarroz]

Attachment: Proposed fix v2, compose part

Comment 27

[Jean-Francois Ducarroz]

Despite all the fact I disable the compose window, I still see it poping up
under debugger even when I am not currently using it! Here is what I am doing:

1) Launch Mozilla under the debugger
2) Open a compose window, close it (don't need to send it)
3) Open the Mail3Pane window
4) set a break point into mime like in MimeInlineTextPlain_parse_begin
5) Click on a message to display it (in that case a plain text one like a
bugzilla email)
6) when the break point is reached, press run
7) --> the previous compose window is back!!!!

Comment 28

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

does your patches fix the scenario in http://bugzilla.mozilla.org/show_bug.cgi?
id=109081#c19?

I believe that when I hardwired to drop the focus event (see 
http://bugzilla.mozilla.org/show_bug.cgi?id=109081#c21) in 
nsMsgComposeService.cpp, nsEventStateManager.cpp, nsWebShellWindow.cpp, 
nsWindow.cpp, it solved even the debugger case.

have you plugged all those holes, including the nsEventStateManager?

Comment 29

[Jean-Francois Ducarroz]

I found why my patch wasn't working properly: The focus event was for the first 
child window which is still enable. Therefore I have to modify the function 
IsEnable to look for the topLevel parent window. I will update the patch soon.

Comment 30

[Jean-Francois Ducarroz]

Meanwhile, I did some more debugging as I have a way to reproduce the problem 
which seems to be the same as the users one or at least very close to it. Here 
is what I found sofar:

Stack trace when we receive a focus event which cause the hidden compose window 
to receive it despite it wasn't addressed to it:

12-  GlobalWindowImpl::Focus(GlobalWindowImpl * const 0x04977bf4) line 1867
11-  nsEventStateManager::PreHandleEvent(nsEventStateManager * const 
0x054aa658, nsIPresContext * 0x049b2390, nsEvent * 0x0012f704, nsIFrame * 
0x04bb3e74, nsEventStatus * 0x0012f670, nsIView * 0x049b0990) line 611
10-  PresShell::HandleEventInternal(nsEvent * 0x0012f704, nsIView * 0x049b0990, 
unsigned int 0x00000001, nsEventStatus * 0x0012f670) line 5846 + 43 bytes
09-  PresShell::HandleEvent(PresShell * const 0x049b0134, nsIView * 0x049b0990, 
nsGUIEvent * 0x0012f704, nsEventStatus * 0x0012f670, int 0x00000001, int & 
0x00000001) line 5777 + 25 bytes
08-  nsView::HandleEvent(nsView * const 0x049b0990, nsGUIEvent * 0x0012f704, 
unsigned int 0x00000000, nsEventStatus * 0x0012f670, int 0x00000001, int & 
0x00000001) line 385
07-  nsViewManager::DispatchEvent(nsViewManager * const 0x049b1d50, nsGUIEvent 
* 0x0012f704, nsEventStatus * 0x0012f670) line 1914
06-  HandleEvent(nsGUIEvent * 0x0012f704) line 83
05-  nsWindow::DispatchEvent(nsWindow * const 0x0552c544, nsGUIEvent * 
0x0012f704, nsEventStatus & nsEventStatus_eIgnore) line 845 + 10 bytes
04-  nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f704) line 866
03-  nsWindow::DispatchFocus(unsigned int 0x0000006b, int 0x00000001) line 4631 
+ 15 bytes
02-  nsWindow::ProcessMessage(unsigned int 0x00000007, unsigned int 0x0008017e, 
long 0x00000000, long * 0x0012fb00) line 3518 + 23 bytes
01-  nsWindow::WindowProc(HWND__ * 0x000801d2, unsigned int 0x00000007, 
unsigned int 0x0008017e, long 0x00000000) line 1113 + 27 bytes
[snap]


what appends:
01- We receive a WM_FOCUS event in window hWnd=801D2
02-06 just pass around the event
07- We try to find the view which will proceed the event, the view chossen is 
the mRootView which (somehow) is the first child (hWnd=801c0) of the hidden 
compose window.
    hWnd=801c0 is not related to hWnd=801D2
08-12 We continue to proceed the focus event but this time with the comose 
window first child hWnd=801c0.

Q: who is hWnd=801D2? Currently hWnd=801D2 is visible!
A: this window as a rect size of (0,0)
   it's a WS_EX_TOPMOST | WS_EX_TOOLWINDOW. My gues it's the famous hidden 
window!

Q: How come hWnd=801c0 is the root view of hWnd=801D2?
A: no idea yet :-(

Comment 31

[Jean-Francois Ducarroz]

the WS_EX_TOPMOST | WS_EX_TOOLWINDOW window is not the "Hidden Window" but 
rather the window associate with the popup menu in the message compose window, 
in fact there is couple of them. Those popup window are not parented to the 
compose window and when we hide the compose window, they stay visible (for the 
OS, you cannot see them because their size is 0 x 0). The question now is why 
those window get the focus sometime, seems t append only if I hide the compose 
window?

At this point, I think I need help... Rods?

Comment 32

[Jean-Francois Ducarroz]

I tried the use the focus controller to activate/deactivate the window like
saari proposed me but it did not work because we are currently resetting the
window as active when we get a focus event!

Now, I'll we try to see if I can make those popup window hidden or at least
disable when they are not open, that might solve the problem too.

Comment 33

[Jean-Francois Ducarroz]

Disabling the popup menu unfortunatly don't fix the problem. Instead of getting
the focus event throught on of the message compose window's popup menu, I know
getting it directly from the compose windows despite this one is disable!
Therefore, the only patch that works is the second one despite the
implementation of the new API IsEnable is not complete on every platform.

Comment 34

[Jean-Francois Ducarroz]

too late for this milestone :-(

Comment 35

[saari (gone)]

Do we know where the focus event that is causing this window to pop forward is
coming from? Which patch do you think is the best? Could we mark the others
obsolete to make it clear? Thanks.

Comment 36

[Jean-Francois Ducarroz]

I still haven't figure out where the activate event comes from! How can we find
that?

Sofar, I have proposed 2 patches:
1) v1 is an uggly patch that will close the window in case this one show up when
it should not. It's more a way to go around the problem.

2) v2 will avoid to set the focus on a disabled window. This patch contains some
platform specific code which is not fully implemeted for each of them.

Comment 37

[scottputterman]

Jean-Francois or Chris, any luck with this patch?

Comment 38

[saari (gone)]

The v2 patches look pretty straight forward. I'm actually looking at bugs right
now where minimized windows in general will pop forward. Seems we're not
deactivating a window when it is minimized. That is related to this bug, but
different right? Different because your window is disabled, which is a different
state than deactivated?

Comment 39

[Jean-Francois Ducarroz]

Currently, the window is just hidden (probably similar to a minimize window
except that the window is not accesible from tha task bar). In the patch, I
propose to disable it which is different than deactivate but that wasn't enough
to prevent the problem, I had to modify the code to refuse to set the focus on a
disable window.

Comment 40

[saari (gone)]

This code is probably okay, but might be redundant over the other fix I'm
working on... unfortunately I'll be on vacation next week.

Tell you what, if you want to check it in, go for it. As I said, it might be
redundant eventually, might not. So instead of marking this fixed when you check
in, reassign it to me. Once I have my fix ready, I can test it and remove this
if it isn't necessary.

Please get bryner and danm to review this. It has my blessing as long as Dan and
Brian are okay with it, r=saari.

Comment 41

[Jean-Francois Ducarroz]

Chris, when the compose window show up by itself, I am not able to minimize it!
it expand right back! Looks like the same problem for me...

What I can do, if you think you could have a fix for your other problem soon,
it's just turn on the flip for the recycled compose window despite of this bug
which occurs rarely. That will give us more testing...

Comment 42

[Jean-Francois Ducarroz]

I am working on a simplified version of the fix which should be ready very soon...

Comment 43

[Jean-Francois Ducarroz]

Attachment: Provosed fix,v3, nsGLobalWIndow.cpp part

This new version of the patch uses nsIWidget::GetNativeData(NS_NATIVE_WIDGET)
to retreive a HWND and then test if the window is enable or not before
accepting to set the focus. I don't need anymore to add an function to
nsIWidget which simplify the fix.

Comment 44

[Jean-Francois Ducarroz]

Attachment: Proposed fix v3, mailnews\compose part

Most of the modifications proposed in the previous patch have been already
commited  as part of other bug fixes. ALl I have to do now is to disable/enable
the window when we hide/show it and also, I need to show the window before we
call the onReopen callback, else the JS will try to set the focus on various
element while the window still disable which will failed.

Comment 45

[varada]

Comment on attachment 70122 [details] [diff] [review]
Proposed fix v3,  mailnews\compose part

r=varada on the mailcompose part alone providing ofcourse that the globalwindow
code prevents setting of focus on disabled windows.

Comment 46

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

Comment on attachment 70122 [details] [diff] [review]
Proposed fix v3,  mailnews\compose part

sr=sspitzer, assuming you get r/sr for the other patch as well.

Comment 47

[Brian Ryner (not reading)]

Comment on attachment 70118 [details] [diff] [review]
Provosed fix,v3,  nsGLobalWIndow.cpp part

Couldn't you use nsIWidget::IsVisible, rather than adding windows-specific code
to nsGlobalWindow?

Comment 48

[Jean-Francois Ducarroz]

Per comment #20, we decide not do take any change to break something. That why I
had to add code!

Comment 49

[Jean-Francois Ducarroz]

cc'ing bryner

Comment 50

[Dan M]

  Hooooboy. Here it is tree-closing day and I'm skirting "jerk" territory. I
somehow suspect that patch 70118 isn't going to be temporary. The patch actually
seems like a decent thing to do, disallowing focus on disabled windows. I think
I don't have a problem with it in concept.
  It could be made a lot nicer by taking out the XP_WIN code, as Brian suggested
in comment 47. That could be done if the method IsEnabled were added to
nsIWidget, right there next to nsIWidget::Enable. That is of course a pain in
the butt. I could let the XP_WIN version go in in view of the date, but I'm
withholding my r= for now. What do people think?

  Oh and while I'm here, I'm going to plunge full into jerk territory. It's such
a short step from the outskirts, anyway. Waugh on the wording of the comment.
"before accepting to set the focus on it" would better read "before accepting
focus." "a window is not disable" should be "a window is not disabled". "weird"
is spelled "ei". So come hit me. I'm rather fond of my language.
  If no one else beats me to the r= bat, you're gonna have to English up the
comment to get mine, you know :)

Comment 51

[Jean-Francois Ducarroz]

I would be glad to change the comment before checking but I will not generate a
new diff just for that...

Comment 52

[Dan M]

Comment on attachment 70118 [details] [diff] [review]
Provosed fix,v3,  nsGLobalWIndow.cpp part

No response on the question of whether we should make you do the nsIWidget
dance. OK. Fix the comment, please. r=me (and just for pendantism's sake, I'll
repeat here that I think the patch is probably OK in concept though would be
happier as an XP modification to the nsIWidget interface, but as far as I'm
concerned it's OK to let that slide for expediency's sake. Damn me, I know.)

Comment 53

[Jean-Francois Ducarroz]

Thanks Dan. Would should do the super review?

Comment 54

[Jean-Francois Ducarroz]

I just tried a patch from Rods which fix this problem as well...

Comment 55

[saari (gone)]

which patch is that? I was supposed to connect with Rod today and got swamped
(big surprise)

Comment 56

[Jean-Francois Ducarroz]

the patch rods is working on is bug 73995. If that fix get checked in, we can
close this bug as a dup of it...

Comment 57

[Jean-Francois Ducarroz]

Attachment: Proposed fix v4, nsGlobalWindow.cpp part

In the new patch, instead of working on the widget level, I use the new API
"enabled" that get added to nsIBaseWindow.

Comment 58

[Jean-Francois Ducarroz]

Attachment: Proposed fix v4, mailnews\compose part

Comment 59

[rods (gone)]

Comment on attachment 72145 [details] [diff] [review]
Proposed fix v4, nsGlobalWindow.cpp part

Look right. r=rods

Comment 60

[Jean-Francois Ducarroz]

danm, bryner, saari and rods, please review and super review the nsGlobalWindow
part. Thanks

varada, sspitzer, please review the compose part. Thanks

Comment 61

[rods (gone)]

Comment on attachment 72146 [details] [diff] [review]
Proposed fix v4, mailnews\compose part

Looks right r=rods

Comment 62

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

Comment on attachment 72146 [details] [diff] [review]
Proposed fix v4, mailnews\compose part

sr=sspitzer

you don't expect the QIs to fail, right?  you could change them to be:

nsCOMPtr<nsIFoo> foo = do_QueryInterface(bar, &rv);

NS_ENSURE_SUCCESS(rv,rv);

to avoid the nesting ifs.

just a style suggestion.

Comment 63

[Alec Flett]

Comment on attachment 72145 [details] [diff] [review]
Proposed fix v4, nsGlobalWindow.cpp part

sr=alecf

Comment 64

[Jean-Francois Ducarroz]

Thanks Alec

Comment 65

[Brendan Eich [:brendan]]

Seth's suggestion in comment #62 is not just stylistic -- the NS_ENSURE_SUCCESS
macro issues an NS_WARNING if NS_FAILED(rv).  So that's substantive in a debug
build.  Saving some overindented code is good too.  :-).

/be

Comment 66

[Asa Dotzler [:asa]]

Comment on attachment 72145 [details] [diff] [review]
Proposed fix v4, nsGlobalWindow.cpp part

a=asa (on behalf of drivers) for checkin to the 0.9.9 branch and the trunk.

Comment 67

[Jean-Francois Ducarroz]

Brendan, I have aggree with Seth to check the fix as it's in 0.9.9 but I will
modify it as suggested for the trunk. The reason I am doing that is that I don't
want to take a risk to check in something in the branch that wasn't fully tested!

Comment 68

[Jean-Francois Ducarroz]

I've checked the fix in the branch.

Comment 69

[Jean-Francois Ducarroz]

Fix checked in the trunk too.

Comment 70

[Jean-Francois Ducarroz]

the dom part of the fix get backed out as it cause bug 128659.

Comment 71

[Brian Ryner (not reading)]

Comment on attachment 72145 [details] [diff] [review]
Proposed fix v4, nsGlobalWindow.cpp part

r=bryner

Comment 72

[Brian Ryner (not reading)]

Comment on attachment 72145 [details] [diff] [review]
Proposed fix v4, nsGlobalWindow.cpp part

(preceding r= is contingent on finding the source of the regression, which I
don't believe is a problem in this code.)

Comment 73

[Jean-Francois Ducarroz]

I have posted a patch for the regression on linux in bug 128659

Comment 74

[Jean-Francois Ducarroz]

Attachment: Proposed fix V1 for fixing the regression exposed by previous patch

this patch has been moved from bug 128659:

I took a close look at danm implementation of isEnable and GetEnable done for
bug 126786 on which my work was based on. I found some inconsistence in the
fact
that sometime those function return true or false as default value whatever if
the function was fully or not implemented. Sofar problem show up only on linux
but we might have problem as well on BeOS and Cocoa.

Before danm implements nsIWidgetIsEnable, we weren't able to query if a widget
was enable or not. We just presumed widget were always enable. Therefore, if we

want to not break that assumption, we must still say a widget is enable by
default unless we now it's realy disabled.

If we cannot determine if a widget is enable or disable, the function IsEnable
must return TRUE and depending of the case an error code. This is not the case
for the windows, beos, linux and cocoa implementation (but doesn't seems to
cause any harm on Windows). I'll post a patch that cleanup all that...

Comment 75

[Jean-Francois Ducarroz]

Comment on attachment 72398 [details] [diff] [review]
Proposed fix V1 for fixing the regression exposed by previous patch

this patch already get a R=bryner while posted in bug 128659

Comment 76

[Jean-Francois Ducarroz]

Comment on attachment 72398 [details] [diff] [review]
Proposed fix V1 for fixing the regression exposed by previous patch

Per blizzard comment in bug 128659, sr=blizzard

Comment 77

[Christopher Blizzard (:blizzard)]

Comment on attachment 72398 [details] [diff] [review]
Proposed fix V1 for fixing the regression exposed by previous patch

a=blizzard on behalf of drivers for 0.9.9 and 1.0

Comment 78

[Jean-Francois Ducarroz]

Fix checked in the branch and the trunk.

Comment 79

[Aleksey Nogin]

I am currently running BuildID 2002030417 on RH Linux 7.2 and I seem to still
have some leftover Composition window focus problems that first appeared
together with bug 128659. Often when I open a new message composition window, I
can not get Subject field or addressees pane to get focus - even it I click
there, it stays in the message body. The workaround I use is to switch to a
browser window and then to come back to composer window.

My be just a coincidence of course, but my guess it's related to this checkin.

Comment 80

[Jean-Francois Ducarroz]

I did not realized that the whole fix get backed out from the branch. Now
everything is back in.

Comment 81

[Aleksey Nogin]

The problem with getting focus into the subject and addressees fields of the
newly opened composer window that I reported in comment #79 are still there in
BuildID 2002031319. Is this a known problem, or should I open a new bug on this?

Comment 82

[Aleksey Nogin]

I've filed bug 131259 on the mail compose window subject and addressee focus
problems. Please dup it if it is a known problem.

Comment 83

[esther]

Using build 20020318 on winxp, linux and mac I did not get a cached compose
window. Since there are no steps to reproduce this and I never saw the problem
on earlier builds I tried several times opening compose windows and replying to
several messages quickly before the compose window finished displaying.  I did
not run into the problem so I will verify this as fixed.  Note: the outstanding
problem Aleksey is having is already logged and dupped.  Scott or Seth if you
see this behavior again please reopen.