Page MenuHomePhabricator

Decide on a strategy for incorporating site-specific mobile CSS in the apps
Closed, ResolvedPublic

Description

Wikis using the MobileFrontend and/or MobileApp extensions (including all Wikimedia wikis) can define mobile CSS on the MediaWiki:Mobile.css page. To date, the apps have not incorporated up-to-date, site-specific mobile CSS. In commit 0db57d7 a copy of the English Wikipedia's Mobile.css was copied into the MobileApp repo as enwiki.less, and it remains there largely unchanged to this date, and that among other things is being bundled into the apps.

A wiki's MediaWiki:Mobile.css page is exposed via the MobileFrontend extension as the module mobile.site. The apps should be updated to take advantage of this. There are a couple of ways we could go about this:

  • If the apps want to move toward a network-first CSS loading strategy, they could link the site-specific mobile CSS directly in the page view index.html by adding <link rel="stylesheet" href="/w/load.php?only=styles&modules=mobile.site">. This seems to me to be the best strategy.
  • If the apps instead prefer to continue following a bundling strategy, then the site-specific CSS could be exposed as a new MCS endpoint to accompany the universal app CSS provided by the base CSS endpoint. In this case the apps could bundle a small additional stylesheet for each site, and dynamically change the one referenced from index.html at runtime when loading a page.

In either case we'll want to get rid of the existing enwiki.less. A patch to do that is here. After that is removed and before a further strategy is decided upon, the apps can continue bundling the (up-to-date) enwiki styles by requesting bundled styles specifically from en.wikipedia.org and adding mobile.site to the list of modules requested.

Event Timeline

Mholloway updated the task description. (Show Details)
Mholloway updated the task description. (Show Details)
Mholloway updated the task description. (Show Details)Mar 19 2018, 8:57 PM

Change 420375 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/extensions/MobileApp@master] Remove enwiki.less

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

Mholloway renamed this task from Decide a strategy for incorporating site-specific mobile CSS in the apps to Decide on a strategy for incorporating site-specific mobile CSS in the apps.Mar 19 2018, 8:58 PM
Tgr added a comment.Mar 19 2018, 10:11 PM

A wiki's MediaWiki:Mobile.css page is exposed via the MobileApp extension as the module mobile.app.site. The apps should be updated to take advantage of this. There are a couple of ways we could go about this:

  • If the apps want to move toward a network-first CSS loading strategy, they could link the site-specific mobile CSS directly in the page view index.html by adding <link rel="stylesheet" href="/w/load.php?only=styles&modules=mobile.app.site">. This seems to me to be the best strategy.

That's already available via w/load.php?only=styles&modules=mobile.site. If mobile.app.site duplicates that, it should probably be removed (as the number of ResourceLoader modules affects page size).

My concern with mobile.site from MobileFrontend is that according to https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/extension.json it declares a dependency on mobile.startup, which looks like a lot of web-specific stuff that the apps don't need or want.

Tgr added a comment.Mar 19 2018, 10:21 PM

Does that have any effect with only=styles though? I think dependencies have been implemented fully on the JS side.
(Related: T190083: Make MediaWiki:Mobile.css render-blocking which will split the module.)

Ah, okay. We're about at the limits of my current knowledge about ResourceLoader. If that's so, then plain mobile.site is probably the way to go (and we could probably take the code redundantly exposing mobile.app.site out of the MobileApp extension).

Mholloway updated the task description. (Show Details)Mar 19 2018, 10:31 PM
Tgr added a comment.Mar 19 2018, 10:49 PM

I don't claim to understand how exactly dependency resolution works in RL, but the two URLs do give identical output.

Out of curiosity, could we in theory start a MediaWiki:MobileApp.css on any wiki that needed some special styling for apps but not mobile web, expose it as mobile.app.site via MobileAppResourceLoaderModule.php, and have it just work? Or would that take some additional configuration (or coding)? I can't find the notion of site-wide CSS beyond Common.css and Mobile.css addressed in the docs.

Tgr added a comment.Mar 21 2018, 1:43 PM

Yeah, you can just declare it as another wikipage module (example). Or a subclass like MobileAppResourceLoaderModule although as far as I see that class doesn't really do anything useful, it just hardcodes the page name.

Change 422053 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[apps/android/wikipedia@master] Update handling of mobile site CSS

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

Dbrant added a comment.Apr 4 2018, 1:25 AM

Generally I'm in favor of moving away from bundling CSS in the apps altogether, or perhaps go back to the apps' old original strategy of bundling a fallback CSS collection, but then periodically refreshing it from the live site in the field. This could apply to both site-specific and even general (minerva) CSS.

Here's a sketch of how we could do this relatively easily using our latest WebView+OkHttp caching setup:

  • Modify the index.html that we load into the WebView to load CSS from the live site (base CSS as well as site-specific CSS (mobile.site), but not page-library CSS).
  • Our custom WebViewClient will intercept the CSS request and cache the response aggressively, perhaps even saving it to the secondary "offline" cache. The next time the WebView requests the same CSS content from the server, I would expect it to return HTTP 304 (when online) so that we can pull the currently cached CSS, or if it returns HTTP 200 we'll overwrite the cache.
  • If the app is offline *and* the cached CSS is somehow missing or unavailable (e.g. if the user goes offline, clears the app cache, then opens the app and loads a reading list article), we fall back to locally-bundled CSS. And even for this edge case, it's probably only necessary to bundle the base CSS, and keep site-specific CSS as part of the normal OkHttp cache.
RHo added a subscriber: RHo.Apr 4 2018, 4:06 PM

@Dbrant And just to be explicit about this, the paths would be relative to the page's base href which is set dynamically on page load, so that you would end up getting the correct, site-specific mobile.site CSS for each page regardless of site. (The downside is that you'd also end up potentially caching multiple copies of the bulk of the CSS which doesn't vary by site, one copy for each site it's requested from, though this response is modest enough in size (roughly 15-25KB) as to be acceptable IMO.) Does that sound correct?

In that case, it sounds like we'll only need to expose a single PCS endpoint for the CSS bundle.

@Mhurd does all of this sound good to you?

Dbrant added a comment.Apr 4 2018, 5:31 PM

@Dbrant And just to be explicit about this, the paths would be relative to the page's base href which is set dynamically on page load, so that you would end up getting the correct, site-specific mobile.site CSS for each page regardless of site.

Wouldn't we just need to add <link rel="stylesheet" href="[site]/w/load.php?only=styles&modules=mobile.site"> and replace [site] with the domain of the wiki to which the article belongs?

Even easier: just add <link rel="stylesheet" href="/w/load.php?only=styles&modules=mobile.site.styles"> and it'll be interpreted relative to the base href set on page load in sections.js or preview.js without any further intervention.

(Note: as of a recent MobileFrontend commit, the module we want is now mobile.site.styles and not mobile.site.)

Dbrant added a comment.Apr 4 2018, 6:07 PM

OK cool! I got confused by the wording of "CSS for each page regardless of site", but it sounds like we're on the same page.

Cool! And, I just double-checked and you could set an explicit base site (href="https://en.wikipedia.org/w/load.php?only=styles&modules=${modules}") when linking the CSS modules that don't vary by site, and that will override the general base href so that you won't need to cache multiple copies of identical CSS as I was concerned about above.

Note: we're talking in terms of load.php URLs here but this could also easily work in terms of the new CSS endpoints:

<link rel="stylesheet" href="https://en.wikipedia.org/api/rest_v1/data/css/mobile/base">
<link rel="stylesheet" href="/api/rest_v1/data/css/mobile/site">

@bearND points out that the new endpoints will help to guard against sudden changes like the module name changing without warning, as just happened.

Tgr added a comment.Apr 4 2018, 6:59 PM

Cool! And, I just double-checked and you could set an explicit base site (href="https://en.wikipedia.org/w/load.php?only=styles&modules=${modules}") when linking the CSS modules that don't vary by site, and that will override the general base href so that you won't need to cache multiple copies of identical CSS as I was concerned about above.

That'll result in the wrong version of the CSS being loaded when the wiki is in the train group 0 or 1 and gets updated 1-2 days before enwiki (although given that the CSS has pretty long expiry anyway it won't matter that much).

bearND added a comment.Apr 4 2018, 7:27 PM

@Tgr The expiry is not that long. We have it set to one day for CSS.

Tgr added a comment.Apr 5 2018, 9:22 AM

The difference in deploy schedule between enwiki and most other wikis is one day (there are very few group0 wikis) so a one-day expiry means any problems with outdated CSS due to using code from a different deploy group are not worse than they would already be.

We could as easily obtain the wiki-independent bundle from a group0 or group1 wiki. There's no special reason to load this bundle from enwiki. OTOH, it's far from rare that group0 and group1 get rolled back for one reason or another, so having the relative stability of loading from group2 might be a good thing.

@Dbrant sounds like you've no objection to merging https://gerrit.wikimedia.org/r/#/c/420375/, then? :)

Mholloway added a subscriber: Jhernandez.EditedApr 5 2018, 4:48 PM

Hey @Jhernandez, @Fjalapeno mentioned that you might have some feedback on how the PCS's CSS endpoints should be structured, coming from the perspective of a progressive web app client. To summarize the above discussion, the basic proposal now is to expose a /base endpoint with a selected set of site-independent CSS, and a /site endpoint that basically wraps MediaWiki:Mobile.css/mobile.site.styles. I've also added a /bundle endpoint that just combines the two responses, but we're considering having the apps just link the first two separately, and possibly dropping /bundle.

The current code is here:
https://github.com/wikimedia/mediawiki-services-mobileapps/blob/master/routes/data/css.js
https://github.com/wikimedia/mediawiki-services-mobileapps/blob/master/lib/css.js

Note that the goal for the current implementation is just to get what the apps are doing now behind a PCS endpoint. There's no provision for per-page variation in the modules loaded yet, though that might come in the future.

(Edit: added link to the routes file.)

Change 420375 merged by jenkins-bot:
[mediawiki/extensions/MobileApp@master] Remove enwiki.less

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

cmadeo added a subscriber: cmadeo.Apr 5 2018, 5:20 PM
Tgr added a comment.Apr 6 2018, 1:47 PM

Ideally it would be loaded from a wiki in the same group; in general, using something other than the current wiki seems like a micro-optimization with dubious benefits. Wikimedia wikis used to load everything from bits.wikimedia.org, but then it turned out to be slower than loading from the local wiki (even with the extra cookies) due to the extra DNS lookup + SSL handshake.

@Tgr That's a good point; and in practice I'd expect the vast majority of app users to use no more than 1-3 languages, so it's probably a mistake to worry too much about caching redundant copies of CSS stylesheets.

@Mholloway Hey, makes sense to me. Seems like the per-page CSS will be considered later, and there have been comments about the caching and rollouts of the different groups and wikis, so it seems like it has been well discussed.

It seems to me that keeping the site css separate in different URLs from the base one makes a lot of sense, given they have different caching characteristics. So +1 to getting rid of /bundle and keeping them separate.

Change 425831 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/services/mobileapps@master] CSS: Remove /bundle endpoint

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

Change 425831 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] CSS: Remove /bundle endpoint

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

Mholloway closed this task as Resolved.Apr 13 2018, 6:29 PM

Change 422053 abandoned by Mholloway:
Update handling of mobile site CSS

Reason:
see Ib7bbbc79ddc626a7dceb81229f0a482092ad243f

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