Page MenuHomePhabricator

Regression: Some MobileFrontend styles are not applied on mobile view of GlobalUserPages
Closed, ResolvedPublic3 Story Points

Description

Visit https://m.mediawiki.org/wiki/User:CKoerner_(WMF)

Cannot replicate this on Jon's: Visit https://m.mediawiki.org/wiki/User:Jdlrobson

How it should look:

The problem seems to be that the following styles are not loaded:

..mw-ui-icon-mf-talk:before, mw-ui-icon-talk:before {}

.user-links li {
    display: inline-block;
}

mobile.userpage.icons and mobile.userpage.icons.styles appear to not be loaded on Chris's page.

Developer notes

The issue effects Global user pages only.
The problem is that ExtMobileFrontend ::domParse is called inside OutputPageBeforeHTML - a hook which is not invoked for global user pages.

There seems to be confusion about responsibility between Minerva and MobileFrontend here.
While, a user page that does not exist works is transformed for Mobile Minerva and[[ https://m.mediawiki.org/w/index.php?title=User:Rickathesperian&useskin=vector | Mobile Vector ]]

The header with the talk page icon and menu bar belong to Minerva:
(note Mobile Vector does not show this header)

Thus, the talk page icon and the rules relating to .user-links that currently live in the module mobile.userpage.styles should in fact belong to Minerva and the talk icon belonging to mobile.userpage.icons should actually live in a module that lives in Minerva.

This was broken in T182162

Fix

  • Restore modules skins.minerva.userpage.styles and skins.minerva.userpage.icons with the styles that should be shipped. This is done in https://gerrit.wikimedia.org/r/410364
  • Remove the talk page icon from the definition inside mobile.userpage.icons
  • Remove styles relating to user-links from the mobile.userpage.styles module.

QA steps

I've setup GlobalUserPages on reading web staging.

The navigation bar at the top of the page should appear horizontally

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 13 2018, 5:24 PM
ovasileva triaged this task as High priority.Feb 13 2018, 5:29 PM
ovasileva set the point value for this task to 5.Feb 13 2018, 5:53 PM
ovasileva lowered the priority of this task from High to Normal.
ovasileva added a subscriber: ovasileva.

As the estimates for this are currently quite high and we expect the issues to disappear when the cache is purged, we're dropping the priority on this for the moment.

Looks like this might be specific to GlobalUserPage's. I'm not sure how the cache strategy works there.

/me slowly backs away from Phabricator... 👀

What caching do you think is the problem here?

Legoktm renamed this task from Regression: Caching issue on Chris's user page (and probably others) to Regression: Some MobileFrontend styles are not applied on mobile view of GlobalUserPages.Feb 13 2018, 8:52 PM
Legoktm added a project: MobileFrontend.
Jdlrobson added a comment.EditedFeb 13 2018, 8:56 PM

This is the normal cached HTML/new CSS problem that we're hit with all the time as skin developers.

Looks like https://m.mediawiki.org/wiki/User:CKoerner_(WMF) is requesting the skins.minerva.icons.images module via the style tag in the HTML (https://m.mediawiki.org/w/load.php?debug=false&lang=en&modules=mediawiki.hlist%7Cmediawiki.ui.button%2Cicon%7Cskins.minerva.base.reset%2Cstyles%7Cskins.minerva.content.styles%7Cskins.minerva.icons.images%7Cskins.minerva.tablet.styles&only=styles&skin=minerva)
This module was replaced another module.

Problem is fixed trivially by the following JS:

mw.loader.using(['mobile.userpage.icons','mobile.userpage.styles'])

or flushing the Varnish cache for that page. It seems applying ?action=purge on this page or logging in does not serve the most recent HTML. I thought that should fix it, but it doesn't, which is strange. Given we've only seen this on a GlobalUserPage it would be worth understanding how the cache gets invalidated there as this appears to work differently from other user pages.

It's not a varnish cache issue, because https://m.mediawiki.org/wiki/User:CKoerner_(WMF)?1 exhibits the same behavior.

I purged the meta userpage that is the source of the globaluserpage, but that didn't change anything either...

According to https://phabricator.wikimedia.org/diffusion/SMIN/browse/master/includes/skins/SkinMinerva.php$1451 that module is still supposed to be added on all pages?

The only code path I see that loads mobile.userpage.icons is https://phabricator.wikimedia.org/diffusion/EMFR/browse/master/includes/MobileFrontend.body.php$75 - is that supposed to be the only place?

Jdlrobson added a comment.EditedFeb 13 2018, 9:23 PM

My bad skins.minerva.icons.images is something else.
So the problem here is mobile.userpage.icons is not being added to the global user page . Sounds like we can rule out caching being a problem.

The domParse function is invoked by hook onOutputPageBeforeHTML (https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/304c0b0923a263386c54f2a484b8a10a26cb094c/includes/MobileFrontend.hooks.php#L230) which modifies the html passed in by reference.

Is it possible the global user page somehow bypasses that?

(Side note and off topic the side effects of that domParse are extremely yucky)

My bad skins.minerva.icons.images is something else.
So the problem here is mobile.userpage.icons is not being added to the global user page . Sounds like we can rule out caching being a problem.

The domParse function is invoked by hook onOutputPageBeforeHTML (https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/304c0b0923a263386c54f2a484b8a10a26cb094c/includes/MobileFrontend.hooks.php#L230) which modifies the html passed in by reference.

Is it possible the global user page somehow bypasses that?

Yep, that hook is only called in OutputPage::addParserOutputText(), which GlobalUserPage skips.

Change 410364 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Restore Minerva specific userpage styles for all user pages

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

Jdlrobson updated the task description. (Show Details)
Jdlrobson removed the point value for this task.

Ahh. I'm removing the estimation as given this new information, the scope of the bug has changed and it doesn't look like it will magically fix itself. Have updated instructions about how to fix this. Thanks @Legoktm for helping me debug this.

ovasileva set the point value for this task to 3.Feb 14 2018, 5:09 PM

Change 410364 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Restore Minerva specific userpage styles for all user pages

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

Change 410564 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Don't ship unnecessary styles to mobile userpages that are not Minerva

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

Change 411014 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Remove unused userpage icon from repo

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

Change 411014 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Remove unused userpage icon from repo

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

Change 410564 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Don't ship unnecessary styles to mobile userpages that are not Minerva

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

This is going to be difficult to test but let me see if I can work something out. ..

Jdlrobson assigned this task to ABorbaWMF.

Over you Anthony. This can be tested now!

Looks ok to me. Here are some screenshots

phuedx reassigned this task from ABorbaWMF to ovasileva.Feb 16 2018, 10:10 AM
phuedx added a subscriber: ABorbaWMF.
ovasileva updated the task description. (Show Details)Feb 16 2018, 10:54 AM
ovasileva closed this task as Resolved.

looks like we're all done here!