Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a theming system (dark theme / night mode) #582

Merged
merged 16 commits into from
Feb 3, 2020
Merged

Conversation

Jaifroid
Copy link
Member

@Jaifroid Jaifroid commented Jan 3, 2020

This backports (but with much improved and generic coding) a dark theme as described in #265.

I have implemented this as a generic theming system, though only three themes are currently implemented: light (the current default); dark (a theme for the app shell only); and dark_invert (a theme for the app shell and the content in the iframe). The screenshots below shows the dark_invert theme.

The app shell themes do not touch the document loaded in the iframe. the content theme of course has to attach a stylesheet, using DOM methods, after the document has loaded. It works in both jQuey and Service Worker modes.

Inversion is carried out by adding a simple inversion filter to the iframe element (articleContent), but we have to attach a stylesheet to the iframe document to re-invert images and videos (generically) so that they do not look like photographic negatives. There are two non-generic rules that are specific to Wikimedia ZIMs (the vast majority). This is practically inevitable: for discussion see openzim/mwoffliner#1026 . If that proposal is implemented in the ZIM, it may be possible to remove one of the non-generic rules for future ZIMs. If a JS API is added to the ZIMs in the future, it will be possible to remove both non-generic rules.

Image rendering

image

Configuration

image

Screenshot showing equations and search

image

Stackexchange

image

@Jaifroid
Copy link
Member Author

Jaifroid commented Jan 4, 2020

Working in FFOS (screenshot), but not in IE11. IE10+ does not support CSS filters and never will. Ironically, IE9 did support an inversion filter, but it was buggy and dropped from IE10+. As this feature does not prevent core functionality in the app, I don't think it's crucial to support IE11 in this particular case.

image

@mossroy
Copy link
Contributor

mossroy commented Jan 4, 2020

That looks cool.
I agree with you about Internet Explorer : it's an optional and non-critical feature (even if quite appreciated, as you pointed), so never mind if such an old deprecated browser does not support it. It would be the same for Firefox OS (or other old browsers) if it didn't work on it.

As you would expect, I'm against putting some ZIM-specific code in our Kiwix client, so I don't like the mwoffliner-specific CSS classes.
I read the discussion on openzim/mwoffliner#1026, and agree with @kelson42.
I also understand that there is currently no other way to have a 100% working night mode on these ZIM files.

Maybe a compromise would be to have 2 different "content themes" instead of a single one :

  • a generic one, that would only do what can be generic (like re-inverting the colors of images and videos). This one would not work 100% on current mwoffliner ZIM files (for mathML and SVG drawings). But it would be supported in the long term, and could take advantage of future (generic) mwoffliner improvements, like the JS API
  • a "temporary" mwoffliner-specific one, that would add the mwoffliner-specific classes. This one would become unsupported as soon as the generic one could work 100%. But we would have to keep it for a while, for compatibility with people that kept old ZIM files.

We would need to find the right wording in the Settings section (and in the code). So that, in the future, a user that kept this "temporary" theme in its preferences would understand he/she should try to switch to the generic one if there are visual issues (with future ZIM files).

@Jaifroid
Copy link
Member Author

Jaifroid commented Jan 4, 2020

Thanks for that feedback, @mossroy . I completely agree with all your points, and it's very easy to add further custom themes - I'll have a think about the wording. It would actually work with popular Wikipedia-specific dark themes such as https://github.com/StylishThemes/Wikipedia-Dark - CSS source code too, but I think for our purposes what you suggest is good compromise.

@Jaifroid
Copy link
Member Author

Jaifroid commented Jan 4, 2020

I've implemented these changes, with a pure theme (dark_invert) and one with Wikimedia workarounds (dark_mwInvert). I've also added some text to explain that appears if the latter is chosen. Finally, I've added a link that appears if the theme has changed for the user to display the article with the new theme applied, without reloading the landing page.

image

@Jaifroid
Copy link
Member Author

Jaifroid commented Jan 5, 2020

No rush to review -- the code needs thorough field-testing, though I've done my usual checks in FFOS, Edge and Chromium, with Wikimedia, Stackexchange and Gutenberg ZIMs.

Copy link
Contributor

@mossroy mossroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not test, but here are some remarks on the code

www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Show resolved Hide resolved
www/css/app.css Show resolved Hide resolved
www/css/app.css Show resolved Hide resolved
@mossroy
Copy link
Contributor

mossroy commented Feb 3, 2020

I've tested on a Firefox OS device : an old platform issue seems to be back when you choose one of the two "Dark+invert" themes : the article content can not be scrolled any more (see #175).
Except from that, it seems to work fine on it.
As it's an optional feature, and this bug seems platform-specific (it does not happen on the Firefox OS Simulator), I think we can simply ignore this bug.

Copy link
Contributor

@mossroy mossroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quickly tested, and it looks fine

@Jaifroid
Copy link
Member Author

Jaifroid commented Feb 3, 2020

Thanks @mossroy . I have noticed at a number of points when developing Kiwix JS Windows a similar issue to #175 on Windows Mobile, which seemed to do with with the OS sometimes sending touch only through to the top-level container -- in our case this would be <section id="search-article" role="region"> which contains the entire UI and the iframe, not just the search-article UI as its ID implies. It would occasionally happen when switching from Config (or About) to the articleContent, that the article could not be scrolled by touch. At various points I fiddled with z-indexes, but don't have a clear-cut solution.

Of course, the issue you notice could be simpler: the app freezes because of the extra cpu used in calculating colour inversions on all the elements in this case.

Once I've merged this, I'll see if I can reproduce a set of conditions on another slow mobile, because it may be revealing something about the code or UI design. If I can reproduce something, I'll open a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants