1135009 - Printing in reader mode cuts off on the left (sidebar overlays text)

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect, P4)

Description

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Photo of printout

Just tried printing in reader mode. The whole article is cut off/overlaid on the left.

Also: the fonts looks quite huge compared to how it looks on screen. Not sure if that should go into a different bug. The URL shown at the top is about:reader?url=..., ideally this would be the original article URL.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

The whole reader mode sidebar should probably be hidden when printing?

Comment 2

[:Margaret Leibovic]

It looks like we can use media queries to specify CSS when printing:
http://stackoverflow.com/questions/355313/how-do-i-hide-an-element-when-printing-a-web-page

We should try using that to hide the toolbar. And we can probably also use that to adjust the font size.

As for the URL at the top, that might require some lower-level change... I'm not sure how to address that.

Comment 3

[:Margaret Leibovic]

FYI, here's the CSS file that should change:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/aboutReader.css

Comment 4

[Vaibhav Bhosale]

Hey Margaret,
Can I work on this bug?

Comment 5

[:Margaret Leibovic]

(In reply to Vaibhav Bhosale from comment #4)
> Hey Margaret,
> Can I work on this bug?

Yes! Do you understand what needs to happen here? Let me know if you have any questions, I'm happy to help.

Comment 6

[:Margaret Leibovic]

Oops :)

Comment 7

[Vaibhav Bhosale]

Hi Margaret,
Can you help me understand where the actual problem lies. I mean how do I know for which div I have to make the changes.

Thank You!

Regards,
Vaibhav

Comment 8

[:Margaret Leibovic]

(In reply to Vaibhav Bhosale from comment #7)
> Hi Margaret,
> Can you help me understand where the actual problem lies. I mean how do I
> know for which div I have to make the changes.
> 
> Thank You!
> 
> Regards,
> Vaibhav

You should be able to use the inspector from the developer tools to inspect the page to find the toolbar, just like you would do if you were developing a normal webpage. Have you used that tool before? If not, you can learn more about it here:
https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/UI_Tour

Comment 9

[Vaibhav Bhosale]

Is this bug specific to some particular types of webpages?
I chacked the print preview of a few. They showed similar issue. But I am still confused how can changes made in toolkit/themes/windows/global/aboutReader.css will be visible in every web-page?

Comment 10

[:Margaret Leibovic]

(In reply to Vaibhav Bhosale from comment #9)
> Is this bug specific to some particular types of webpages?
> I chacked the print preview of a few. They showed similar issue. But I am
> still confused how can changes made in
> toolkit/themes/windows/global/aboutReader.css will be visible in every
> web-page?

This bug is about viewing webpages in reader view. First, you'll need to enable reader view (if you haven't already) by setting "reader.parse-on-load.enabled" to true in about:config. Then you should navigate to an page that shows the reader view button (it should appear on most blog/news posts). When you click on the button, you will see reader view appear. Under the hood, that button is loading aboutReader.html, which is how the aboutReader.css styles are loaded.

Then, if you try to print the article (you can just print to a pdf to test), you'll see the reader mode toolbar appear. What we want to do in this bug is add some CSS that will hide that toolbar when printing the page.

Comment 11

[Vaibhav Bhosale]

I am unable to see the option reader.parse-on-load.button when I open about:config
Has it got something to do with me having an Ubuntu machine? Because when I tried looking for it on the web, almost all the links were related to the android or windows version.

Comment 12

[Vaibhav Bhosale]

Hey Margaret,
I have understood the problem now. But I am unable to see the print preview of the page.
It shows an error:
e10s printing is not implemented yet. Bug 927188.

Comment 13

[Vaibhav Bhosale]

Also, I suppose adding the following snippet would help in solving this problem
@media print {
  .toolbar, .toolbar * {
    display: none !important;
  }
}

Please rectify me if I am wrong.
Thank You!

Comment 14

[:Margaret Leibovic]

(In reply to Vaibhav Bhosale from comment #12)
> Hey Margaret,
> I have understood the problem now. But I am unable to see the print preview
> of the page.
> It shows an error:
> e10s printing is not implemented yet. Bug 927188.

Ah, yes. You should just disable e10s for testing this bug (or test in a new non-e10s window).

(In reply to Vaibhav Bhosale from comment #13)
> Also, I suppose adding the following snippet would help in solving this
> problem
> @media print {
>   .toolbar, .toolbar * {
>     display: none !important;
>   }
> }
> 
> Please rectify me if I am wrong.
> Thank You!

This looks like the correct approach! However, I don't think we need the !important, and we can probably just do .toolbar as the selector (although you should test this to make sure I'm correct :).

Comment 15

[Justin Dolske [:Dolske]]

P4 based on this not being a primary use-case and, other than frustration the first time you do it, just means learning not to print from reader view.

But we'd really really love a fix, and awesome that someone's started to look at this!

Comment 16

[Vaibhav Bhosale]

Attachment: reader.diff

Comment 17

[Vaibhav Bhosale]

Hey Margaret,
I have submitted a patch. Just ot confirm, your nickname is ':Margaret' right?

Comment 18

[:Margaret Leibovic]

(In reply to Vaibhav Bhosale from comment #17)
> Hey Margaret,
> I have submitted a patch. Just ot confirm, your nickname is ':Margaret'
> right?

Yep, thanks for the patch! I'll take a closer look some time today.

Comment 19

[:Margaret Leibovic]

Comment on attachment 8574596 [details] [diff] [review]
reader.diff

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

Looking good! Just 2 tiny issues, and then we can land this!

Also, I think we should add this to the Android CSS file as well, since we may support cloud printing at some point (and this would also work with save as pdf).

http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReader.css

::: toolkit/themes/windows/global/aboutReader.css
@@ +404,5 @@
>    background-image: url("chrome://global/skin/reader/RM-Reading-List-24x24.svg");
>  }
> +
> +@media print {
> +  .toolbar{

Please add a space before the open brace.

@@ +407,5 @@
> +@media print {
> +  .toolbar{
> +    display: none;
> +  }
> +}
\ No newline at end of file

Please make sure there's a new line at the end of the file.

Comment 20

[Fred Wenzel [:wenzel]]

Noticed this on today's nightly. Are we landing this? :)

@Dolske: I actually think printing (likely: to PDF) is a really good use case for reader mode. Printing ad- and UI-laden web pages is a nightmare.

Comment 21

[:Margaret Leibovic]

(In reply to Fred Wenzel [:wenzel] from comment #20)
> Noticed this on today's nightly. Are we landing this? :)
> 
> @Dolske: I actually think printing (likely: to PDF) is a really good use
> case for reader mode. Printing ad- and UI-laden web pages is a nightmare.

Thanks for poking this.

I just fixed the nits and landed this:
https://hg.mozilla.org/integration/fx-team/rev/c103cfed74e6

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c103cfed74e6

Comment 23

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 39.0a1 (2015-03-20) using Ubuntu 14.04 (x64), Windows 7 (x64) and Mac OS X 10.9.5.

Testing was done by printing several articles to pdf. The sidebar was no longer visible in them in any way and their text was displayed properly on my test machines. 

Please note that the URL at the top of each file is still the same as in Comment 0 - I assume that'll be treated in a separate bug when a fix is found (see Comment 2).

Comment 24

[:Margaret Leibovic]

Comment on attachment 8574596 [details] [diff] [review]
reader.diff

Approval Request Comment
[Feature/regressing bug #]: reader view
[User impact if declined]: reader view toolbar will appear when printing articles
[Describe test coverage new/current, TreeHerder]: landed on nightly/aurora
[Risks and why]: low-risk small CSS change
[String/UUID change made/needed]: none

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a1214cc8c57e

Comment 26

[Andrei Vaida [:avaida]]

Verified fixed on Beta 38.0b2-build1 (20150406174117) as well, using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

Comment 27

[Bill]

I am running Aurora 39.0a2 buildID 20150506004013 and this problem seems to have re-surfaced.

Comment 28

[Bill]

(In reply to Bill from comment #27)
> I am running Aurora 39.0a2 buildID 20150506004013 and this problem seems to
> have re-surfaced.

Whoops! I see that this has already been reported in 1158322.