Page MenuHomePhabricator

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

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.

Jdlrobson updated the task description. (Show Details)Oct 31 2017, 4:25 PM

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.

Jdlrobson updated the task description. (Show Details)Nov 1 2017, 5:34 PM
MBinder_WMF set the point value for this task to 3.

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

Jdlrobson claimed this task.Nov 2 2017, 6:06 PM

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

pmiazga updated the task description. (Show Details)Nov 3 2017, 11:36 PM
Jdlrobson reassigned this task from Jdlrobson to ABorbaWMF.Nov 6 2017, 11:39 PM
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


Mac - Chrome

Mac - Firefox

Windows - Firefox

Windows - Edge

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

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
Win 10 - IE 11

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


Mac 10 - Firefox

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


Windows 10 - Firefox 56

Windows 10 - IE 11

Windows 10 - Edge

Mac 10 - Safari 11

Mac 10 - Firefox 56

Mac 10 - Opera 48

ovasileva closed this task as Resolved.Nov 9 2017, 10:14 AM

looks good, thanks @ABorbaWMF

Legoktm reopened this task as Open.Dec 24 2017, 7:38 PM
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

Restricted Application added subscribers: Liuxinyu970226, Jay8g, TerraCodes. · View Herald TranscriptDec 27 2017, 2:40 AM

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.

Thanks for the heads up, @matmarex.

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

Paladox closed this task as Resolved.Dec 29 2017, 11:08 PM
Paladox removed a project: Patch-For-Review.

Thanks all for getting this fixed!