73995 - hide the "Printing..." preview - dialog windows and popups problems.

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[tgodouet]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux 2.4.0 i686; en-US; 0.8.1) Gecko/20010326
BuildID:    2001032614

After having clicked on the "print" button with a message selected,
a "print" dialog box appears and a "Printing..." preview also.
The problem is that the preview is very small, and can be neither
resized nor scrolled, making it useless.

It seems to be a more general problem of Mozilla : when a dialog 
box appears, every mozilla windows seem to be not resizeable
(the borders move, but the content is not adjusted).


Reproducible: Always
Steps to Reproduce:
1.open Mail component
2.select a mail
3.hit the print button
4.try to resize the "Printing..." preview or another window

Actual Results:  The content of the resized-window is not adjusted to the new
borders size when a dialog window is shown.

Expected Results:  The content of the resized-window should always correspond to
its border.

I think that a dialog box such as the "print" one,
should not block all the windows of Mozilla.
And if it does so, it should always be on top, not being able
to be hidden by another Mozilla window.

I have sometimes a popup telling me a page can't be accessed
which blocks all Mozilla, and it's very annoying when you don't
know where this popup is (on which virtual screen), and even
more when it is hidden by another mozilla window.

Can't a popup block only the window it is related to ?

Comment 1

[Leston Buell]

I've been seeing this for a long time, too. (I'm using Linux, too.) And i agree
that it's annoying when a dialog hidden by a window blocks everything else from
working. You have to minimize all of your windows one at a time to find the
offending dialog.

And while we're at it. Since the print preview window is not a progress window,
and since it displays before printing begins, wouldn't it make more sense for
the window title to be "Print preview" rather than "Printing..."?

Comment 2

[Keyser Sose]

Marking NEW based on comments.

Comment 3

[dcone (gone)]

This is a general dialog problem.. not a printing problem.

Comment 4

[Leston Buell]

The part about dialogs blocking the manipulation of other windows is a general
dialog problem, but the issue about the tiny original size of the print preview
window is not. There seem to be two possible solutions which don't depend on
changing the general behaviour of dialogs:

1. Set the original size of the print preview window to something 
   reasonable.

2. Do not display a print preview window at all.

I think that the second solution makes more sense. The print preview window,
even if it were of a reasonable size, is of little use because you can't scroll
down to view the whole message anyway. The fact that this window locks up when
the dialog appears frustrates the user experience, as well. It's completely
superfluous, and disappears as soon as the dialog is closed anyway. Why not just
elimiate this preview window altogether?

Comment 5

[carl]

Build ID: 2001042021

A very annoying bug that prevents mail access. The ssl certificate dialog is too
small (In general all mozilla dialogs are too small) and cannot be resized. Thus
you cannot continue.

Comment 6

[Koike Kazuhiko]

I can resize "Printing..." window with 2001052411/Linux.
But the preview window isn't necessary. I agree with Leston Buell.

Bug 17006 for [File]-[Print Preview].

Comment 7

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

that "Printing..." dialog is not supposed to be visible.

see bug #58011.  there's a patch started there to make it invisible, so it won't
show up at all (not even in the task bar).

once that patch is finished and landed, this will be invalid.

I'd prefer the reporter to agree, and to mark this invalid.

Comment 8

[Thibault G.]

Ok, let's say that this bug is invalid ...
at least concerning to the print window.

But in fact I'm not sure that bug should be closed, because
it raises the issue of the dialog/error windows with Mozilla :
a "blocker" window should always be on top of moz windows 
(and I'm not sure it is always the case).
Moreover, a blocker window should always be on top on the active
virtual screen.

But the most important thing is that an error window related
to a browser window should only block that window, not all
windows !
And a print dialog for a mail should only block the mail component,
not all browser windows ...
And it is not the case as for now ... :((

But maybe I should open another bug for that ?

Comment 9

[Leston Buell]

This bug seems to be dealing with two distinct issues. I suggest that we limit
the scope of this bug to the "Printing..." preview dialog. This problem is still
occuring. Could someone with the authority to do so, change the summary of this
bug to:
    can't resize "Printing..." preview

The problem with dialogs blocking should be a new bug, if a separate bug has not
already been filed for that.

Comment 10

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

rods may have a fix for this.

we want that window to be hidden.

Comment 11

[rods (gone)]

I decided to tackle this bug because the Print Dialog (on Windows) appears
partially off-screen. The reason is that the Print Dialog is parented to the
hidden window (this should be fixed some day) which is shoved off to the bottom
right of the screen and the Print Dialog wants to place itself in the center of it.

It looks bad and hurts usability.

Comment 12

[rods (gone)]

Attachment: patch

This patch does the following:
1) It adds a "visibility" attr to nsIDOMWindowInternal for setting the 
   visibility of a DOMWindow.
2) Is adds a new method to nsIWidget for suppressing focus notifications and
   the showing of a window. The problem is on Windows only and there are 
   times when you have hidden a window and you do not want to send focus
   notifications. There are time however that this is ok and should happen.
   Also, there times when a GlobalWindow gets activated right before it is 
   hidden and then causes the window to be shown again right after the "hide".
3) The cached mail compose window and the mail hidden Printing window takes
   advatage of this.

NOTE: Adding the visibility attr to the nsIDOMWindowInternal is enough on Mac
and Linux to properly hidden the Printing window. The nsIWidget code is added
to keep the MS-Windows from redisplaying it once it is hidden.

Comment 13

[rods (gone)]

Attachment: new patch

I had the name of a new method wrong in nsBaseWidget, didn't compile on the
Mac.

Comment 14

[Jean-Francois Ducarroz]

Rods, looks like you've attached the wrong patch...

Comment 15

[Jean-Francois Ducarroz]

This fix is a better alternative to the fix for bug 109081 which is currently an
0.9.9 P1 nsbeta1+. We really need this fix in order to enable the cached message
compose window which represent a real perf improvement.

Comment 16

[scottputterman]

If this can land in 0.9.9 that would be a big help for mailnews. We can turn on
one of our bigger performance improvements.

Comment 17

[rods (gone)]

Attachment: new patch

Correct patch

Comment 18

[rods (gone)]

Attachment: the REAL patch

ignore all others

Comment 19

[Jean-Francois Ducarroz]

Comment on attachment 70595 [details] [diff] [review]
the REAL patch

R=ducarroz for the mailnews part of it.

Comment 20

[Johnny Stenback (:jst)]

I don't see this new "visibility" property used from JS anywhere, and I don't
like adding properties to the window object in JS unless it's absolutely
necessary. So, let's move this getter/setter into nsPIDOMWindow in stead, that
way it won't pollute the JS global namespace, ok?

Comment 21

[Kevin McCluskey (gone)]

Marking nsbeta1+

Comment 22

[rods (gone)]

Attachment: full patch with JS files

I am at a loss as to why the JS files were not in the other patch.

Jst, the visibility is set on the window in JS:

printEngineWindow.visibility = false;

After the printing window is created, I then set it to be hidden. I could move
the visibility call into nsPIDOMWindow, except that it isn't scriptable. Would
the "printEngineWindow" be derived from a nsPIDOMWindow so I could QI it if it
was scriptable?

Comment 23

[Johnny Stenback (:jst)]

nsPIDOMWindow is intentionally not scriptable. Adding |visibility| to the global
JS namespacee gives the the willies, I don't like that, at all.

Would it be ok to add this new property only to chrome windows? If so, you could
add it to nsIDOMChromeWindow. Then it would be scriptable, but it would only be
there on chrome windows (i.e. dialogs n' toplevel window objects).

Comment 24

[Jean-Francois Ducarroz]

Comment on attachment 70740 [details] [diff] [review]
full patch with JS files

R=ducarroz for the mailnews part.

Comment 25

[rods (gone)]

Attachment: patch

This patch adds anadditional arg to nsIWidget::Enable. This arg is used to tell
the widget to suppress/ignore focus events. The only platform this is needed
for at this time is Windows. Adding the new arg, means we must touch each
platform.

Also, instead of using the nsIDOMChromeWindow.idl for changing the visibility
we have added an extra method to the nsIMsgPrintEngine.idl "ShowWindow" which
is used to hide the window where the messages are being laid out for printing.

Comment 26

[Kevin McCluskey (gone)]

Comment on attachment 71307 [details] [diff] [review]
patch

You need to remove the	 following cruft from your patch:   
//NS_IMETHOD		  SuppressFocusAndShow(PRBool aSuppressFocAndShow)  { 
mSuppressFocusAndShow = aSuppressFocAndShow; return NS_OK; }
+    NS_IMETHOD 	     SuppressFocusAndShow(PRBool aSuppressFocAndShow) 
{  return NS_OK; }

With that change:

r=[e-mail]

Comment 27

[Jean-Francois Ducarroz]

Comment on attachment 71307 [details] [diff] [review]
patch

I think you should move the code in nsMsgPrintEngine::SetWindow that turn off
show & focus events into the function nsMsgPrintEngine::ShowWindow. That would
make more sense. Also, this patch does contains modification needed by the msg
compose window but if you want I can do it myself, based on this patch, as part
of bug 109081. Appart that, R=ducarroz

Comment 28

[Jean-Francois Ducarroz]

I've just modified the msg compose window code to use the new Enable function to
suppress focus even and that does NOT solve the problem because we receive the
focus event though a related window (in occurance a window created for on of the
popupmenu)! Therefore the variable mIsEnabledFocusEvents is not set to true for
this sub window. That's why you need to stop the focus event in nsGlobalWindow
once  you have retreive the RootView.

Comment 29

[rods (gone)]

Attachment: patch

This patch adds a a new arg to nsIWidget::Enable to indicate that focus events
should be disabled, or in other wrds, not dispatched.

It also disbaled focus events from being diapatched from XUL menus when the XUL
menus are not visible and active.

The mail compose window disbles focus events when the it if hidden, as does the
hidden mail printing window.

Comment 30

[Jean-Francois Ducarroz]

The new patch works for the message compose. I haven't tested the print yet as I
don't have access to a printer right now.

I found two little problems:

1) you need to add gfx in the unix make file for mailnews/compose like you do
for windows.

2) Also, you miss the following part from the patch in bug 109081
Index: src/nsMsgComposeService.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/compose/src/nsMsgComposeService.cpp,v
retrieving revision 1.65
diff -u -2 -w -r1.65 nsMsgComposeService.cpp
--- src/nsMsgComposeService.cpp	12 Feb 2002 03:45:33 -0000	1.65
+++ src/nsMsgComposeService.cpp	18 Feb 2002 19:32:17 -0000
@@ -67,4 +67,5 @@
 #include "nsIDOMElement.h"
 #include "nsIXULWindow.h"
+#include "nsIWidget.h"
 #include "nsIWindowMediator.h"
 #include "nsIDocShellTreeItem.h"
@@ -257,7 +258,8 @@
           */
           nsCOMPtr<nsIDOMWindowInternal> domWindow(mCachedWindows[i].window);
-          mCachedWindows[i].listener->OnReopen(params);
           rv = ShowCachedComposeWindow(domWindow, PR_TRUE);
           if (NS_SUCCEEDED(rv))
+          {
+            mCachedWindows[i].listener->OnReopen(params);
             return NS_OK;
         }
@@ -265,4 +267,5 @@
     }
   }
+  }
       
   //Else, create a new one...

this is needed to avoid trying to reset the focus (OnReopen will do that) before
the compose window get enable.

Comment 31

[Jean-Francois Ducarroz]

The message print works too.

Comment 32

[rods (gone)]

Attachment: comprehensive patch

This includes ducarroz's suggestions.

Comment 33

[Jean-Francois Ducarroz]

Comment on attachment 71885 [details] [diff] [review]
comprehensive patch

one little mistake:
+      rv = widget->Enable(PR_TRUE, !aShow);

should be:
+      rv = widget->Enable(PR_TRUE, aShow);

appart that, R=ducarroz for the mailnews part.

Comment 34

[rods (gone)]

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

Comment 35

[Jean-Francois Ducarroz]

FYI, I am running on Windows the patch I made for the depending bug 109081 for
couple months now without problem, that patch is very similar to the one
proposed here as it block focus event on disable window.

Comment 36

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

sr=sspitzer, but a few small nits and questions

1)

why do we need to center this window?

-                                          "chrome,dialog=no,all",
+                                          "chrome,dialog=no,all,centerscreen",


2)

top is no longer used, so we don't need to calculate it.  the same might be 
true for left, but it's not in the patch.

can you remove those two?

also, why do we need to center this window?

   var top = screen.height + 50;
   printEngineWindow = window.openDialog
("chrome://messenger/content/msgPrintEngine.xul",
 							
	                        "",
-							
	                        "chrome,dialog=no,all",
-							
	                        "left="+left+",top="+top+")");
+							
	                        "chrome,dialog=no,all,centerscreen");

3)

+  if (nsnull == mWindow)
+    return NS_ERROR_NOT_INITIALIZED;;


how about:

+ NS_ENSURE_TRUE(mWindow, NS_ERROR_NOT_INITIALIZED);

(note, you have two semi-colins in your code.)

4)

void ShowWindow(in boolean aShow);

I don't think anyone is ever going to call ShowWindow(true);

seems like this should be:

void HideWindow();

and then, in the C++

+nsMsgPrintEngine::HideWindow()

and

+    // hide the window
+    rv = webShellWindow->Show(PR_FALSE);

[note, the interCaps of this interface needs to be fixed, but that's beyond the 
scope of rods patch.  we can worry about that later.]

5)

Index: mailnews/compose/src/Makefile.in
Index: mailnews/compose/src/makefile.win

don't forget about the mac.

Comment 37

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

whoops, sr=sspitzer for just the mailnews parts, once rods addresses those nits 
and the questions.

Comment 38

[Jean-Francois Ducarroz]

Answer to ssptizer comments:
4) maybe for debugging, we would like an easy way to see the print window. Why
limit that API when you can have both show and hide for the same price!

5) Mac does not need to specify that as we do a recursive include from dist.

Comment 39

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

>Answer to ssptizer comments:
>4) maybe for debugging, we would like an easy way to see the print window. Why
>limit that API when you can have both show and hide for the same price!

good point.  I forgot about for debugging mail print bugs.  best to leave it as 
is.

> 5) Mac does not need to specify that as we do a recursive include from dist.
I didn't know that.  good to know.

Comment 40

[Kevin McCluskey (gone)]

Comment on attachment 71885 [details] [diff] [review]
comprehensive patch

r=[e-mail] for the widget changes

this comment is confusing:

 // If the Window is hidden and we are suppressing the Focus Events 
+	   // Show, then do NOT send a focus event
+	   // Sometimes it is ok to focus a hidden window, 
+	   // at other times this causes it to be shown

Please make it clearer. It does not match the code that comes after it. There
isn't any sort of a check for a hidden window. The code that comes after it
suppresses the focus events based only the mIsEnabledFocusEvents flag

Comment 41

[rods (gone)]

For spitzer's Comments:
Item #1) The window needs to have a resonable size and be centered, because at
the moment the Print Dialog is "centered" to it, which is why the Print Dialog
used to show up in the bottom right. In the future we need to have a way to
parent eht dialog to a different window, because the Printer Dialog is now modal
to the hidden window and not the mail window (need a new bug for this)

Items 2 & 3) I'll fix them.

Comment 42

[rods (gone)]

Oh, fixed Kevin's comment issue.

Comment 43

[Christopher Blizzard (:blizzard)]

I'm seriously confused, here.  Why are we asking the widget code to do this
suppression of notificiation, when there are more central places to do it? (the
event state manager is a good place to start.)

This patch just feels wrong to me.  Something isn't being handled right and
"fixing" the widget code is just painting over the underlying problem.  The
focus code is already a mess in Mozilla, there's no need to make it worse.

Comment 44

[Jean-Francois Ducarroz]

I just found a new simple way to solve this problem (at least the one with the
message compose window). Instead of working at the widget level as we do since
the beginning, we can just work at the window level (nsIBaseWindow) by calling
the API enabled. Then all we need to do is to block the focus event if the
window is disable in nsGlobalWindow.cpp. I'll post a patch asap...

Comment 45

[rods (gone)]

Chris, it is a widget toolkit problem. Although, it looks like danm has added a
new "enabled" property to nsIBaseWindow. Which is something I looked into, but I
didn't want to touch all the classes that implement that interface (go danm). 

The other interesting thing about this, is that it only happends on Windows.
MS-Windows sends focus events to hidden (and I think, even disabled windows)

So these unwanted events were making it up to the nsGlobalWindow. I agree that
the current patch in this bug is not great, but up until danm did his thing, it
was the best approach purposed so far.

Comment 46

[Jean-Francois Ducarroz]

Since we have a fix for the underline problem under bug 109081 which I'll
checkin very soon, I don't  think this bug should be assigned to rods. Reassign
back to sspitzer.

I'll post an updated patch for the remaing work...but as I don't know how to
reproduce the problem, I cannot certify it will fix the bug. 

Comment 47

[Jean-Francois Ducarroz]

And thank you Rod for all your the time spent on this bug.

Comment 48

[Jean-Francois Ducarroz]

Attachment: news patch based on fix for bug 109081 and rods' previous patch

Seth, can you test if it realy fix the problem

Comment 49

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

moving to 1.0, not a blocker for 0.9.9.

after ducarroz lands #109081, I'll apply and work on this patch.

I second ducarroz:  thanks rods for doing all the work for this.  when I check 
in, I'll re-assign back to give credit to you.

Comment 50

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

Attachment: updated patch

waiting for ducarroz to land to fully test this.

this patch includes some of the clean up I mentioned during the code reviews,
and cleans up the interface to follow proper interCaps.

Comment 51

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

Attachment: updated patch, we can remove some strings since this dialog is not going to be visible anymore

Comment 52

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

the latest patch is essentially rods' patch, with the some cleanup and the 
interCaps change.  also, since this window is no longer visible, I removed the 
window title from the .dtd file, and simplified the print engine xul.

with this patch alone, the print window comes up in the middle of the screen.

but, with the mozilla/dom patch for #109081, the window is hidden and it 
doesn't show in the toolbar, which means this bug is fixed.

once #109081, that last patch can land, assuming I get r=,sr,and a=.

Comment 53

[Jean-Francois Ducarroz]

Comment on attachment 72207 [details] [diff] [review]
updated patch, we can remove some strings since this dialog is not going to be visible anymore

R=ducarroz

Comment 54

[David :Bienvenu]

Comment on attachment 72207 [details] [diff] [review]
updated patch, we can remove some strings since this dialog is not going to be visible anymore

sr=bienvenu

Comment 55

[Asa Dotzler [:asa]]

Comment on attachment 72207 [details] [diff] [review]
updated patch, we can remove some strings since this dialog is not going to be visible anymore

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 56

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

fixed.  thanks to rods for the initial patch.

Comment 57

[esther]

Using build 20030318 on winxp, mac and linux this is what I found.  I think this
is fixed for the original scenario. If any of the results I have found are new
bugs let me know and I will log them. 

Winxp, mac and linux all bring up the Print dialog only when "Print" is selected
(note no Print preview dialog as mentioned in original steps pops up).  = OK
This Print dialog is large enough to see and is fully displayed on screen
(meaning not in corner partially off screen and not too small to use it). =  OK

Winxp, mac and linux bring up a status of printing window which stays up until
printing is done, cancelled or is closed.  = OK
This status window is not resizable for winxp and mac but can be resized on
linux.  = Not sure, is this by design for linux OS?
*This status window on does not allow user to open any other app window while it
is up or when the status window has been closed using the x button (*to see this
behavior you have to print a message with embedded jpgs using a slow system). 
=I believe this is correct behavior for a modal window, however when this window
is closed using the x button it does not cancel the printing and the user cannot
open other mail messages or app windows until the printing is completed.

Comment 58

[esther]

verifying, reopen if results show the fix was not complete.