Bug 43658 – Calling window.print() before the page is loaded results in a blank printed page

• Status: RESOLVED
• Resolution: FIXED
• Component: Printing

[Lei Zhang]

Created attachment 63790 [details]
Patch to fix the bug.

This is http://code.google.com/p/chromium/issues/detail?id=25027

I've included a minimal test case above.

The expect result is a printed page with the words "Print me".
The current result with Chrome and Safari is a blank page.

I believe the solution is to defer printing until the page has finished loading. Attached is a patch to do this.

[WebKit Review Bot]

Attachment 63790 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/loader/FrameLoader.cpp:2231:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/page/DOMWindow.h:93:  print_deferred is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/page/DOMWindow.h:401:  m_print_deferred is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 63790 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3598897

[Lei Zhang]

Created attachment 63791 [details]
Patch to fix the bug. (try 2)

[WebKit Review Bot]

Attachment 63791 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/loader/FrameLoader.cpp:2234:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 63790 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3668045

[Tony Chang]

Hamaji might be able to review.  He also might have some ideas on how to write a layout test for this.  There are some general notes about layout tests here:
http://trac.webkit.org/wiki/CreatingLayoutTests
http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

[Dimitri Glazkov (Google)]

Comment on attachment 63791 [details]
Patch to fix the bug. (try 2)

It needs a ChangeLog and a layout test (if possible). Tony's right; Hamaji-san is the right person to help here.

Is this change compatible with what other browsers do? Is it expected that if the script calls print() multiple times while loading, printing will only happen once at most. What happens if the script has called print() while loading, and before the page is done loading, the user closes the page or navigates to another page?

[Lei Zhang]

(In reply to comment #8)
> Is this change compatible with what other browsers do? Is it expected that if the script calls print() multiple times while loading, printing will only happen once at most. What happens if the script has called print() while loading, and before the page is done loading, the user closes the page or navigates to another page?

I tried changing the test page to call print() multiple times. On Windows, we have the following behavior:

IE 8: prompts once always
FF 3.6: prompts once always
Chrome 5: prompts once if you hit cancel, multiple prompts if you hit ok
Safari 4: prompts multiple times always
Opera 10.60: does not prompt at all

Based on this, I'd say it's ok if the WebKit based browser match IE/FF. It doesn't make sense to me to prompt multiple times on initial load. If you know of any cases where one would want to do that, please let me know.


For the question of what happens when a script calls printing while the page is loading, but then the user navigates away, I made a simple html page that calls print(), followed by a few MBs of text. I restricted the bandwidth on the web server to 10 kbps to simulate slow rendering and found the following:

IE 8: If you navigate away before the page loads, the print prompt never shows up.
FF 3.6: If you navigate away, FF will pop up a print prompt and print out what it has loaded - which can be blank or a partial page. This behavior is also a bit weird because you could have closed the last FF window, but you'd get a lone print dialog.

There's no clear defined behavior here either. For WebKit, with my patch, we'd match IE's behavior. Since the page never finished loading, we wouldn't get a print prompt.


It appears I'd need to add a layout test to LayoutTests/printing ? I'll take a look there.

[Lei Zhang]

Created attachment 63966 [details]
Defer printing patch 3 - add a changelog entry and pass check-webkit-style.

Just a clean up patch, no real code change. I looked at the printing layout tests and it's not obvious to me how we can test the behavior in this bug. I'm open to suggestions.

[Shinichiro Hamaji]

Comment on attachment 63966 [details]
Defer printing patch 3 - add a changelog entry and pass check-webkit-style.

Oops. Sorry for the latency. I think the test method I've added cannot help testing window.print(). I think we can add WebCore/manual-tests for this test.

This change looks good to me as the new behavior sounds more compatible to other browsers. I hesitate to r+ this change now because this patch is related to loader which I don't know well.

WebCore/loader/FrameLoader.cpp:2233
 +          window->print();
A question: don't we need to reset m_printDeferred?

[Lei Zhang]

Created attachment 64371 [details]
Defer printing patch 3 - add a manual test.

(In reply to comment #11)
> (From update of attachment 63966 [details])
> Oops. Sorry for the latency. I think the test method I've added cannot help testing window.print(). I think we can add WebCore/manual-tests for this test.

Ok, I added a copy of 25027.htm to manual-tests. See try 4.

> This change looks good to me as the new behavior sounds more compatible to other browsers. I hesitate to r+ this change now because this patch is related to loader which I don't know well.
> 
> WebCore/loader/FrameLoader.cpp:2233
>  +          window->print();
> A question: don't we need to reset m_printDeferred?

DOMWindow::print() resets m_printDeferred.

[Shinichiro Hamaji]

Comment on attachment 64371 [details]
Defer printing patch 3 - add a manual test.

Looks good. As I wrote in Comment 11, I'm not sure I'm a good reviewer for loader code. Could you wait for a few days before you land this to see if no one objects?

> DOMWindow::print() resets m_printDeferred.

Ah, I see. Thanks for clarification!

[WebKit Commit Bot]

Comment on attachment 64371 [details]
Defer printing patch 3 - add a manual test.

Clearing flags on attachment: 64371

Committed r66428: <http://trac.webkit.org/changeset/66428>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

I see multiple problems with this patch:

It’s not good how this patch spreads the feature over two classes. The code in both FrameLoader and DOMWindow is inelegant. All the code should be in one class, with the other class calling over as needed. That way you would not have to expose a printDeferred function.

The name printDeferred is not a good boolean function or data member name. It sounds like a verb "print deferred". I would name it "should print when finished loading":

    m_shouldPrintWhenFinishedLoading

Also, I think FrameLoader::finishedLoading may not be the right place to call this. That's the function called when the main resources is loaded, not when the entire page is loaded.

[Lei Zhang]

(In reply to comment #16)
> I see multiple problems with this patch:
> 
> It’s not good how this patch spreads the feature over two classes. The code in both FrameLoader and DOMWindow is inelegant. All the code should be in one class, with the other class calling over as needed. That way you would not have to expose a printDeferred function.
> 
> The name printDeferred is not a good boolean function or data member name. It sounds like a verb "print deferred". I would name it "should print when finished loading":
> 
>     m_shouldPrintWhenFinishedLoading
> 
> Also, I think FrameLoader::finishedLoading may not be the right place to call this. That's the function called when the main resources is loaded, not when the entire page is loaded.

I'd be happy to fix this to address your concerns. Should I post a follow up patch to this bug / a separate bug, or revert?

If FrameLoader::finishedLoading is the wrong place, then do you know where the right place is?

[Darin Adler]

(In reply to comment #17)
> I'd be happy to fix this to address your concerns. Should I post a follow up patch to this bug / a separate bug, or revert?

I think a separate bug is the cleanest way to attack it.

> If FrameLoader::finishedLoading is the wrong place, then do you know where the right place is?

It would take a little thought to be sure. I would want it to be a place that exactly corresponded to whatever test we're doing to check if it should be deferred. So if the test is "isLoading" then it should be the place that makes isLoading false. Or if the test is "did not fire load event yet", then it should be the place that the load event is fired. Sorry, I don’t have time to do the research myself right now.

[Alexey Proskuryakov]

> I tried changing the test page to call print() multiple times. On Windows, we have the following behavior:
>
> IE 8: prompts once always
> FF 3.6: prompts once always
<...>
> Based on this, I'd say it's ok if the WebKit based browser match IE/FF.

This is not what I see in Firefox 3.6 on Mac or Windows. What I see is that window.print() is always synchronous in Firefox, just like in Safari 5 - as evidenced by the included test case, which fails in Firefox.

So, this patch has changed WebKit from matching Firefox to matching IE. What am I missing? Why does the original issue with Bank of America not occur with Firefox?

[Lei Zhang]

(In reply to comment #19)
> > I tried changing the test page to call print() multiple times. On Windows, we have the following behavior:
> >
> > IE 8: prompts once always
> > FF 3.6: prompts once always
> <...>
> > Based on this, I'd say it's ok if the WebKit based browser match IE/FF.
> 
> This is not what I see in Firefox 3.6 on Mac or Windows. What I see is that window.print() is always synchronous in Firefox, just like in Safari 5 - as evidenced by the included test case, which fails in Firefox.
> 
> So, this patch has changed WebKit from matching Firefox to matching IE. What am I missing? Why does the original issue with Bank of America not occur with Firefox?

Which test case did you try exactly? On Windows XP, with Firefox 3.6.12 and a CutePDF dummy printer, I loaded up http://leiz.org/chromium/25027_test2.html, which calls window.print() 3 times, but only pops up the print dialog once.

screen capture of what I described above:
http://leiz.org/chromium/25027_ff.avi

[Alexey Proskuryakov]

I've been testing with local files, and your test case also results in three synchronous print dialogs when loaded from a local file. Can you confirm that the regression test that went to manual-tests fails in Firefox for you?

Looks like Firefox has a special case for file:///, or maybe it just has some kind of timing issue. Interesting.

[Lei Zhang]

(In reply to comment #21)
> I've been testing with local files, and your test case also results in three synchronous print dialogs when loaded from a local file. Can you confirm that the regression test that went to manual-tests fails in Firefox for you?
> 
> Looks like Firefox has a special case for file:///, or maybe it just has some kind of timing issue. Interesting.

Yes, I do see the different behavior from Firefox when using a file:// url rather than http://. Perhaps some kind of safeguard against malicious webpages?

The regression test did not fail for me with Firefox. When I loaded print-before-load.html, the CutePDF 'printer' output had the text from the webpage. Same for both file:// and http://.

[Alexey Proskuryakov]

> The regression test did not fail for me with Firefox.

That's still strange, but is less important than the fact that Firefox also delays printing web pages until the page is loaded.

(In reply to comment #23)
> That's still strange, but is less important than the fact that Firefox also delays printing web pages until the page is loaded.

As far as I can tell from <http://www.whatwg.org/specs/web-apps/current-work/multipage/timers.html#printing>, the user agent may delay printing, but if it does so, it should still print the page as it was when print() was called. At any rate, it may not defer the printing prompt.

It appears as if Firefox (and perhaps WebKit browsers after this fix) or not in compliance with HTML.

s/or not/are not/

[Lei Zhang]

Then that means IE / FF / Webkit all violates the WHATWG spec, but at least they are consistent with respect to each other.

[Alexey Proskuryakov]

I've sent an e-mail to WHATWG about this.