1319067 - Make the printing media section respect the font size

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect)

Description

[Marco Zehe (:MarcoZ)]

Neil Osman (CC'ed) sent me this report by e-mail. Excerpt:

Lately a media print section was added to aboutReader.css (line 18) that sets printing to a very small font size (14px), regardless of user configuration.  
This  prevents me from printing in my preferred font size (largest). 
I think this change doesn't make sense - why would someone who sets viewing to the highest font size would wish to read printed pages any differently? 

End of quote.

The preselection for a font size for printing should be smart enough to choose a relative font size that matches the font size used while reading something on the screen.

Comment 1

[:Gijs (he/him)]

Why are we force-setting a font-size in the first place? Matheus?

Comment 2

[Matheus Longaray (:mlongaray)]

(In reply to :Gijs Kruitbosch from comment #1)
> Why are we force-setting a font-size in the first place? Matheus?

I'm pretty confident it was an UX call when we first sent over a Simplify Page build for UX review. Text size could get very large by default (so going a step or smaller would really help there). As well as better using the column width in order to maintain a sensible character-per-line count.

We could move media print sections added on aboutReader.css and aboutReaderContent.css to simplifyMode.css. Do you guys think that would work?

Comment 3

[:Gijs (he/him)]

(In reply to Matheus Longaray (:mlongaray) from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > Why are we force-setting a font-size in the first place? Matheus?
> 
> I'm pretty confident it was an UX call when we first sent over a Simplify
> Page build for UX review. Text size could get very large by default (so
> going a step or smaller would really help there). As well as better using
> the column width in order to maintain a sensible character-per-line count.

Without the fixed font size, do we:

- use the user's reader mode preferences (which I recognize they can't change in "simplify" mode) ? If so, I think we should just do that and remove the styling. Optionally, we could add a control to the print preview toolbar in simplified mode to control the font size.
- use the user's preffered default font size (16px by default, I think, modifiable in the main settings/preferences pages, outside of reader mode) then I think that is also fine and we don't need to set anything.
- use something else, and if so, what? In this case I could be convinced that keeping something in simplifyMode.css would be a good idea.

Philipp/Stephen, can you elaborate on what you think we're supposed to be doing here, and/or how you feel about the above?

Comment 4

[Matheus Longaray (:mlongaray)]

(In reply to Matheus Longaray (:mlongaray) from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > Why are we force-setting a font-size in the first place? Matheus?
> 
> I'm pretty confident it was an UX call when we first sent over a Simplify
> Page build for UX review. Text size could get very large by default (so
> going a step or smaller would really help there). As well as better using
> the column width in order to maintain a sensible character-per-line count.
> 
> We could move media print sections added on aboutReader.css and
> aboutReaderContent.css to simplifyMode.css. Do you guys think that would
> work?

Since we are now injecting the extracted article directly into the DOM (simplified browser), we may not need to set print styling on AboutReader/AboutReaderContent.css after all. I think we missed this when implementing the feature because we were using "about:reader" infra when simplifying at first.

If we decide to set those same rules when using Simplify Page, we could add it to simplifyMode.css and/or change the DOM structure we are injecting (see at http://searchfox.org/mozilla-central/source/toolkit/content/browser-content.js#561).

Thank you for drawing my attention to this, Gijs.

Comment 5

[Matheus Longaray (:mlongaray)]

Attachment: WIP1: Move @media print queries to simplifyMode.css

This patch moves @media print queries from AboutReader.css/AboutReaderContent.css to simplifyMode.css.

"about:reader" would not be affected anymore by @media print queries previously added. Simplify Page feature would still leverage some styling from "about:reader" while adding styling for its own use.

Patch was tested on Ubuntu and OS X (preview activated locally) and worked as expected.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8817173 [details] [diff] [review]
WIP1: Move @media print queries to simplifyMode.css

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

Can we keep the margins in the aboutReaderContent.css ?

More generally, can we avoid setting the font size at all here? That seems like it should Just Work based on the default font size (though I haven't tested - see comment 3 for some relevant questions)

Comment 7

[Matheus Longaray (:mlongaray)]

(In reply to :Gijs Kruitbosch from comment #6)
> Comment on attachment 8817173 [details] [diff] [review]
> WIP1: Move @media print queries to simplifyMode.css
> 
> Review of attachment 8817173 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can we keep the margins in the aboutReaderContent.css ?

Absolutely. I also added padding to align the content when printing (same way content is aligned when displayed on reader mode).

> More generally, can we avoid setting the font size at all here? That seems
> like it should Just Work based on the default font size (though I haven't
> tested - see comment 3 for some relevant questions)

Agreed.

Comment 8

[Matheus Longaray (:mlongaray)]

Attachment: WIP2: Avoid force-setting the font size

This patch removes force-setting of font size when simplifying. It also corrects where we inject simplifyMode.css into the DOM of our simplified browser.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8817441 [details] [diff] [review]
WIP2: Avoid force-setting the font size

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

This looks OK to me, though I'm not sure why we're adding padding?

::: toolkit/themes/shared/aboutReaderContent.css
@@ +54,5 @@
>    li,
>    figure,
>    .wp-caption {
>      margin: 0 0 10px 0 !important;
> +    padding: 10px 0 !important;

Why the addition of padding here?

Comment 10

[Matheus Longaray (:mlongaray)]

(In reply to :Gijs Kruitbosch from comment #9)
> Comment on attachment 8817441 [details] [diff] [review]
> WIP2: Avoid force-setting the font size
> 
> Review of attachment 8817441 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks OK to me, though I'm not sure why we're adding padding?
> 
> ::: toolkit/themes/shared/aboutReaderContent.css
> @@ +54,5 @@
> >    li,
> >    figure,
> >    .wp-caption {
> >      margin: 0 0 10px 0 !important;
> > +    padding: 10px 0 !important;
> 
> Why the addition of padding here?

We actually do not need to add padding here. I think we previously set our margin for printing wrongly.

You can see at http://searchfox.org/mozilla-central/source/toolkit/themes/shared/aboutReaderContent.css#111 that reader mode sets the left margin as -10px (for those elements).

So when we set "margin: 0 0 10px 0" for printing, those elements were not aligned anymore as it was when presenting reader mode.

What we could do instead is only set "margin-bottom: 10px" for printing so those elements remain aligned.

Comment 11

[Matheus Longaray (:mlongaray)]

Attachment: WIP3: Fix margin issue when printing

This patch addresses a margin issue when printing from reader mode and simplify page. It also adds yet another rule for thumbcaptions so other elements do not overlap its box-model space.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8817474 [details] [diff] [review]
WIP3: Fix margin issue when printing

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

::: toolkit/themes/shared/aboutReaderContent.css
@@ +53,5 @@
>    ol,
>    li,
>    figure,
>    .wp-caption {
> +    margin-bottom: 10px !important;

So this leaves the other margins to be negative, doesn't that bring the edges of the text too close to the page sides?

@@ +57,5 @@
> +    margin-bottom: 10px !important;
> +  }
> +
> +  div.thumbcaption > p.readability-styled {
> +    display: block !important;

What are thumbcaptions? Are those classes we add? I suspect this is just a bug in readability that we should fix (it should remove classes it doesn't add itself).

Comment 13

[Matheus Longaray (:mlongaray)]

(In reply to :Gijs Kruitbosch from comment #12)
> Comment on attachment 8817474 [details] [diff] [review]
> WIP3: Fix margin issue when printing
> 
> Review of attachment 8817474 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/themes/shared/aboutReaderContent.css
> @@ +53,5 @@
> >    ol,
> >    li,
> >    figure,
> >    .wp-caption {
> > +    margin-bottom: 10px !important;
> 
> So this leaves the other margins to be negative, doesn't that bring the
> edges of the text too close to the page sides?
> 

In this case, text does not extrapolate page boundaries because we are setting a padding of 10px to those elements right after setting margins, see here: http://searchfox.org/mozilla-central/source/toolkit/themes/shared/aboutReaderContent.css#112.

I think we could let margin the way it was, and set padding to 0 to get all elements aligned.

> @@ +57,5 @@
> > +    margin-bottom: 10px !important;
> > +  }
> > +
> > +  div.thumbcaption > p.readability-styled {
> > +    display: block !important;
> 
> What are thumbcaptions? Are those classes we add? I suspect this is just a
> bug in readability that we should fix (it should remove classes it doesn't
> add itself).

Sorry, you're right. I thought we were adding this class for captions we detect. This class is present on Wikipedia pages, e.g., https://en.wikipedia.org/wiki/Albert_Einstein.

I saw that the only place readability adds "readability-styled" class to an element is at http://searchfox.org/mozilla-central/source/toolkit/components/reader/Readability.js#725. Since we added this class to the element, we will not be able to remove the original class at http://searchfox.org/mozilla-central/source/toolkit/components/reader/Readability.js#1173. Is that correct? At least it could be something to start with.

Comment 14

[Matheus Longaray (:mlongaray)]

Attachment: ul/ol elements not aligned on reader mode

Screenshot showing that ul/ol elements are not aligned to the text on reader mode.

Comment 15

[Matheus Longaray (:mlongaray)]

Attachment: WIP4: Sets padding to 0 and fixes alignment issue for ul/ol elements

This patch sets padding to 0 so keep all nodes aligned as well as fixes an alignment issue for ul/ol elements (see screenshot attached to the bug).

Comment 16

[Matheus Longaray (:mlongaray)]

Comment on attachment 8818088 [details]
ul/ol elements not aligned on reader mode

Note: red line represents our content's border; blue represents the ol's border; green represents the ul's border.

Comment 17

[Matheus Longaray (:mlongaray)]

Comment on attachment 8818089 [details] [diff] [review]
WIP4: Sets padding to 0 and fixes alignment issue for ul/ol elements

Note: adjustments added only for printing.

Comment 18

[:Gijs (he/him)]

Comment on attachment 8818089 [details] [diff] [review]
WIP4: Sets padding to 0 and fixes alignment issue for ul/ol elements

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

r=me without the list style changes.

::: toolkit/themes/shared/aboutReaderContent.css
@@ +59,5 @@
> +  }
> +
> +  ul,
> +  ol {
> +    list-style-position: inside !important;

The consequence of this is that list items wrap like this:

* foo bar baz
quux stuff
* another list
item with text


Which doesn't seem desirable.

Comment 19

[:Gijs (he/him)]

(In reply to Matheus Longaray (:mlongaray) from comment #16)
> Comment on attachment 8818088 [details]
> ul/ol elements not aligned on reader mode
> 
> Note: red line represents our content's border; blue represents the ol's
> border; green represents the ul's border.

Sorry, does this mean that when we print we don't show the list item number / symbol at all? That's not what I saw when printing to PDF in a quick test...

Comment 20

[Matheus Longaray (:mlongaray)]

(In reply to :Gijs Kruitbosch from comment #19)
> (In reply to Matheus Longaray (:mlongaray) from comment #16)
> > Comment on attachment 8818088 [details]
> > ul/ol elements not aligned on reader mode
> > 
> > Note: red line represents our content's border; blue represents the ol's
> > border; green represents the ul's border.
> 
> Sorry, does this mean that when we print we don't show the list item number
> / symbol at all? That's not what I saw when printing to PDF in a quick
> test...

It doesn't happen when printing from reader mode because we're setting an specific padding to the document's body, so the list item number/symbol will be showed on the PDF. See here: http://searchfox.org/mozilla-central/source/toolkit/themes/shared/aboutReader.css#6.

In this patch we're setting padding to 0 on simplifyMode.css, so it might happen that the list item number be cut out when printing, e.g., list item number with long digits. Should we also set an specific padding to simplifyMode.css as well?

Comment 21

[:Gijs (he/him)]

(In reply to Matheus Longaray (:mlongaray) from comment #20)
> (In reply to :Gijs Kruitbosch from comment #19)
> > (In reply to Matheus Longaray (:mlongaray) from comment #16)
> > > Comment on attachment 8818088 [details]
> > > ul/ol elements not aligned on reader mode
> > > 
> > > Note: red line represents our content's border; blue represents the ol's
> > > border; green represents the ul's border.
> > 
> > Sorry, does this mean that when we print we don't show the list item number
> > / symbol at all? That's not what I saw when printing to PDF in a quick
> > test...
> 
> It doesn't happen when printing from reader mode because we're setting an
> specific padding to the document's body, so the list item number/symbol will
> be showed on the PDF. See here:
> http://searchfox.org/mozilla-central/source/toolkit/themes/shared/
> aboutReader.css#6.
> 
> In this patch we're setting padding to 0 on simplifyMode.css, so it might
> happen that the list item number be cut out when printing, e.g., list item
> number with long digits. Should we also set an specific padding to
> simplifyMode.css as well?

Can we just avoid setting padding to 0 for ul/ol/li ?

Comment 22

[Matheus Longaray (:mlongaray)]

Attachment: WIP5: Remove list style changes

This patch removes list style changes at gijs' request. It also reverts body padding on simplifyMode.css.

Comment 23

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #21)
> (In reply to Matheus Longaray (:mlongaray) from comment #20)
> > It doesn't happen when printing from reader mode because we're setting an
> > specific padding to the document's body, so the list item number/symbol will
> > be showed on the PDF. See here:
> > http://searchfox.org/mozilla-central/source/toolkit/themes/shared/
> > aboutReader.css#6.
> > 
> > In this patch we're setting padding to 0 on simplifyMode.css, so it might
> > happen that the list item number be cut out when printing, e.g., list item
> > number with long digits. Should we also set an specific padding to
> > simplifyMode.css as well?
> 
> Can we just avoid setting padding to 0 for ul/ol/li ?

(In reply to Matheus Longaray (:mlongaray) from comment #22)
> Created attachment 8818299 [details] [diff] [review]
> WIP5: Remove list style changes
> 
> This patch removes list style changes at gijs' request. It also reverts body
> padding on simplifyMode.css.

So, now we still set padding: 0 on the ul/ol/li, right? So does stuff get cut off? Or is there still horizontal padding on body that prevents this?

Comment 24

[Matheus Longaray (:mlongaray)]

(In reply to :Gijs Kruitbosch from comment #23)
> So, now we still set padding: 0 on the ul/ol/li, right? So does stuff get
> cut off? Or is there still horizontal padding on body that prevents this?

Yes, we now set padding: 0 to get all elements aligned. And ul/ol/li elements won't get cut off due our default horizontal padding on body (defined on aboutReader.css) - same behavior for reader mode.

Comment 25

[:Gijs (he/him)]

OK, then I think we're good to go.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05f393387c37
Make the printing media section respect the font size. r=Gijs

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/05f393387c37