Last Comment Bug 743252 - Don't print the URL and other information when printing PDFs
: Don't print the URL and other information when printing PDFs
Status: RESOLVED FIXED
https://github.com/mozilla/pdf.js/iss...
: dev-doc-needed
Product: Core
Classification: Components
Component: Printing: Output (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla22
Assigned To: Julian Viereck
:
: Jet Villegas (:jet)
Mentors:
: 839916 843217 846819 847824 (view as bug list)
Depends on: 748935 847049 1197647 844473 848641 853033
Blocks: 714712
  Show dependency treegraph
 
Reported: 2012-04-06 09:13 PDT by JK
Modified: 2016-03-29 10:13 PDT (History)
31 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
+
verified
+
verified
verified


Attachments
WIP (4.49 KB, patch)
2013-02-12 16:32 PST, Julian Viereck
no flags Details | Diff | Splinter Review
WIP 2 (11.46 KB, patch)
2013-02-15 08:10 PST, Julian Viereck
no flags Details | Diff | Splinter Review
Page I've used for testing. (219 bytes, text/html)
2013-02-15 08:12 PST, Julian Viereck
no flags Details
WIP 3 (11.52 KB, patch)
2013-02-15 08:23 PST, Julian Viereck
roc: review+
Details | Diff | Splinter Review
WIP 4 (12.97 KB, patch)
2013-02-17 02:32 PST, Julian Viereck
julian.viereck: review+
Details | Diff | Splinter Review
Disable header/footer when printing using PDF Viewer (1.08 KB, patch)
2013-02-17 02:34 PST, Julian Viereck
no flags Details | Diff | Splinter Review
WIP 4.1 (13.07 KB, patch)
2013-02-20 07:09 PST, Julian Viereck
julian.viereck: review+
bajaj.bhavana: approval‑mozilla‑aurora+
bajaj.bhavana: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Disable header/footer when printing using PDF Viewer (1.11 KB, patch)
2013-02-20 07:10 PST, Julian Viereck
bajaj.bhavana: approval‑mozilla‑aurora+
bajaj.bhavana: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description JK 2012-04-06 09:13:04 PDT
The file name, URL, amount of pages, date and time should not be printed when printing PDFs with pdf.js. Adobe Reader doesn't print them.
Comment 1 Guillaume C. [:ge3k0s] 2012-04-27 07:25:11 PDT
The pdf page should also be printed on the full page.
Comment 2 [:Cww] 2013-01-31 11:02:30 PST
This issue shows up in feedback. I don't know why it got lost.

https://input.mozilla.org/opinion/3498048
https://input.mozilla.org/opinion/3495711

It sort of breaks the point of PDFs which is to have the printed file be exactly as it was intended.
Comment 3 Alex Keybl [:akeybl] 2013-02-01 15:13:14 PST
Triage doesn't feel this necessarily blocks the feature in FF19, but we'd like to hear how easily this may be resolved before making a final decision here. Any ideas Yury?
Comment 4 Yury Delendik (:yury) 2013-02-04 15:43:52 PST
That directly connected with printing of the HTML/CSS content. I have no idea if it's hard or easy to extend the HTML/CSS with some special style attributes to suppress standard header/footer. There is also a possibility to extend the mozPrintCallback to return/set some special settings, but again, it is hard for me to judge is it's easy to add/test that in short period of time.
Comment 5 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-06 12:06:28 PST
Thanks for the feedback Yury, it looks to me like this might involve too much effort for the short time we have left until shipping 19 and at this point we really only want to land critical fixes to b6.  Will continue to track this for 20 so we can get it fixed and properly tested in earlier betas.
Comment 6 Julian Viereck 2013-02-10 12:40:30 PST
*** Bug 839916 has been marked as a duplicate of this bug. ***
Comment 7 Julian Viereck 2013-02-10 12:41:40 PST
How about implement a "mozNoPrintHeaderByDefault" and "mozNoPrintFooterByDefault" flag similar to the "mozdisallowselectionprint" flag implemented in bug #830278?

Some more thoughts are in: 

  https://bugzilla.mozilla.org/show_bug.cgi?id=839916#c0
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-02-11 16:19:44 PST
http://www.w3.org/TR/css3-page/#margin-boxes has some features which could be used to override the default header/footer. However, there has been some discussion that maybe those could be replaced by something simpler.

A moznomarginboxes attribute might be better for now.
Comment 9 Julian Viereck 2013-02-12 07:50:21 PST
Yury told me you're looking for someone to work on this, so I try to do the best I can for this week and maybe have to pass it to someone else afterwards.
Comment 10 Julian Viereck 2013-02-12 08:27:52 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
>
> A moznomarginboxes attribute might be better for now.

Okay - then let's drop my mozNoPrintHeaderByDefault and mozNoPrintFooterByDefault idea and go with mozNoMarginBoxes. 

Assume the <body> element has the mozNoMarginBoxes flag set and the page is printed. Is it still possible to choose the page header/footer from the print settings dialog?
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-02-12 14:13:07 PST
Easiest would be to say "yes" --- only the default header/footer would be removed.
Comment 12 Julian Viereck 2013-02-12 16:32:24 PST
Created attachment 713175 [details] [diff] [review]
WIP

WIP. My machine is still building so I cannot tell you if this is any good or not. Will test it more later today.
Comment 13 Julian Viereck 2013-02-13 15:25:12 PST
There is a problem with the current patch aproach. The print settings implementation reuses the last made settings for the footer/header as the default value for next time's the printing. That means, if one page has the mozNoMarginBoxes attribute and printing is performed, then the user won't get the header/footers again for other pages without changing the settings again. This looks like an unexpected behavior for a user to me.

For setting the default values for the next print action in terms of footer/header, should we ignore the selections made for header/footer in case mozNoMarginBoxes is set?
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-02-13 17:19:46 PST
That seems easiest.
Comment 15 Julian Viereck 2013-02-14 12:51:09 PST
(In reply to Julian Viereck from comment #13)
>
> For setting the default values for the next print action in terms of
> footer/header, should we ignore the selections made for header/footer in
> case mozNoMarginBoxes is set?

My plan to implement this is the following:

- https://mxr.mozilla.org/mozilla-central/source/widget/nsIPrintSettings.idl: Add new getIgnoreMarginBoxOnWritePref() and setIgnoreMarginBoxOnWritePref(in bool ignore). If |ignoreMarginBoxOnWritePref| is set, then the header/footer information is not stored after the printing.
- https://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsPrintSettingsImpl.cpp: implement the new set/get functions
- nsPrintEngine.cpp: call `mPrt->mPrintSettings->setIgnoreMarginBoxOnWritePref(true)` if mozNoMarginBoxes is set
- https://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsPrintOptionsImpl.cpp#502: in `nsPrintOptions::WritePrefs`, skip all the `Preferences::SetString` that are related to footer/header if aPS->getIgnoreMarginBoxOnWritePref() is true.

Any idea for a better name then getIgnoreMarginBoxOnWritePref / setIgnoreMarginBoxOnWritePref is welcome :)
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-02-14 20:17:38 PST
get/setPersistMarginBoxSettings?
Comment 17 Julian Viereck 2013-02-15 08:10:21 PST
Created attachment 714382 [details] [diff] [review]
WIP 2

This patch implements all the functionality as discussed for the moznomarginboxes. It follows the implementation outline (except get/setPersistMarginBoxSettings was implemented as an attribute persistMarginBoxSettings). Pushing this one through the try server in a minute.
Comment 18 Julian Viereck 2013-02-15 08:12:07 PST
Created attachment 714383 [details]
Page I've used for testing.
Comment 19 Julian Viereck 2013-02-15 08:23:01 PST
Created attachment 714396 [details] [diff] [review]
WIP 3

WIP 2 had rejects once I poped and pushed my queue again. Therefore I'm adding here the patch that I pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=86230226dffa
Comment 20 Julian Viereck 2013-02-15 23:11:08 PST
(In reply to Julian Viereck from comment #19)
> Created attachment 714396 [details] [diff] [review]
> WIP 3
> 
> WIP 2 had rejects once I poped and pushed my queue again. Therefore I'm
> adding here the patch that I pushed to try:
> https://tbpl.mozilla.org/?tree=Try&rev=86230226dffa

Try run looks good to me. I've only tested this on OSX so please can someone else download the binaries for other platforms and confirm that the header/footer are not printed by default?
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-02-16 01:25:03 PST
Comment on attachment 714396 [details] [diff] [review]
WIP 3

Review of attachment 714396 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with those fixed.

::: widget/xpwidgets/nsPrintOptionsImpl.cpp
@@ +615,5 @@
>            Preferences::SetBool(GetPrefName(kPrintOddPages, aPrinterName), b);
>          }
>    }
>  
> +  if (aFlags & nsIPrintSettings::kInitSaveHeaderLeft && persistMarginBoxSettings) {

Just put this entire code block inside if (persistMarginBoxSettings).

::: widget/xpwidgets/nsPrintSettingsImpl.cpp
@@ +367,5 @@
> +/* attribute boolean persistMarginBoxSettings; */
> +NS_IMETHODIMP nsPrintSettings::GetPersistMarginBoxSettings(bool *aPersistMarginBoxSettings)
> +{
> +  NS_ENSURE_ARG_POINTER(aPersistMarginBoxSettings);
> +  *aPersistMarginBoxSettings = (bool)mPersistMarginBoxSettings;

Unnecessary cast.

@@ +372,5 @@
> +  return NS_OK;
> +}
> +NS_IMETHODIMP nsPrintSettings::SetPersistMarginBoxSettings(bool aPersistMarginBoxSettings)
> +{
> +  mPersistMarginBoxSettings = (bool)aPersistMarginBoxSettings;

Unnecessary cast.
Comment 22 Julian Viereck 2013-02-17 02:32:02 PST
Created attachment 714887 [details] [diff] [review]
WIP 4

Addresses the review comments made by :roc in comment #21. Carrying over r+.
Comment 23 Julian Viereck 2013-02-17 02:34:06 PST
Created attachment 714888 [details] [diff] [review]
Disable header/footer when printing using PDF Viewer

This sets the |moznomarginboxes| for the PDF Viewer.
Comment 24 Julian Viereck 2013-02-17 02:35:34 PST
Requesting "verifyme" as I've only test this on my OSX/10.8 machine and haven't done testing on other platforms.
Comment 25 [:Cww] 2013-02-19 13:12:36 PST
According to http://boomswaggerboom.wordpress.com/2013/02/19/exciting-stuff-firefox-19s-built-in-pdf-reader/#comment-20789 we're also scaling the page. Can we not do that either?
Comment 26 Heribert Slama 2013-02-19 17:16:35 PST
(In reply to [:Cww] from comment #25)
> [...] we're also scaling the page. Can we not
> do that either?

There really should be a separate Page Setup for PDF with these defaults: scaling factor "Shrink to Fit" (paper size), no margins, no headers and footers. Changes to these settings should be possible on a per print job basis.
Comment 27 Julian Viereck 2013-02-20 04:06:45 PST
(In reply to Heribert Slama from comment #26)
> There really should be a separate Page Setup for PDF with these defaults:
> scaling factor "Shrink to Fit" (paper size), no margins, no headers and
> footers. Changes to these settings should be possible on a per print job
> basis.

Thanks a lot raising this. Can you please open a separate bug for this? Getting the page size handling right will require some more work and specs to be implemented. That's why I don't want it to be covered in this bug here.
Comment 28 Julian Viereck 2013-02-20 07:09:25 PST
Created attachment 716024 [details] [diff] [review]
WIP 4.1

Same as WIP 4 but with patch comment & author.
Comment 29 Julian Viereck 2013-02-20 07:09:51 PST
Comment on attachment 716024 [details] [diff] [review]
WIP 4.1

Carry over r+.
Comment 30 Julian Viereck 2013-02-20 07:10:45 PST
Created attachment 716026 [details] [diff] [review]
Disable header/footer when printing using PDF Viewer

Same as before but with patch comment & author.
Comment 31 Julian Viereck 2013-02-20 07:12:27 PST
I've tested the try-run build on a windows machine and it worked. Therefore removing |verifyme| and added |checkin-needed|.
Comment 32 Ryan VanderMeulen [:RyanVM] 2013-02-20 07:57:20 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/8de609c5d378
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ab6e6416d67

I assume that the PDF Viewer patch will be getting upstreamed as well?
Comment 33 Julian Viereck 2013-02-21 02:04:23 PST
(In reply to Ryan VanderMeulen [:RyanVM] from comment #32)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8de609c5d378
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8ab6e6416d67
> 
> I assume that the PDF Viewer patch will be getting upstreamed as well?

Landed already on the GitHub repo: https://github.com/mozilla/pdf.js/pull/2734
Comment 35 Julian Viereck 2013-02-24 01:51:07 PST
I've created try builds with this patch applyed on Aurora and Beta release channel:

https://tbpl.mozilla.org/?tree=Try&rev=c3b93601e804
https://tbpl.mozilla.org/?tree=Try&rev=72f30589cb30

Both try runs fail on Android. Any idea why?
Comment 36 Alex Keybl [:akeybl] 2013-02-25 15:32:54 PST
(In reply to Julian Viereck from comment #35)
> I've created try builds with this patch applyed on Aurora and Beta release
> channel:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=c3b93601e804
> https://tbpl.mozilla.org/?tree=Try&rev=72f30589cb30
> 
> Both try runs fail on Android. Any idea why?

CCing mfinkle/blassey, who have a lot of experience here. I think try server has issues against Aurora/Beta.
Comment 37 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-26 10:24:25 PST
Once it's determined what the cause of the Android failures are, please nominate for uplift to branches.
Comment 38 Alex Keybl [:akeybl] 2013-02-27 11:25:13 PST
*** Bug 843217 has been marked as a duplicate of this bug. ***
Comment 39 Julian Viereck 2013-02-28 11:41:52 PST
Gave the patch another try-run, but still burning on Android: https://tbpl.mozilla.org/?tree=Try&rev=aa8827708fd4
Comment 40 Brendan Dahl [:bdahl] 2013-03-01 09:59:30 PST
Mark,
Is android on try for aurora/beta permanently broken or is this some issue with the patch?
Comment 41 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-03-01 15:04:02 PST
try is generally unreliable for non-mozilla-central branches, since its slaves use mozilla-central configurations. I think this is particularly true for mobile given bundle name differences and such. So I wouldn't block landing any patches there on try results - you need to monitor your pushes to those repos anyways.
Comment 42 Thomas D. (needinfo?me) 2013-03-02 05:13:48 PST
*** Bug 846819 has been marked as a duplicate of this bug. ***
Comment 43 Mark Finkle (:mfinkle) (use needinfo?) 2013-03-02 06:41:33 PST
Gavin's comment 41 covers it all
Comment 44 Thomas D. (needinfo?me) 2013-03-03 00:29:08 PST
(In reply to [:Cww] from comment #25)
> According to
> http://boomswaggerboom.wordpress.com/2013/02/19/exciting-stuff-firefox-19s-
> built-in-pdf-reader/#comment-20789 we're also scaling the page. Can we not
> do that either?

(In reply to Julian Viereck from comment #27)
> (In reply to Heribert Slama from comment #26)
> > There really should be a separate Page Setup for PDF with these defaults:
> > scaling factor "Shrink to Fit" (paper size), no margins, no headers and
> > footers. Changes to these settings should be possible on a per print job
> > basis.
> 
> Thanks a lot raising this. Can you please open a separate bug for this?
> Getting the page size handling right will require some more work and specs
> to be implemented. That's why I don't want it to be covered in this bug here.

I've filed bug 847049 for the downsizing/scaling issue. Translating the suggested-above understanding of "Shrink to fit" (paper size): We should use original size whereever possible unless that's bigger than the paper, then shrink to paper size without adding margins or whatsoever.

Setting dependency for ease of tracking (and might be technically related).
Comment 45 Julian Viereck 2013-03-03 01:49:22 PST
Comment on attachment 716024 [details] [diff] [review]
WIP 4.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: UA's default print header/footer show up on top of PDF content
Testing completed (on m-c, etc.): yes - landed on m-c a few days back and sticked
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none
Comment 46 Julian Viereck 2013-03-03 01:49:58 PST
Comment on attachment 716026 [details] [diff] [review]
Disable header/footer when printing using PDF Viewer

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: UA's default print header/footer show up on top of PDF content
Testing completed (on m-c, etc.): yes - landed on m-c a few days back and sticked
Risk to taking this patch (and alternatives if risky): very low risk
String or UUID changes made by this patch: none
Comment 47 bhavana bajaj [:bajaj] 2013-03-04 10:33:43 PST
Comment on attachment 716024 [details] [diff] [review]
WIP 4.1

low risk uplift of a recent feature in Fx19.This helps with avoiding printing of header/footer which is the expected behavior.

Please make sure to land it before 3/5(tomorrow) for this to make it into FX 20 beta 3.
Comment 48 Mihaela Velimiroviciu (:mihaelav) 2013-03-05 04:38:17 PST
*** Bug 847824 has been marked as a duplicate of this bug. ***
Comment 49 Ryan VanderMeulen [:RyanVM] 2013-03-05 06:41:25 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/a5167835c4b5
https://hg.mozilla.org/releases/mozilla-aurora/rev/171bd25178c6

Pushed to Aurora. However, this requires binary approval to land on Beta due to the IDL changes. I'll land this on beta once it gets approval.
Comment 50 Thomas D. (needinfo?me) 2013-03-05 06:49:07 PST
(In reply to Ryan VanderMeulen [:RyanVM] from comment #49)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/a5167835c4b5
> https://hg.mozilla.org/releases/mozilla-aurora/rev/171bd25178c6
> 
> Pushed to Aurora. However, this requires binary approval to land on Beta due
> to the IDL changes. I'll land this on beta once it gets approval.

Ryan, thanks for pushing this to Aurora. How can we request the binary approval required for Beta?
Comment 51 Ryan VanderMeulen [:RyanVM] 2013-03-05 09:05:35 PST
302 jorgev :)
Comment 52 Jorge Villalobos [:jorgev] 2013-03-05 12:52:29 PST
It's only an addition to an existing interface, and doesn't look like something binary add-ons would normally use.

ba+ from me.
Comment 53 Thomas D. (needinfo?me) 2013-03-06 03:27:31 PST
(In reply to Ryan VanderMeulen [:RyanVM] from comment #49)
> Pushed to Aurora. However, this requires binary approval to land on Beta due
> to the IDL changes. I'll land this on beta once it gets approval.

(In reply to Jorge Villalobos [:jorgev] from comment #52)
> It's only an addition to an existing interface, and doesn't look like
> something binary add-ons would normally use.
> 
> ba+ from me.

Ryan, with :jorgev's ba+, this is now waiting for you to land it on beta asap :)
Comment 55 Julian Viereck 2013-03-07 23:21:04 PST
For reference: I'm working on a proper standard to disable the header/footer here: http://lists.w3.org/Archives/Public/www-style/2013Mar/0060.html

Removing keyword "dev-doc-needed" as I don't think there is value in documenting this flag. I hope there will be a standardized solution soon that might be different from what we have here at the moment. Please feel free to re-add the flag if you disagree with me.
Comment 56 Bogdan Maris, QA [:bogdan_maris] 2013-03-08 02:04:35 PST
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130307 Firefox/21.0

Verified as fixed on Firefox 20 Beta 4 (buildID: 20130307075451) and latest Aurora (buildID: 20130307042016). The URL and other information are not printed in Header/Footer by default.
Comment 57 Benjamin Smedberg [:bsmedberg] 2013-03-27 05:26:46 PDT
Jorge, this just came up in the context of bug 853033 (because this didn't include a required UUID change). This seems like the kind of interface change that we would automatically reject on beta and should not have recevied ba+.

"It's only an addition to an existing interface" doesn't mean much; any change to interfaces can affect binary compat.
Comment 58 Jorge Villalobos [:jorgev] 2013-03-27 06:23:07 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #57)
> Jorge, this just came up in the context of bug 853033 (because this didn't
> include a required UUID change). This seems like the kind of interface
> change that we would automatically reject on beta and should not have
> recevied ba+.
> 
> "It's only an addition to an existing interface" doesn't mean much; any
> change to interfaces can affect binary compat.

I know any changes can affect binary compatibility, but this didn't appear to be something that add-ons would normally use, which is why I approved it. I didn't notice the missing UUID rev, though :(. You're right that I should be more conservative with these approvals.

The comment about just adding to the interface was meant to address non-binary compatibility.
Comment 59 Julian Viereck 2013-04-23 00:32:45 PDT
(In reply to Julian Viereck from comment #55)
> For reference: I'm working on a proper standard to disable the header/footer
> here: http://lists.w3.org/Archives/Public/www-style/2013Mar/0060.html
> 
> Removing keyword "dev-doc-needed" as I don't think there is value in
> documenting this flag. I hope there will be a standardized solution soon
> that might be different from what we have here at the moment. Please feel
> free to re-add the flag if you disagree with me.

Not sure if there will be a standard for hiding/showing the header soon, so I re-add the "dev-docs-needed" flag for now.
Comment 60 Ioana (away) 2013-05-02 06:31:26 PDT
Verified as fixed on Windows 7, Ubuntu 12.04 and Mac OSX 10.8.3, on Firefox 21 beta 6 (20130430204233).
Comment 61 Ioana (away) 2013-06-18 05:43:39 PDT
Also verified as fixed on Firefox 22 beta 6 - 20130617145905 - same OSs as comment 60.
Comment 62 Tobias Schneider [:tobytailor] 2016-03-29 10:12:23 PDT
Now that we have a more standard conform way to archive the same effect as with mozNoMarginBoxes (see https://bugzilla.mozilla.org/show_bug.cgi?id=1260480) I suggested to remove this feature in https://bugzilla.mozilla.org/show_bug.cgi?id=1260480.
Comment 63 Tobias Schneider [:tobytailor] 2016-03-29 10:13:31 PDT
Sorry, first link was meant to be https://bugzilla.mozilla.org/show_bug.cgi?id=1250674.

Note You need to log in before you can comment on or make changes to this bug.