After chatting with @Nirzar and @ovasileva they've requested that I get these new print styles enabled on reading web staging. Heads up to @pmiazga I'll be pushing a patch to MobileFrontend to allow us to use MwContentProvider on desktop...

Change 367627 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Fix stylesheet

https://gerrit.wikimedia.org/r/367627

Feeling a bit overwhelmed and demoralised with all the moving parts here. Code review very much needed y'all.

I've updated staging with all the above patches and have asked @Nirzar to review http://reading-web-staging.wmflabs.org/wiki/Albert_Einstein and other pages - please feel free to do the same

If you find any bugs please list them in this etherpad: https://etherpad.wikimedia.org/p/DudeYourPrintStyles

I'll talk through the problems with @Nirzar sometime tomorrow.

Reviewing the reading web staging

Here are things that need fixing

• Use "Charis SIL", 'Georgia' , 'Times' , serif; as font stack wherever we use serif. this stack is used on Wikipedia.

Note: we will be just replacing Charis with Linux libertine. also this is dependent on T169828 but we would have rest of the stack same including Georgia.

• All tables, infoboxes, image captions, fig captions, UL, LIs should use Sans-Serif stack
• Only H1, H2s, p, blockquote would be using serifs
• No borders for images, thumbs, use mobilefrontend style
• The list items in footer sections, further reading / references / notes should be the same.

• I didn't mention the obvious breaking of H2s and infoboxes

I will keep testing and add stuff to this comment. but looks like we are pretty close :)

Change 367627 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Fix CSS selector output in print stylesheet

https://gerrit.wikimedia.org/r/367627

Change 367812 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Print mode: Define when to use serif and when to use sans-serif

https://gerrit.wikimedia.org/r/367812

Change 367814 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Print styles: Apply padding and margins to ol as well as ul

https://gerrit.wikimedia.org/r/367814

Change 367817 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Print styles: Border bottom of headings should not overlap infoboxes

https://gerrit.wikimedia.org/r/367817

Please help with code review. I have a bunch of follow ups in the description after a 2.5hr session with @Nirzar - he's happy with the styles - so please let me know what needs improving to get these all merged!

Change 367814 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Print styles: Apply padding and margins to ol as well as ul

https://gerrit.wikimedia.org/r/367814

Change 367812 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Print mode: Define when to use serif and when to use sans-serif

https://gerrit.wikimedia.org/r/367812

Change 366967 merged by jenkins-bot:
[mediawiki/core@master] Fix font size and backgrounds in gallery

https://gerrit.wikimedia.org/r/366967

The following patch has a +1. Any reason not to merge?:
https://gerrit.wikimedia.org/r/#/c/367817/

The following patches still need review:
https://gerrit.wikimedia.org/r/#/c/366738/
https://gerrit.wikimedia.org/r/#/c/366971/
https://gerrit.wikimedia.org/r/#/c/366968/
https://gerrit.wikimedia.org/r/#/c/366970/

Change 366738 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Print styles: footer

https://gerrit.wikimedia.org/r/366738

We would like to underline the <a> tags in print styles. it's better for accessibility as well.

Really ? There's a reason we don't underline to begin with, and that is because we have too many links to begin with. It creates a really 'busy' view, which generally is harder to read.

Why would you care if an article exists when you are reading it ? You can't click it (well in styled PDFs, but then we might as well keep the colors).

In T169823#3461827, @Nirzar wrote:

I think text should immediately follow a heading (as opposed to heading staying on the previous page):

I struggled with this a lot :( the page break: avoid property doesn't work at all :( :(
and we can't really use page break always because that adds million more pages

It would be amazing if we can actually solve this with css. The existing styles have this issue too

page-break-inside is for content inside elements, not for breaks between elements.

h1, h2, h3, h4, h5, h6 {
  page-break-after: avoid;
  break-after: avoid;
}

And that is actually already part of our print styles i believe But support seems... lacklustre...
Possibly because we also have: "page-break-before: avoid" on those elements, which doesn't really seem to make sense to me. Might be wise to modify that a bit (and to make sure we support the new break-after).

Change 367817 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Print styles: Border bottom of headings should not overlap infoboxes

https://gerrit.wikimedia.org/r/367817

also, staging has visible edit section links for me at this moment... ?

In T169823#3481108, @TheDJ wrote:

also, staging has visible edit section links for me at this moment... ?

Should be fixed by T171519 but I still need to verify :)

Change 366968 merged by jenkins-bot:
[mediawiki/core@master] Adjustments to print table styles

https://gerrit.wikimedia.org/r/366968

@Nirzar a question on the code review about this rule:

div.thumbinner { text-align: left; }

"sure about this ? This might mess with things like Template:Mutltiple_image which try to replicate thumbnails.. There isn't any text in this block normally. The only text should be thumb caption, which enforces it's own text-align specifically for this reason."

Can I safely remove?

Change 366971 merged by jenkins-bot:
[mediawiki/core@master] Adjustments to floating elements in print styles

https://gerrit.wikimedia.org/r/366971

Only https://gerrit.wikimedia.org/r/#/c/366970/ remains for review for which there's an open question for @Nirzar: https://phabricator.wikimedia.org/T169823#3487065

Once this is done we can then start reviewing these styles and working out what follows up are needed along with T172144.

@Nirzar a question on the code review about this rule:

can we add that rule to the thumbcaption then? I wanted captions for images, single or multiple to be left aligned with the image itself. right now the center align breaks the layout a bit.

can we add that rule to the thumbcaption then? I wanted captions for images, single or multiple to be left aligned with the image itself. right now the center align breaks the layout a bit.

It's already there. So yup I've retained it. Looks the same to me.

Anyone want to pull the trigger on https://gerrit.wikimedia.org/r/366971 so we can begin some in depth testing :)

https://gerrit.wikimedia.org/r/#/c/366971/ is already merged.

Change 366970 merged by jenkins-bot:
[mediawiki/core@master] Adjust print styles for thumb

https://gerrit.wikimedia.org/r/366970

@Nirzar @ovasileva over to you. All changes are merged and on staging. On staging we're still using production data and the feature flag is enabled.
You can test with these on: http://reading-web-staging.wmflabs.org/wiki/Albert%20Einstein
Note: there are 2 issues with testing on staging - table of contents will not display here and footer will display incorrectly.

If you want to see print styles with the feature flag displayed you can test on the beta cluster e.g. https://en.wikipedia.beta.wmflabs.org/wiki/Albert_Einstein - this will help you understand what's feature flagged and A/B testable and what's not.

Hope that makes sense?

Please raise any tasks for follow up work as a subtask of T154965 and we'll make sure those get worked on together.

I'm now going to see if I can get table of contents to show up in staging...

Okay.. table of contents is now showing. In case you developers watching at home were curious

switching

$wgMFMwApiContentProviderBaseUri = "https://en.m.wikipedia.org/w/api.php";

to

$wgMFMwApiContentProviderBaseUri = "https://en.wikipedia.org/w/api.php";
$wgMFRemovableClasses = [
        "base"=>[".navbox",".nomobile"],
        "beta"=>[]
];

did the trick.
Table of contents will be showing up in mobile view of staging so let me know when you are done testing so I can revert this and avoid confusing people.

In T169823#3487962, @Jdlrobson wrote:

If you want to see print styles with the feature flag displayed you can test on the beta cluster e.g. https://en.wikipedia.beta.wmflabs.org/wiki/Albert_Einstein - this will help you understand what's feature flagged and A/B testable and what's not.

Can you document what is and what isn't testable on T169731: Set up print styles a/b test on all projects, please? Knowing the limits up front will better inform the design of the test.

This skipped the Needs QA column. Is @Nirzar responsible for QA here?

@phuedx - we'll be QAing separately under T172184: QA of new desktop print styles

Are we and if so how are we dealing with overly long strings (especially URLs):

In T142207#3042907, @Tobi_WMDE_SW wrote:

e.g. if you want to print https://de.wikipedia.org/wiki/Benutzer:TMg/advancedSearch which includes some overly long strings, the line is not breaking and thus text is cut off. This should be easy to avoid by improving the stylesheet.

In T142207#3042976, @Jan_Dittrich wrote:

Two remarks:

1. It seems not to be easy. One could try with overflow-wrap: break-word; word-wrap: break-word; but they could also break in cases in which it is not essential.
2. It is sadly a common problem that also exists in LaTex etc.