Closed Bug 351957 Opened 13 years ago Closed 13 years ago

Printing gives error dialog: You cannot print while in print preview

Categories

(Calendar :: Printing, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: sebo.moz, Assigned: dbo)

References

Details

Attachments

(2 files, 2 obsolete files)

When I enter a title in the printer dialog, I get an error after submitting the print job: "The page was replaced while you were trying to print. Please try again" Printing without title works (apart from bug 351944).

Steps to reproduce:
1. create fresh profile
2. create one event
3. in print dialog enter title "test"
4. choose postscript/default, print to file, enter filename

Result: Dialog appears: "The page was replaced while you were trying to print. Please try again", no errors in console.

using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060909 Calendar/0.3a2+
I would consider this to be high visibility if it can be confirmed (probably Linux only). If a title is entered you will never be able to print.
Flags: blocking0.3?
Works for me if the focus is moved away from title textbox after entering a custom title and before pressing Print button; e.g. by selecting another (or same) print option or date range.

You can see from the dialog title when the entered title is taken over as it changes from 'Print preview of Untitled' to 'Print preview of <xyz>'.

If I do not move focus away from title textbox after entering a custom title I see the same alert dialog as above after pressing Print button. -> Confirming

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060916 Calendar/0.3a2+
Status: UNCONFIRMED → NEW
Ever confirmed: true
Given the workaround in comment 3, this doesn't need to block.
Flags: blocking0.3? → blocking0.3-
- The title box's onchange is fired once focus has left that box triggering a refreshHtml().
- Problem seems to be that the preview html document is loaded asynchronously:

document.getElementById("content").contentWindow.location = tempUri.spec;

Thus calling PrintUtils.print() sometimes runs into the stated error message. We need to wait until both the potentially async provider call is back and the document has been loaded successfully before calling print().
The patch fixes:
1. onaccept/printAndClose will add an onload handler to the iframe before calling refreshHtml(), which will trigger printing and cancel/close the dialog. onaccept/printAndClose returns false to keep the dialog open, otherweise the js context is lost.
2. broken init code when no view is present (Lightning only).
Assignee: nobody → daniel.boelzle
Status: NEW → ASSIGNED
Attachment #249253 - Flags: first-review?(lilmatt)
(In reply to comment #4)
> 2. broken init code when no view is present (Lightning only).
> 
Is that bug 361642?
From bug 361642 comment #2, it appears it is.
While printing on Solaris seems to be fixed using a document load listener or the like (e.g. callback in temp file patch), on Linux this doesn't help.

citing bug 351944 comment #9 which fits better here:
What Andreas and I found out:
The cause seems to be that the dialog is closed too early (even with the
document listener patch). If you patch the code keeping it open while printing,
all works fine.
Further it seems that multiple printing device contexts have been created which
doesn't work on Unix (NS_ERROR_GFX_PRINTER_PRINT_WHILE_PREVIEW issued in
gfx/src/ps/nsDeviceContextPS.cpp).
Any guru help appreciated.

BTW: A workaround for this may at least be to change the dialog's "Cancel"
mimic to "Close". A bit helpless, I admit...
(In reply to comment #7)
> The cause seems to be that the dialog is closed too early (even with the
> document listener patch). If you patch the code keeping it open while printing,
> all works fine.
> Further it seems that multiple printing device contexts have been created which
> doesn't work on Unix (NS_ERROR_GFX_PRINTER_PRINT_WHILE_PREVIEW issued in
> gfx/src/ps/nsDeviceContextPS.cpp).
> Any guru help appreciated.
> 
> BTW: A workaround for this may at least be to change the dialog's "Cancel"
> mimic to "Close". A bit helpless, I admit...

For a short term workaround for 0.5, shouldn't we consider the latter option, making printing generally work at all on Unix/Linux? It just makes the situation better, even it won't be the final fix.
BTW: who keeps track of resources/content/calPrintEngine.(xul|css|js)? To me it seems those bits aren't used anymore... cvs remove?
(In reply to comment #8)
Could you elaborate more on the proposed fix?  I'm not sure which changes we need to make to make this work on Unix.


(In reply to comment #9)
When I submitted the large changes to printDialog.* last year, I was told to keep those files around for use when we create a true print preview.

Personally, I would be fine with attaching them to the true print preview bug and cvs rm-ing them, but it's not my call.

(In reply to comment #10)
> (In reply to comment #8)
> Could you elaborate more on the proposed fix?  I'm not sure which changes we
> need to make to make this work on Unix.

a) Before calling PrintUtils.print() (and closing the dialog), it's necessary to wait for the preview being set either by installing an onload listener (this patch) or using a callback (-> "no temp file" bug).
b) Further, on Unix we are running into NS_ERROR_GFX_PRINTER_PRINT_WHILE_PREVIEW, most probably from gfx/src/ps/nsDeviceContextPS.cpp. What works (however I don't know why): keeping the print dialog open while printing.

> (In reply to comment #9)
> When I submitted the large changes to printDialog.* last year, I was told to
> keep those files around for use when we create a true print preview.
> 
> Personally, I would be fine with attaching them to the true print preview bug
> and cvs rm-ing them, but it's not my call.

IMO go ahead; we can get them out of cvs anyway.
Attachment #249253 - Attachment is obsolete: true
Attachment #252929 - Flags: first-review?(lilmatt)
Attachment #249253 - Flags: first-review?(lilmatt)
Comment on attachment 252929 [details] [diff] [review]
further iteration, no temp file use

>Index: printDialog.js
>===================================================================
>-function loadCalendarPrintDialog()
>+function getCalendarView()
> {
>-    // set the datepickers to the currently selected dates
>     var theView = window.opener.currentView();
>-    document.getElementById("start-date-picker").value = theView.startDay.jsDate;
>-    document.getElementById("end-date-picker").value = theView.endDay.jsDate;
>+    if (!theView.startDay)
>+        theView = null;
>+    return theView;
>+}
The if statement needs curly braces.

>-                stream.close();
>+                pipe.outputStream.close();
>+                // byte-array to UTF-8 string:
Add the word "Convert" to this comment to be more clear. My apologies that I didn't mention it last time around.

>+                var convStream =
>+                    Components.classes["@mozilla.org/intl/converter-input-stream;1"]
>+                              .createInstance(Components.interfaces.nsIConverterInputStream);
>+                convStream.init(pipe.inputStream, "UTF-8", 0,
>+                                Components.interfaces.nsIConverterInputStream.DEFAULT_REPLACEMENT_CHARACTER);
>+                try {
>+                    var portion = {};
>+                    while (convStream.readString(-1, portion)) {
>+                        html += portion.value;
>+                    }
>+                }
>+                finally {
>+                    convStream.close();
>+                }
Put the } and finally on the same line, similar to the catch below.

>             } catch (e) {
>-                dump("printDialog::refreshHtml:" + e + "\n");
>+                html = ("printDialog::refreshHtml:" + e + "\n");
>+                dump(html);
>+            }
We don't want to print out the error message, which it appears to me we'll be allowing the user to do if we continue with this approach.
Instead, I think we should fail, and tell the user why.

>+            document.getElementById('content').contentDocument.documentElement.innerHTML = html;
>+            if (finishFunc) {
>+                finishFunc();
>             }
Add a blank line between the document... line and the if statement for increased readibility.


r=lilmatt with those changes
Attachment #252929 - Flags: first-review?(lilmatt) → first-review+
(In reply to comment #13)
> >+    if (!theView.startDay)
> >+        theView = null;
> >+    return theView;
> >+}
> The if statement needs curly braces.
Does it? Is this a definite need/must? IMO it clutters the source.

> >-                stream.close();
> >+                pipe.outputStream.close();
> >+                // byte-array to UTF-8 string:
> Add the word "Convert" to this comment to be more clear. My apologies that I
> didn't mention it last time around.
Ok, but please keep in mind that I just took over that comment "as is" from the original source.

> >+                }
> >+                finally {
> >+                    convStream.close();
> >+                }
> Put the } and finally on the same line, similar to the catch below.

Ok, will do so, although I am really no friend of that style and haven't read it defined anywhere, e.g. <http://developer.mozilla.org/en/docs/JavaScript_style_guide>.

> >             } catch (e) {
> >-                dump("printDialog::refreshHtml:" + e + "\n");
> >+                html = ("printDialog::refreshHtml:" + e + "\n");
> >+                dump(html);
> >+            }
> We don't want to print out the error message, which it appears to me we'll be
> allowing the user to do if we continue with this approach.
> Instead, I think we should fail, and tell the user why.
An error box or dumping to the js console only?

> r=lilmatt with those changes
Is r2 needed? Who'll do it?
(In reply to comment #14)
> > The if statement needs curly braces.
> Does it? Is this a definite need/must? IMO it clutters the source.
Yes, as it was decided a while back to require curly braces for all conditionals, even when not strictly necessary.

> > Add the word "Convert" to this comment to be more clear. My apologies that I
> > didn't mention it last time around.
> Ok, but please keep in mind that I just took over that comment "as is" from
> the original source.
Understood, but since we're touching this line anyway (as far as cvs blame is concerned) I think adding that would clear this up slightly, at least for me.

> > Put the } and finally on the same line, similar to the catch below.
> Ok, will do so, although I am really no friend of that style and haven't read
> it defined anywhere, e.g.
> <http://developer.mozilla.org/en/docs/JavaScript_style_guide>.
Doing so formats it similar to an if/else block.


> > We don't want to print out the error message, which it appears to me we'll
> > be allowing the user to do if we continue with this approach.
> > Instead, I think we should fail, and tell the user why.
> An error box or dumping to the js console only?
An error dialog seems more appropriate, so we don't silently fail and leave the user bewildered as to what's happening, and what possibly went wrong.  However I could be convinced otherwise, and it's really a ui-r question anyway.  I'll chat with dmose about this today when I see him. (I'm in California today.)

> > r=lilmatt with those changes
> Is r2 needed? Who'll do it?
Yes. jminta and mvl are the most likely candidates, as dmose is bogged down with the 0.3.1 release and associated timezone craziness.
(In reply to comment #15)
> > Does it? Is this a definite need/must? IMO it clutters the source.
> Yes, as it was decided a while back to require curly braces for all
> conditionals, even when not strictly necessary.
Can someone write that up into Wiki, please?

> > <http://developer.mozilla.org/en/docs/JavaScript_style_guide>.
> Doing so formats it similar to an if/else block.
In the above mentioned style guide "else" is put on the same level as "if".

> > > We don't want to print out the error message, which it appears to me we'll
> > > be allowing the user to do if we continue with this approach.
> > > Instead, I think we should fail, and tell the user why.
> > An error box or dumping to the js console only?
> An error dialog seems more appropriate, so we don't silently fail and leave the
> user bewildered as to what's happening, and what possibly went wrong.  However
> I could be convinced otherwise, and it's really a ui-r question anyway.  I'll
> chat with dmose about this today when I see him. (I'm in California today.)
Ok, please keep in mind that we couldn't localize those errors, thus I would suggest to just dump into the js console.

> > > r=lilmatt with those changes
> > Is r2 needed? Who'll do it?
> Yes. jminta and mvl are the most likely candidates, as dmose is bogged down
> with the 0.3.1 release and associated timezone craziness.
I will try Joey, he may has some time to do it.
Attachment #252929 - Flags: second-review?(jminta)
We haven't yet solved the Unix/Linux printing issue ("printing failed while in preview"). Wouldn't be the proposed workaround sketched in comment #8 be an (interims) option? It would at least make printing possible on that platforms *now*.
I thought that was part of the patch I reviewed?


If not, let's get a patch for that in review as well.

(In reply to comment #18)
> I thought that was part of the patch I reviewed?
> 
> 
> If not, let's get a patch for that in review as well.

No, I haven't yet prepared a patch for that yet (ie what we talked about on the phone).
Comment on attachment 252929 [details] [diff] [review]
further iteration, no temp file use

There seems to be some extra stuff in here, besides the tempfile removal, that makes this more difficult to review.

As I read this, we're trying to fix this bug, bug 312084, and bug 361642 in this patch?

Can someone confirm/clarify before I go on?
(In reply to comment #20)
> (From update of attachment 252929 [details] [diff] [review])
> There seems to be some extra stuff in here, besides the tempfile removal, that
> makes this more difficult to review.
> 
> As I read this, we're trying to fix this bug, bug 312084, and bug 361642 in
> this patch?
> 
> Can someone confirm/clarify before I go on?
> 

Yes, this patch fixes those, too. Please go on.
Comment on attachment 252929 [details] [diff] [review]
further iteration, no temp file use

+                // byte-array to UTF-8 string:
+                var convStream =
+                    Components.classes["@mozilla.org/intl/converter-input-stream;1"]
+                              .createInstance(Components.interfaces.nsIConverterInputStream);
+                convStream.init(pipe.inputStream, "UTF-8", 0,
+                                Components.interfaces.nsIConverterInputStream.DEFAULT_REPLACEMENT_CHARACTER);
Why are we forcing the printing to be UTF-8?

             } catch (e) {
-                dump("printDialog::refreshHtml:" + e + "\n");
+                html = ("printDialog::refreshHtml:" + e + "\n");
+                dump(html);
+            }
I agree with lilmatt, we should not be presenting errors to the user this way.

+function printAndClose()
+{
+    refreshHtml(
+        function finish() {
+            PrintUtils.print();
+            document.getElementById("calendar-new-printwindow").cancelDialog();
+        });
+    return false; // leave open
+}
+
This is the part that fixes the actual bug right?  It's unfortunate that we have to force another re-draw, but given that we already do that for every letter typed in the title field, I guess it's not so bad.

r2=jminta with the UTF-8 bit explained or fixed, and catch() changes reverted.
Attachment #252929 - Flags: second-review?(jminta) → second-review+
(In reply to comment #22)
> .createInstance(Components.interfaces.nsIConverterInputStream);
> +                convStream.init(pipe.inputStream, "UTF-8", 0,
> +                               
> Components.interfaces.nsIConverterInputStream.DEFAULT_REPLACEMENT_CHARACTER);
> Why are we forcing the printing to be UTF-8?

The html exporter encodes into UTF-8.

>              } catch (e) {
> -                dump("printDialog::refreshHtml:" + e + "\n");
> +                html = ("printDialog::refreshHtml:" + e + "\n");
> +                dump(html);
> +            }
> I agree with lilmatt, we should not be presenting errors to the user this way.

I will dump errors into the js console.
all remarked nits minded and checked in.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
- still to be done: leaving the print dialog open on Unix
- reopening bug
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch dialog open on unix (obsolete) — Splinter Review
Keeping the dialog open works to avoid "cannot print while in print preview" on Unix. We all know that this doesn't fix the root cause, which I assume lies in the PS printing core of the toolkit. However, unless that has been fixed, IMO it's a sensible interim solution on Unix.
Attachment #255205 - Flags: first-review?(lilmatt)
Updating summary because the bug was moved to handle new issue.
Summary: Printing with entered title gives error dialog: The page was replaced while you were trying to print → Printing gives error dialog: You cannot print while in print preview
Comment on attachment 255205 [details] [diff] [review]
dialog open on unix

Unfortunately, #ifndef XP_UNIX will affect OSX as well as Linux/Solaris.

We'll need to do something ugly with multiple ifdefs to make this work properly.

Please also replace "hack:" in your comment with "XXX:"

minusing on the OSX thing
Attachment #255205 - Flags: first-review?(lilmatt) → first-review-
reworked patch
Attachment #255205 - Attachment is obsolete: true
Attachment #256463 - Flags: first-review?(lilmatt)
Comment on attachment 256463 [details] [diff] [review]
dialog open on unix, minding XP_MACOSX

>+++ mozilla/calendar/resources/content/printDialog.js	2007-02-26 16:26:31.918417600 +0100
>+#ifdef XP_MACOSX
>             document.getElementById("calendar-new-printwindow").cancelDialog();
>+#elifndef XP_UNIX
>+            document.getElementById("calendar-new-printwindow").cancelDialog();
>+#endif
I hate repeating code like this. How about this instead:

            var isUnixButNotMac = false;
#ifdef XP_UNIX
#ifndef XP_MACOSX
            isUnixButNotMac = true;
#endif
#endif
            // XXX: Printing fails with a "printing failed while in print
            //      preview" error if the dialog is closed too early on Unix.
            if (!isUnixButNotMac) {
                document.getElementById("calendar-new-printwindow").cancelDialog();
            }




>+++ mozilla/calendar/resources/jar.mn	2007-02-26 15:54:17.536913600 +0100
>-    content/calendar/printDialog.js  (content/printDialog.js)    
>+*   content/calendar/printDialog.js  (content/printDialog.js)    
Remove the trailing spaces from the line you're changing ^^


If you're okay with my suggestion above, r+ with these changes.
Attachment #256463 - Flags: first-review?(lilmatt) → first-review+
(In reply to comment #30)
> I hate repeating code like this. How about this instead:
I hate preprocessors, and even more preprocessors without support for logical terms ;)

> If you're okay with my suggestion above, r+ with these changes.
If you insist, no problem.
Attachment #256463 - Flags: second-review?(mvl)
Comment on attachment 256463 [details] [diff] [review]
dialog open on unix, minding XP_MACOSX

r2=mvl
Attachment #256463 - Flags: second-review?(mvl) → second-review+
fixed and checked in.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
I filed bug 377414 to deal with the workaround on linux. (Print Dialog left open)
verified this doesn't occur any longer
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.