Page MenuHomePhabricator

Fix the printable versions of modern Vector
Closed, ResolvedPublic5 Estimated Story PointsBUG REPORT

Description

Description

Compare

It would seem that this is because the legacy version hides the whole sidebar, of which the logo is a part. The new version places the logo in the header.mw-header element, which isn't hidden.

This issue impacts:
  1. printing wiki pages (without ?printable=yes)
  2. ?printable=yes (to be removed, apparently) and
  3. Special:ElectronPdf (download as pdf)
  4. ...?
Two print stylesheets
  1. In core/resources/src/mediawiki.skinning/commonPrint.css
  2. In Vector/resources/skins.vector.styles/common/print.less

Special:ElectronPdf uses the same styles as Ctrl+P

Acceptance criteria

  • CTRL+print and check the output looks like the mock
  • Check the proton PDF output looks like the mock

Mock


Development notes

Display: flex should suffice here.

QA Results - Beta

ACStatusDetails
1T253842#6360364
2T253842#6360364

QA Results - Prod

ACStatusDetails
1T253842#6370911
2T253842#6370911

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 599357 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/Vector@master] [modern] Hide header when printing

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

Demian added a subscriber: Demian.May 28 2020, 3:59 PM

@Mainframe98 Thank you for the report!

Demian claimed this task.May 28 2020, 3:59 PM

Thanks!
@alexhollender I'm not sure we want to hide this though? Isn't it important to have some kind of branding on the printed page? We could have this at the bottom using display: block

Demian added a comment.EditedMay 29 2020, 1:19 AM

DEMO for testing hidden logo:

Logo was hidden previously, for a decade+. Still, it can added to print, but that will be more complicated and raises many questions. A few that comes to mind:

  1. Add the logo to each page / first page only / last page only?
  2. Where to add the logo: top / bottom?
  3. Where to add the logo on the last page: (top) / bottom of page / end of content?
ovasileva triaged this task as Medium priority.Jun 1 2020, 8:52 AM
ovasileva added a subscriber: ovasileva.

Did we make a decision on this? Tested today and can see the logo at the bottom of the page as discussed above

@Jdlrobson apologies on the delay here. Can you clarify what is being discussed? I'm not entirely sure what I'm looking at with those links in the description. Also where can I see what is being proposed?

Are we talking about how a print/PDF version of an article renders?

@alexhollender @ovasileva the issue Aron is pointing impacts the printable=yes link (which is I believe is pretty low priority):


but the many printed forms of Vector (including ElectronPdf) also have problems with the new header. If you test the beta cluster with download as pdf for example the branding is at the bottom of the page:

This would be a good time to document how this should behave for all these printable modes.

@alexhollender @ovasileva the issue Aron is pointing impacts the printable=yes link (which is I believe is pretty low priority)

+1 on the low priority for this one - I thought we were just referring to printable version and download as PDF, both of which are now showing branding at the bottom. For the latter two, I think we should consider them a regression and try to put them back at the top although with medium priority. Will defer to @alexhollender for other ways we can highlight branding though, maybe we have different opportunities here as well...

Cool sounds good. Note the printable mode is also marked for removal - T167956 - we might want to accelerate that as part of our changes to avoid further confusion.

Demian added a comment.Jun 3 2020, 5:08 AM

@alexhollender @ovasileva the issue Aron is pointing impacts the printable=yes link (which is I believe is pretty low priority):

Sidenote: Just fixing... Mainframe98 reported it.

This impacts:

  1. printing wiki pages (without ?printable=yes)
  2. ?printable=yes (to be removed, apparently) and
  3. Special:ElectronPdf (download as pdf)
  4. ...?

With the decision to show the logo in print from now on, the task becomes:

  • Hiding elements in the header except the logo (soon there will be other elements too in the header).
  • Positioning the logo in print.

One concern is:
?printable=yes uses only commonPrint.css from core, but not the print.less stylesheet from Vector (or the active skin). This will be removed, but what about ElectronPdf? There is no general approach to apply the skin's print stylesheet, so I assume that's also using just the core stylesheet.
It seems to me, the rules to be introduced to solve this have to be added to core, or services like ElectronPdf has to load the skin's print stylesheet too.

Demian updated the task description. (Show Details)Jun 3 2020, 5:16 AM
Demian added a comment.Jun 3 2020, 5:21 AM

try to put them back at the top although with medium priority.

The logo was never at the top, or visible at all in print.
There are 3 questions to answer for properly showing the logo in print. I suggest doing that in a separate task, not a bug report.
That's not high priority and it will take some time, therefore in the meantime I suggest merging the most simple fix for this bug as a temporary, immediate solution. When the task to properly show the logo in print is solved, it will remove this temp fix.

Demian added a comment.Jun 3 2020, 7:37 PM

Thanks for the pointer, that explains why you prefer to show a logo.
However, that code adds an extra wordmark, not a logo, it's different from what I was discussing from my perspective.

I haven't seen that wordmark in any of the reported use-cases: neither ElectronPdf or printable=yes is affected by it, understandably.
The wordmark was removed from modern when the Logo was added. I see you refer to the wordmark as a predecessor of the logo in this case.
This was never discussed in the 6 weeks of creating the Logo patch.

Do you agree with the below conclusion?

  • modern - print: Logo should be visible at the top of the first page, content positioned accordingly.
  • ElectronPdf and printable=yes: Logo should not be visible.
Jdlrobson reassigned this task from Demian to ovasileva.Jun 5 2020, 6:14 PM
Jdlrobson moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.

Change 599357 abandoned by Aron Manning:
[modern] Hide header when printing

Reason:
"The bug/task is a reminder this is a problem." -- JdlRobson

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

Change 599357 restored by Aron Manning:
[modern] Hide header when printing

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

Demian added a comment.EditedJun 15 2020, 12:53 AM

https://en.wikipedia.beta.wmflabs.org/wiki/Main_Page?printable=yes


is terrible, therefore I've uploaded a new patch that restores legacy's print layout (including the logo).

@ovasileva is the product owner and she has said this is low priority for now. It's terrible yes, but we will get to it at some point and with design input we'll fix this properly.

@ovasileva I do advise we get this fixed before deploying to production wikis as print is obviously broken to end-users. Should this be made a subtask of one of the deployment tasks?

ovasileva added a comment.EditedJun 16 2020, 10:04 AM

@ovasileva is the product owner and she has said this is low priority for now. It's terrible yes, but we will get to it at some point and with design input we'll fix this properly.

@ovasileva I do advise we get this fixed before deploying to production wikis as print is obviously broken to end-users. Should this be made a subtask of one of the deployment tasks?

Sorry - I've had a to-do to split these up for a little while now:

  • medium priority: PDFs and browser print. Do we know
  • low priority: ?printable=yes

@Jdlrobson - it seems these would be two separate fixes right?

@ovasileva is the product owner and she has said this is low priority for now. It's terrible yes, but we will get to it at some point and with design input we'll fix this properly.

I've already fixed it, so you don't need to work on it and can focus on the high priority tasks, which I've also implemented weeks/months ago. As such the priority is not an issue, however, not benefiting from volunteer contributions is.

The fix I've submitted is temporary using the legacy solution, for the reason that a final fix with the modern logo requires the stabilization of the DOM, which is very unstable atm because of the constraints of T246420 and T246419. That could take weeks (the patch has been worked on for 2+ weeks now).

  • medium priority: PDFs and browser print. Do we know
  • low priority: ?printable=yes

@Jdlrobson - it seems these would be two separate fixes right?

The current fix does all 3.
PDFs and ?printable=yes depend on the print style in core, those are the same fix and also fix "browser print".
"browser print", however, can be a separate fix as it depends on the print style in Vector. Doing that would not fix PDFs and ?printable=yes

This now also shows the sidebar imposed over the text, making the whole thing even more useless than before.
Hiding the whole header .mw-header fixes both this task and the imposed sidebar.

ovasileva raised the priority of this task from Medium to High.Jun 29 2020, 6:19 PM
Jdlrobson renamed this task from Printable version has the logo and the H1 superimposed on top of eachother to Fix the printable versions of modern Vector.Jun 29 2020, 6:23 PM
Jdlrobson updated the task description. (Show Details)
ovasileva updated the task description. (Show Details)Jun 30 2020, 10:08 AM
ovasileva updated the task description. (Show Details)

Suggested fix (as print style):

#mw-panel,
#mw-sidebar-button { display: none;}

main { margin-top: @header-height; }
Jdlrobson updated the task description. (Show Details)Jul 1 2020, 5:27 PM
Demian added a comment.Jul 1 2020, 5:39 PM
  • Check the printable=yes output looks like the mock
  • Check the proton PDF output looks like the mock
  • Those need styles in core. These styles are however specific to Vector.
  • SearchBox will be visible after it's moved to the header.
  • printable=yes's margin: 0 !important; takes precedence
Jdlrobson updated the task description. (Show Details)Jul 7 2020, 8:10 PM

Change 610155 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Fix print styles on Ctrl+p and ElectronPdf

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

After talking to Olga and given T167956, fixing printable=yes version is not something we're going to work on. Main focus is on fixing the other printed versions more commonly used (ElectronPdf and browser).

Having looked into it, when we come to estimate I want to point out that this task will likely resurface discussions around keeping print.less separate and how global variables should work as this requires changes to print LAYOUT. Currently there is no home for these. Flex box seems the easiest way to solve this - we can use feature queries to provide a suitable fallback for non flex box browsers. All things to talk about during estimation.

Jdlrobson set the point value for this task to 5.Jul 14 2020, 5:35 PM

@Jdlrobson Following your task description, we're following the browser support matrix and provide -ms-flexbox fallbacks to IE 9+…!

ovasileva removed ovasileva as the assignee of this task.Jul 20 2020, 5:20 PM

Change 615807 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/QuickSurveys@master] Hide quick surveys when printing

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

Change 615808 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Restrict image border to images in border

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

Change 615807 merged by jenkins-bot:
[mediawiki/extensions/QuickSurveys@master] Hide quick surveys when printing

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

Change 615808 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Restrict image border to images in border

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

Change 615809 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Collapse media print query

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

Change 617208 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Print: Logo styles should apply in printed media

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

Change 617209 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] (alternative) [legacy] Print: Add layout print styles on Ctrl+p and ElectronPdf

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

Change 617208 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Print: Logo styles should apply in printed media

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

Change 615809 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Collapse media print query

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

Change 599357 abandoned by Jdlrobson:
[mediawiki/skins/Vector@master] [modern] Hide header when printing, show old logo instead

Reason:
Fixed by https://gerrit.wikimedia.org/r/617208 and https://gerrit.wikimedia.org/r/615808.

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

Change 610155 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Print: Add layout print styles on Ctrl+p and ElectronPdf

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

Change 617209 abandoned by Jdlrobson:
[mediawiki/skins/Vector@master] [legacy+modern] Print: Add layout print styles for modern

Reason:
See https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/ /610155

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

Jdlrobson reassigned this task from Jdrewniak to Edtadros.Aug 3 2020, 9:35 PM
Jdlrobson added a subscriber: Jdrewniak.

Test Result - Beta

Status: ✅ PASS
Environment: beta
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA steps

✅ AC1: CTRL+print and check the output looks like the mock

✅ AC2: Check the proton PDF output looks like the mock

Edtadros updated the task description. (Show Details)Aug 4 2020, 4:40 PM

Test Result - Prod

Status: ✅ PASS
Environment: enwiki
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA steps

✅ AC1: CTRL+print and check the output looks like the mock

✅ AC2: Check the proton PDF output looks like the mock

Edtadros reassigned this task from Edtadros to ovasileva.Aug 8 2020, 6:46 PM
Edtadros updated the task description. (Show Details)
Edtadros added a subscriber: Edtadros.
ovasileva closed this task as Resolved.Aug 10 2020, 9:35 AM

Looks good!