Page MenuHomePhabricator

Remove $wgVectorExperimentalPrintStyles from Vector codebase
Closed, ResolvedPublic3 Estimated Story Points

Assigned To
Authored By
Jdlrobson
Oct 12 2017, 12:03 AM
Referenced Files
F10682016: image.png
Nov 8 2017, 7:34 PM
F10681996: image.png
Nov 8 2017, 7:34 PM
F10682000: image.png
Nov 8 2017, 7:34 PM
F10681990: image.png
Nov 8 2017, 7:34 PM
F10682006: image.png
Nov 8 2017, 7:34 PM
F10682013: image.png
Nov 8 2017, 7:34 PM
F10682010: image.png
Nov 8 2017, 7:34 PM
F10665111: image.png
Nov 7 2017, 7:36 PM

Description

New print styles were deployed on October 11th.
We should give these a few weeks to debug any problems and then remove the feature flag

Acceptance criteria

  • Remove $wgVectorExperimentalPrintStyles from the Vector codebase
  • Merge the vector-experimental-print-styles class and styles into the skins.vector.styles module
  • Remove config instances relating to $wgVectorExperimentalPrintStyles in Vector codebase

QA steps

Time sensitive!
This will roll out on the train from Tuesday-Thursday, so we need to verify this quickly.
QA should be performed on the beta cluster (https://en.wikipedia.beta.wmflabs.org/wiki/Test_previews) AND https://reading-web-staging.wmflabs.org

  • Verify that the printed articles on Vector skin look good (existing print styles are still applied).
  • Verify that skin Vector has no strange styling artifacts in non-print mode

Sign off steps

  • Remove the config change in mediawiki config

Event Timeline

@Jdlrobson - This would remove the old styles completely right? We might still want to run some comparisons on the old vs new styles in terms of page length.

Discussion in standup: we're not sure if this is just a config change or a config change + replace old styles.

This is technical debt we have introduced and our responsibility to remove in a timely manner. I've updated what that means in the description since there seems to be a little confusion.

With respect to comparing old vs new we can still do this if we remove the styles from the codebase (git has history). We don't need them to be in the codebase to do that.

Change 388133 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Print styles in Vector are no longer feature flagged

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

Change 388133 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Print styles in Vector are no longer feature flagged

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

Jdlrobson updated the task description. (Show Details)

Over to you @ABorbaWMF . This one is time sensitive!

I was able to look at this a bit. So far I have not found any major problems. I did notice some kerning issues with references that you can see in the screenshots below.

From https://en.wikipedia.beta.wmflabs.org/wiki/Carnivore
Mac - Safari

image.png (768×1 px, 252 KB)

Mac - Chrome
image.png (768×1 px, 368 KB)

Mac - Firefox
image.png (768×1 px, 252 KB)

Windows - Firefox
image.png (768×1 px, 458 KB)

Windows - Edge
image.png (768×1 px, 466 KB)

I will add some feedback from staging as well.

I did notice some kerning issues with references that you can see in the screenshots below.

the only one i can sort of see is mac + firefox

image.png (846×876 px, 163 KB)

but this also seems like spacing issues than kerning issue. expected due to justify alignment

@ABorbaWMF can you confirm?

@ABorbaWMF can you confirm that when generating these pdfs you used the browser print menu item or did you use the "download as pdf" link in the left menu?

I am seeing some odd behavior on staging that is not present on beta:

Broken on Win 10 Edge & IE 11Works on Beta
https://reading-web-staging.wmflabs.org/wiki/Vladimir_Leninhttps://en.wikipedia.beta.wmflabs.org/wiki/Dog
Win 10 - Edge 16
image.png (768×1 px, 103 KB)
image.png (768×1 px, 244 KB)
Win 10 - IE 11
image.png (768×1 px, 67 KB)
image.png (768×1 px, 244 KB)

But these seem to fine on Staging
Win 10 - Chrome 62

image.png (768×1 px, 268 KB)

Mac 10 - Firefox
image.png (768×1 px, 275 KB)

The problems on Staging may be due to the tool I am using (CrossBrowserTesting). I have not had a chance to try it on a real windows system yet.

@Jdlrobson - I used the article controls to generate the PDFs. Should I also try the browser print function?

@ABorbaWMF yes if you could please always generate the PDFs via the printer (unless we specify otherwise) we'll avoid surfacing some already known issues, the "Download as PDF" button has a variety of issues including this one (T178665).

Ok, no problem. I reran some of the tests and the results are coming out the same for me. Everything is working except for Windows 10 with IE and Edge, but only on Staging.

Thanks @ABorbaWMF !
Provided they work fine on beta cluster, I wouldn't worry too much about staging. It's possible the proxying we are doing there is causing unexpected problems.

@ABorbaWMF - just to clarify, we you still experiencing the same kerning issues on the Carnivore article?

@ovasileva, It looks good on that article:

Windows 10 - Chrome 62

image.png (768×1 px, 386 KB)

Windows 10 - Firefox 56
image.png (768×1 px, 287 KB)

Windows 10 - IE 11
image.png (768×1 px, 281 KB)

Windows 10 - Edge
image.png (768×1 px, 276 KB)

Mac 10 - Safari 11
image.png (768×1 px, 267 KB)

Mac 10 - Firefox 56
image.png (768×1 px, 246 KB)

Mac 10 - Opera 48
image.png (768×1 px, 281 KB)

Legoktm added a subscriber: Legoktm.

This was reverted, as it broke the MediaWiki installer. I don't think having the skins.vector.styles module use the Vector\\ResourceLoaderLessModule class is going to work for the installer. None of the Vector PHP code is ever loaded, just the module definition.

Jdlrobson raised the priority of this task from Medium to Unbreak Now!.Dec 27 2017, 2:40 AM
Jdlrobson added subscribers: phuedx, greg.

Having reverted this, we have broken the styles for printed pages for all page views (the new print styles are disabled after the patch and there is no corresponding config).

Re-reverting this and fixing this and T183640 should now be considered a deployment blocker cc @greg @phuedx

Just to clarify, the revert isn't deployed yet so nothing is actually broken until whenever the train rolls out, correct? (Aside from beta cluster)

Change 400389 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/skins/Vector@master] Revert "Revert "Print styles in Vector are no longer feature flagged""

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

Just to clarify, the revert isn't deployed yet so nothing is actually broken until whenever the train rolls out, correct? (Aside from beta cluster)

AFAICT the print styles won't be misconfigured/disabled in production if/when the revert rides the train.

[[ https://phabricator.wikimedia.org/source/mediawiki-config/browse/master/wmf-config/InitialiseSettings.php;fc6a15058908fa135c5deb230fcadd0c66ae7892$7213 | The wgVectorExperimentalPrintStyles variable is still true by default in the config ]], meaning that both the Beta Cluster and production are configured correctly (fortunately).

While I think that rSVEC3bae18859fd2 does fix the issue and the revert revert can be merged, I don't think that we'll see a regression in January if we don't act immediately. Have I missed something @Jdlrobson?

The original revert was messy and I'm not really sure if it does what it says it does. So I think you should either carefully verify the current behavior, or review the followups, before the next train.

Change 400389 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Revert "Revert "Print styles in Vector are no longer feature flagged""

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

Thanks all for getting this fixed!