Page MenuHomePhabricator

Improper CSS being served to apps.
Closed, ResolvedPublic1 Story PointsBUG REPORT

Description

The Base CSS endpoint seems to no longer return all the necessary CSS that's used by the app.
https://en.wikipedia.org/api/rest_v1/data/css/mobile/base

The following output from the endpoint is indicative of the breakage:

/*
Problematic modules: {"skins.minerva.base.reset":"missing"}
*/

This is resulting in various incorrect styles applied to our article presentation, including but not limited to:

Details

Related Gerrit Patches:
mediawiki/services/mobileapps : masterCSS hotfix for Android
mediawiki/services/mobileapps : masterDrop nonexistent skins.minerva.base.reset, use skins.minerva.base.styles

Event Timeline

Dbrant triaged this task as High priority.Jan 25 2019, 5:54 PM
Dbrant created this task.
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 486496 had a related patch set uploaded (by BearND; owner: Mholloway):
[mediawiki/services/mobileapps@master] Drop nonexistent skins.minerva.base.reset, use skins.minerva.base.styles

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

Change 486496 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] Drop nonexistent skins.minerva.base.reset, use skins.minerva.base.styles

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

Mentioned in SAL (#wikimedia-operations) [2019-01-25T18:28:43Z] <bsitzmann@deploy1001> Started deploy [mobileapps/deploy@94b76f5]: Update mobileapps to 4c42e3d (T214714)

Mentioned in SAL (#wikimedia-operations) [2019-01-25T18:32:16Z] <bsitzmann@deploy1001> Finished deploy [mobileapps/deploy@94b76f5]: Update mobileapps to 4c42e3d (T214714) (duration: 03m 33s)

Jdlrobson added a subscriber: Jdlrobson.EditedJan 25 2019, 10:31 PM

I flagged during code review that you may also want to import your own css reset file rather than using skins.minerva.base.styles as potentially that will have other unwanted styles. E.g. https://www.npmjs.com/package/reset-css

Long term it would be better if apps could used versioned npm modules rather than Minervas live styles. I cant guarantee this won't break again in the forseeable future - resource loader modules are not intended for use in this way like an API. Maybe something we can talk about next week.

Long term it would be better if apps could used versioned npm modules rather than Minervas live styles. I cant guarantee this won't break again in the forseeable future - resource loader modules are not intended for use in this way like an API. Maybe something we can talk about next week.

Agreed on thinking this through next week—we need to find some sane solution for this.

Change 487908 had a related patch set uploaded (by BearND; owner: BearND):
[mediawiki/services/mobileapps@master] CSS hotfix for Android

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

Change 487908 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] CSS hotfix for Android

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

Mentioned in SAL (#wikimedia-operations) [2019-02-05T18:21:50Z] <bsitzmann@deploy1001> Started deploy [mobileapps/deploy@2959e12]: Update mobileapps to 107c1b1 (T214714)

Mentioned in SAL (#wikimedia-operations) [2019-02-05T18:26:32Z] <bsitzmann@deploy1001> Finished deploy [mobileapps/deploy@2959e12]: Update mobileapps to 107c1b1 (T214714) (duration: 04m 43s)

@bearND Thanks for the fix. I have one question about the future of the CSS endpoints:
Now that we've updated our index.html to pull the CSS from all the correct endpoints (i.e. get the Base and PageLib css from metawiki), supposing that we'll deploy this update soon, what is the roadmap for *supporting* these endpoints in the long term? The reason I ask is that our next update will likely be the last one that supports API 19 (KitKat), and I'd like to make sure that all the endpoints that it's using will have sufficiently long-term support, so that we can let it go in peace and not worry about back-porting hotfixes.

Besides @Dbrant's questions, I want to keep this open until we at least know how we are going to deal with this issue in the future.

It seems from this comment (emphasis mine):

Long term it would be better if apps could used versioned npm modules rather than Minervas live styles. I cant guarantee this won't break again in the forseeable future - resource loader modules are not intended for use in this way like an API. Maybe something we can talk about next week.

That reading web won't be keeping in mind apps when making changes to Minerva. This isn't a decision that can be made lightly on a comment, and it needs discussion.

The reality is that apps depend directly or indirectly on minerva and mobilefrontend in different ways for now and have done so for a while, so at the very least the team needs to be responsible when making certain changes. It is the same as when core changes and breaks mobile frontend or minerva, it is never fun.

I think we need to meet and discuss this in depth, because it may not make sense to split styling off given that parsoid html will be coming at some point and apps have the styles ready for it. In any case, if we decide to fork minerva's styles and finally separate apps completely from web (once ios migrates to all the new services), it needs to be an explicit call with assurances.

Thoughts @phuedx @JoeWalsh @Dbrant @Jdlrobson @Niedzielski @bearND ?

Jdlrobson added a comment.EditedFeb 14 2019, 11:15 PM

The bit that is still confusing me is why apps need to load Minerva's styles. Most the styles of content are found inside the HTML now thanks to TemplateStyles or inside mobile.site.styles

Loading Minerva's styles will give you a bunch of unnecessary styles that style the UI.

Styles for font changes and link colors etc while useful could be copied into apps and give apps control over their experience. I pointed out that recently Minerva changed its link color and that would have impacted apps without their consent. FWIW the mobile skin used to share styles with desktop but moved away from doing that for very similar problems. Surely apps would like more control of these basic styles to avoid issues like this?

If not, Minerva is converging on having a single RL module at the request of the performance team to minimise RL modules so if we do need to continue to support this it is certainly doable but will be necessary to adapt the loading and/or improve the optimisation. I can fold that requirement into the tasks that make that change.

I think we need to meet and discuss this in depth,

+1 to that.

To be clear, the only things used were the reset and the content styles, which given minerva is the wikimedia mobile web skin, and apps are mobile platforms, makes a lot of sense.

For reference, before this whole thing happened: https://github.com/wikimedia/mediawiki-services-mobileapps/blob/f78bd20cd123617a5088d2b967389fccf5920214/lib/css.js#L16-L17

Given the unreliability of the current dependency and the lack of accountability as things broke, the most sensible solution will most probably be forking off styles from before the refactor and embedding them either on the mobileapps extension or on page library, for the apps to rely on via the REST URLs.

I personally do think that we should work together where possible, to benefit from shared efforts, rather than further isolate ourselves from each other.

I'll talk to @JoeWalsh and @phuedx to schedule something. Comments welcome here meanwhile.

Jhernandez added a subscriber: Mholloway.

It was informal but there was some discussion on the patch too: https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/483650/.

Regardless, working together better would be very welcome by me In the short term, I'll try to be practical and more mindful of our exposed APIs when making changes. In the long term, I hope we can find more opportunities to converge our efforts. If there are our other ways I can better support RI and apps, let me know.

Jdlrobson added a comment.EditedFeb 19 2019, 4:28 PM

personally do think that we should work together where possible, to benefit from shared efforts, rather than further isolate ourselves from each other.

Definitely but I honestly and strongly believe this shouldnt be done on the CSS level. Something like sharing definitions in https://github.com/wikimedia/wikimedia-ui-base would be a lot better.

FWIW the fact content styles live in Minerva make little sense to me since the content is defined in templates and core parser.

@Jdlrobson Making the apps' CSS dependent on what parser was used to generate the HTML makes sense to me. So, I guess for the iOS app they are better off using what they have right now until they switch to mobile-html. For the Android app since it's using HTML generated by Parsoid (mobile-sections, later mobile-html) we probably should make sure that the https://en.wikipedia.org/api/rest_v1/data/css/mobile/base CSS output works well at least with Parsoid generated HTML. Having said that, unfortunately we can't drop the legacy parser based CSS since the Android app still uses legacy parser output for some fallback cases (e.g. Chinese language variants).

wikimedia-ui-base sounds interesting. I'd like to learn more about that.

  • Is this for content emitted via the legacy parser or Parsoid?
  • What is the RL module which exposes this?

wikimedia-ui-base sounds interesting. I'd like to learn more about that.

It's not a RL loader module but an npm module that defines variables (mostly colors right now). Long term we're hoping to ingest that in Minerva and various other places. It's interesting as it allows us to share common colors/fonts without forcing an implementation or CSS paradigm on you so in apps it might be

body { font-family: @wikimedia-ui-default-font-family-mobile; }
`

Parsoid content is styled in https://en.wikipedia.org/w/load.php?debug=false&lang=en&skin=minerva&target=mobile&only=styles&modules=mediawiki.skinning.content.parsoid
You can safely use/add to that without worrying about the same issues as using a Minerva style module, as it's very specific and scoped to styling Parsoid HTML.

Meeting has been set with an agenda.

Thanks for collaborating on this.

Jhernandez closed this task as Resolved.Feb 26 2019, 6:38 PM