| Dbrant | |
| Jan 25 2019, 9:05 PM |
| F28043501: no goes away without bgcolor setting.png | |
| Jan 25 2019, 11:23 PM |
| F28043494: bad.png | |
| Jan 25 2019, 11:23 PM |
| F28042125: device-2019-01-25-160134.png | |
| Jan 25 2019, 9:05 PM |
With the latest update(s) to the CSS endpoint and/or the Minerva styles themselves, it looks like the ability of the page-library to set the Theme of the article to stop working.
There don't seem to be any javascript errors, so it's a bit difficult to pinpoint where exactly the error lies.
The main problem is that the background of the text itself is not being set correctly, which leads to the following appearance in dark mode:
We are getting a torrent of OTRS feedback about this issue.
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | None | T169242 Develop Page Content Service for Reading Clients | |||
| Resolved | None | T177425 Develop General Layer of PCS | |||
| Resolved | None | T177433 Develop HTML Content API to be used with companion structured JSON APIs | |||
| Resolved | • Jhernandez | T220044 PCS CSS improvements | |||
| Resolved | BUG REPORT | Dbrant | T214728 Dark mode broken (page background remains light) in apps. |
Hmm, I can repro in the Android app but not with https://en.wikipedia.org/api/rest_v1/page/mobile-html/Cat, which is surprising. (I manually added class="pagelib_platform_android content-ltr pagelib_theme_dark pagelib_dim_images" to trigger dark mode using Chrome inspector.)
If all else fails, would it be possible to roll back today's update to the CSS endpoint? Dark mode seems to work fine without it, and this is clearly a more serious breakage than the H1/H2 styles...
@Dbrant. Yes, we could do that. Let me know when you are done trying to find a fix. I'm going to look a bit, too.
Where does the css live? Are there any errors in inline comments in the CSS?
Only changes I can think on Minerva side are https://gerrit.wikimedia.org/r/483649 and https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/483650... both are not reversible at this stage.
@Jdlrobson The revert would be done at the PCS (base CSS) level, not MinervaNeue. Reverting T214714 would result into changes in the H1/H2 headings. Any help with that would be appreciated.
The CSS is at https://meta.wikimedia.org/api/rest_v1/data/css/mobile/base
Change 486695 had a related patch set uploaded (by BearND; owner: BearND):
[mediawiki/services/mobileapps@master] Remove skins.minerva.base.styles for now
Per chat with @bearND I'm taking a peek at a page lib fix while he handles the short-term fix.
Change 486695 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] Remove skins.minerva.base.styles for now
Mentioned in SAL (#wikimedia-operations) [2019-01-25T22:42:25Z] <bsitzmann@deploy1001> Started deploy [mobileapps/deploy@5e859c4]: Update mobileapps to a8834e8 (T214728)
Mentioned in SAL (#wikimedia-operations) [2019-01-25T22:45:52Z] <bsitzmann@deploy1001> Finished deploy [mobileapps/deploy@5e859c4]: Update mobileapps to a8834e8 (T214728) (duration: 03m 27s)
I think I found the change which is causing the bug:
Without that line it works as it did previously:
Background: The page lib theme css currently tells certain elements to inherit their color. Some divs inside the content div are doing this, so that change which forces the content div to white means these nested divs inherit white.
I can tweak the page lib to work around this, or if doesn't create any issues the new white .overlay-enabled,#content{ setting could be removed upstream. Let me know how you want to proceed :)
@Jdlrobson Do you know what introduced the .overlay-enabled,#content{ setting I mentioned above?
@Mhurd Previously MCS included skins.minerva.base.reset in the /css/mobile/base response. skins.minerva.base.reset was merged into skins.minerva.base.styles in this commit (https://github.com/wikimedia/mediawiki-skins-MinervaNeue/commit/24b1e7d54d348165d20781c3f2647c5bd0f19158) which rolled out to all wikis yesterday. Today @Dbrant noticed that some styles were missing, we found the MinervaNeue commit, and we determined that we needed to include skins.minerva.base.styles in the /css/mobile/base response.
I don't think that rule is new to skins.minerva.base.styles, just new to /css/mobile/base since we hadn't been including it previously.
@Mholloway Ah makes sense! Can that single color setting be removed (wondering if it even does anything, and it seems like making such an assumption about the #content background color would only make any attempts at theme-ing harder than need be...) or do you think the page lib should deal with it?
I can tackle page lib changes if you think that's the best option... just wondering if an upstream tweak would be easier/faster/better...
@Mholloway ya I had to change skins.minerva.base.reset to skins.minerva.base.styles to repro on iOS. @bearND cued me in :)
@Mhurd I'll have to defer to @Jdlrobson on that one. I don't know why the rule exists, but presumably there's some good reason?
See https://phabricator.wikimedia.org/T214714#4910259
The rule is needed for Minerva skin for the site chrome. None of that module is likely useful in apps.
I recommend you roll your own css normalize/reset file and drop the base styles.
Longer term let's find a more sane way for you to obtain the content styles in Minerva. There are likely big changes coming there as part of our refactoring work that will impact you further.
Here's a page lib patch which works with both skins.minerva.base.reset and skins.minerva.base.styles:
https://github.com/wikimedia/wikimedia-page-library/pull/171
If you could try a quick Android integration to confirm (only tested on iOS) I can cut build whenever.
Do we know when the cache(s) will be updated? It's currently still serving the old content.
I'm still seeing the old CSS content. Is there someone from SRE who can purge the cache for this? (assuming that's the problem)
I purged the URLs in the form of https://[lang].wikipedia.org/api/rest_v1/data/css/mobile/base for all Wikipedias on this list: https://phabricator.wikimedia.org/diffusion/GMOA/browse/master/private/languages_list.json.
As mentioned on IRC consider using meta.wikimedia.org instead of the language specific Wikipedia in the app for the base CSS.
Change 487908 had a related patch set uploaded (by BearND; owner: BearND):
[mediawiki/services/mobileapps@master] CSS hotfix for Android
Change 487908 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] CSS hotfix for Android
Unfortunately this is still not fully fixed, namely on API 19 (KitKat):
Apparently the version of Chromium shipped with KitKat does not support the unset keyword in CSS, therefore the fix applied earlier is not having any effect. It's possible that there's a webkit-specific keyword that we can use, but I can't find it...
Change 492751 had a related patch set uploaded (by BearND; owner: BearND):
[mediawiki/services/mobileapps@master] CSS: fix for dark mode on Android 4.4
Change 492751 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] CSS: fix for dark mode on Android 4.4
Mholloway deployed about an hour ago. I purged https://meta.wikimedia.org/api/rest_v1/data/css/mobile/base and many languages https://<lang>.wikipedia.org/api/rest_v1/data/css/mobile/base, for older versions of the Android app which still use the base CSS from WP.