Closed Bug 1135009 Opened 5 years ago Closed 5 years ago

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

Categories

(Toolkit :: Reader Mode, defect, P4)

defect

Tracking

()

VERIFIED FIXED
mozilla39
Tracking Status
firefox38 --- verified
firefox39 --- verified

People

(Reporter: ttaubert, Assigned: vaibhavbhosale15, Mentored)

References

Details

(Whiteboard: [good first bug][lang=css])

Attachments

(2 files)

Attached image 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.
The whole reader mode sidebar should probably be hidden when printing?
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.
Mentor: margaret.leibovic
Whiteboard: [lang=css]
FYI, here's the CSS file that should change:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/aboutReader.css
Component: General → Reader Mode
Product: Firefox → Toolkit
Whiteboard: [lang=css] → [good first bug][lang=css]
Hey Margaret,
Can I work on this bug?
(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.
Assignee: nobody → margaret.leibovic
Oops :)
Assignee: margaret.leibovic → vaibhavbhosale15
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
(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
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?
(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.
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.
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.
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!
(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 :).
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!
Priority: -- → P4
Attached patch reader.diffSplinter Review
Attachment #8574596 - Flags: feedback?(margaret.leibovic)
Hey Margaret,
I have submitted a patch. Just ot confirm, your nickname is ':Margaret' right?
(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 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.
Attachment #8574596 - Flags: feedback?(margaret.leibovic) → feedback+
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.
(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
Flags: qe-verify+
QA Contact: andrei.vaida
https://hg.mozilla.org/mozilla-central/rev/c103cfed74e6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
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).
Status: RESOLVED → VERIFIED
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
Attachment #8574596 - Flags: approval-mozilla-beta?
Attachment #8574596 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Depends on: 1158322
I am running Aurora 39.0a2 buildID 20150506004013 and this problem seems to have re-surfaced.
(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.
You need to log in before you can comment on or make changes to this bug.