Bug 88201 - REGRESSION (r118039): Incorrect formatting of pdf when printing from Reader view
Summary: REGRESSION (r118039): Incorrect formatting of pdf when printing from Reader view
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Printing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.7
: P1 Major
Assignee: Nobody
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2012-06-03 17:30 PDT by Wade
Modified: 2012-06-05 18:51 PDT (History)
7 users (show)

See Also:


Attachments
screenshot of resulting incorrect formatting (deleted)
2012-06-03 17:30 PDT, Wade
no flags Details
Regression fix for Reader printing. (3.55 KB, patch)
2012-06-05 01:02 PDT, Vitaly Buka
no flags Details | Formatted Diff | Diff
Regression fix for Reader printing. (3.70 KB, patch)
2012-06-05 01:08 PDT, Vitaly Buka
beidson: review+
Details | Formatted Diff | Diff
Regression fix for Reader printing. (3.70 KB, patch)
2012-06-05 12:11 PDT, Vitaly Buka
beidson: review+
beidson: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wade 2012-06-03 17:30:59 PDT
Created attachment 145496 [details]
screenshot of resulting incorrect formatting

regardless of site, when the Reader function is used and I try to print, the resulting pdf is formatted horribly incorrectly, making a two page document a 200 page document, for instance.
Comment 1 Wade 2012-06-03 17:32:58 PDT
forgot to mention- 'official' version of Safari is not showing same error.
Comment 2 Alexey Proskuryakov 2012-06-04 15:58:36 PDT
<rdar://problem/11569133>
Comment 3 Brady Eidson 2012-06-04 16:17:13 PDT
This is about iframes, and broke in http://trac.webkit.org/changeset/118039
Comment 4 Vitaly Buka 2012-06-04 17:14:57 PDT
Is it possible to export to html Reader result?
Or create layout test with broken feature?
Comment 5 Wade 2012-06-04 17:24:33 PDT
additional point- from the Reader view, the formatting for the email function is just fine. It is only the PDF export that formats incorrectly.
Comment 6 Wade 2012-06-04 17:26:34 PDT
additional point- from the Reader view, the formatting for the email function is just fine. It is only the PDF export that formats incorrectly.
Comment 7 Brady Eidson 2012-06-04 17:37:12 PDT
(In reply to comment #5)
> additional point- from the Reader view, the formatting for the email function is just fine. It is only the PDF export that formats incorrectly.

For the sake of the WebKit engineers, "PDF export" is not specifically interesting.  "Printing mode" is what is interesting.
Comment 8 Vitaly Buka 2012-06-04 17:55:45 PDT
Does any one work on this?
I can try to localize, but there is probability that Reader does something to compensate bug fixed in my patch.
Comment 9 Alexey Proskuryakov 2012-06-04 18:11:00 PDT
> I can try to localize, but there is probability that Reader does something to compensate bug fixed in my patch.

I don't know if Reader does something unusual here, but it certainly doesn't try to compensate for bug 85118 - simply because Safari never shipped with a version of WebKit that had this problem.
Comment 10 Vitaly Buka 2012-06-04 18:27:31 PDT
(In reply to comment #9)
> > I can try to localize, but there is probability that Reader does something to compensate bug fixed in my patch.
> 
> I don't know if Reader does something unusual here, but it certainly doesn't try to compensate for bug 85118 - simply because Safari never shipped with a version of WebKit that had this problem.

My CL fixed two independent issues:
1. Source/WebCore/page/Frame.cpp. It was not shipped with with Safari. And it's unrelated to this issue.
2. Source/WebCore/rendering/RenderView.cpp. It was shipped and present for a long time. For most sites it's just a little strange iframe content when printing. But http://code.google.com/p/chromium/source/search is not printed at all. 

If you revert just RenderView.cpp or just use official Safari you'll see difference on layout tests attached to that bug. Content of iframes alined incorrectly after printing.
Comment 11 Vitaly Buka 2012-06-04 18:36:32 PDT
> If you revert just RenderView.cpp or just use official Safari you'll see difference on layout tests attached to that bug. Content of iframes alined incorrectly after printing.

Actually I am not so sure. I need to experiment with this a little more.
Comment 12 Vitaly Buka 2012-06-04 21:06:28 PDT
I believe I understand the issue.
Base assumption of my CL was that only root frame should handle page size. It ok for regular printing. However Reader requests to print subframe only. So correct condition should be to check page size in closest to root printed frame.

I'll send patch for review soon.
Comment 13 Vitaly Buka 2012-06-05 01:02:58 PDT
Created attachment 145716 [details]
Regression fix for Reader printing.
Comment 14 WebKit Review Bot 2012-06-05 01:04:35 PDT
Attachment 145716 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/page/Frame.cpp:539:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/page/Frame.cpp:540:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Vitaly Buka 2012-06-05 01:08:17 PDT
Created attachment 145717 [details]
Regression fix for Reader printing.
Comment 16 Brady Eidson 2012-06-05 08:40:03 PDT
Comment on attachment 145717 [details]
Regression fix for Reader printing.

View in context: https://bugs.webkit.org/attachment.cgi?id=145717&action=review

> Source/WebCore/page/Frame.cpp:542
> +    // Only top frame being printed should be fit to page
> +    // size. Subframes should be constrained by parents only.
> +    return m_doc->printing() && (!tree()->parent() || !tree()->parent()->m_doc->printing());

This is the OCD in me;  To clean up this comment please move "size." that starts the second line up to the end of the first line
Comment 17 Vitaly Buka 2012-06-05 12:11:00 PDT
Created attachment 145853 [details]
Regression fix for Reader printing.
Comment 18 Vitaly Buka 2012-06-05 12:11:42 PDT
Comment on attachment 145717 [details]
Regression fix for Reader printing.

View in context: https://bugs.webkit.org/attachment.cgi?id=145717&action=review

>> Source/WebCore/page/Frame.cpp:542
>> +    return m_doc->printing() && (!tree()->parent() || !tree()->parent()->m_doc->printing());
> 
> This is the OCD in me;  To clean up this comment please move "size." that starts the second line up to the end of the first line

Done
Comment 19 Darin Adler 2012-06-05 18:51:15 PDT
Committed r119548: <http://trac.webkit.org/changeset/119548>