Page MenuHomePhabricator

Dark mode broken (page background remains light) in apps.
Closed, ResolvedPublicBUG REPORT

Description

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.

Event Timeline

Dbrant triaged this task as Unbreak Now! priority.Jan 25 2019, 9:05 PM
Dbrant created this task.
Restricted Application added subscribers: Liuxinyu970226, Mholloway, TerraCodes, Aklapper. · View Herald Transcript
bearND added a subscriber: bearND.Jan 25 2019, 9:13 PM

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.

bearND added a comment.EditedJan 25 2019, 9:51 PM

@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

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

Mhurd added a subscriber: Mhurd.Jan 25 2019, 10:34 PM

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

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

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)

Mhurd added a comment.EditedJan 25 2019, 11:23 PM

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.

Mhurd added a comment.EditedJan 25 2019, 11:50 PM

@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?

Mhurd added a comment.EditedJan 25 2019, 11:51 PM

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?

There usually is :)

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.

@Mholloway @Dbrant @bearND

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 Operations who can purge the cache for this? (assuming that's the problem)

bearND added a comment.EditedJan 26 2019, 5:01 AM

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.

Dbrant closed this task as Resolved.Jan 28 2019, 11:28 PM
Dbrant claimed this task.

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

Dbrant reopened this task as Open.Feb 25 2019, 5:27 PM

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

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

Change 492751 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] CSS: fix for dark mode on Android 4.4

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

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.

@Dbrant Would you mind signing this off if it is fixed?

Dbrant closed this task as Resolved.Feb 26 2019, 6:42 PM

Fixed indeed! Thanks all.