Page MenuHomePhabricator

mobile-html: revisit 'notheme' exclusion and investigate alternatives for theming/theme exclusion
Closed, ResolvedPublic

Description

Background information

Initial problem statement and 'notheme' proposal: T236137: mobile-html: Exclude elements from theming using a `notheme` class instead of querying every page for specific elements

What

Revisit the problem and investigate alternative solutions. Is there a better way to fix this?

How

Utilize the side by side testing tool to look at the affect of theming changes across different wikis and articles.

Acceptance criteria

  • Investigation completed
  • Either tickets are created for a new proposed solution or 'notheme' solution is confirmed

Event Timeline

I did an analysis of the current 'notheme' approach. I think the approach is solid and does not need substantial revision. However, in light of recent changes to the structure of the CSS and themes, I was able to make some augmentations to the existing approach that I think will be valuable to the mobile team as well as the editors of the articles.

The overall problem is to allow editors and certain articles to bypass the styles of the themes for particular elements. There are now multiple layers of implementation for the solution to this challenge. They include:

1. Restoration of the Default Theme and "No Theme" Portals
Most of our theming is now implemented using CSS variables, whose values shift depending on the currently-applied theme. By adding the "notheme" class to any element, it now reapplies the default theme to that element (and to all child elements). The fact that this applies to not only the element containing the "notheme" class but also applies to all child elements is an important change from the previous behavior. It used to be the case that "notheme" had to be applied to every single element in a tree that needed custom styles. This led to some tricky code in the server transforms as well as some more cumbersome HTML.

2. Overridable vs "Universal" Theme Overrides
There were several places in the old styles that had formulations like "th:not(.notheme)" indicating that the style should only be applied in the absence of a "notheme" class applied to that element. The styles without this ":not(.notheme)" selector are considered "universal" as they are applied regardless of whether the user has applied the "notheme" class to an element. To achieve this same effect, we've added two new utilities "nonDefaultThemesUniversal" and "darkThemesOnlyUniversal" to pair with the existing "nonDefaultThemes" and "darkThemesOnly".

"nonDefaultThemes" and "darkThemesOnly" behave like styles did when appended with ":not(.notheme)". Such styles are only applied in the absence of the "notheme" class and when an element is not a child of an element with the "notheme" class. "nonDefaultThemesUniversal" and "darkThemesOnlyUniversal" are applied regardless of whether the element contains the "notheme" class or whether its parent contains the "notheme" class.

These utilities are used most extensively in the "ThemesTransform" file and should be avoided whenever possible. It's best to use CSS variables for theming as much as possible to avoid issues with CSS specificity.


Finally, I took a look at how to better enable editors of articles to override default styles without having to use the "notheme" class (which reverts everything back to the default theme). It seems as though it is not unusual for tags in articles to have inline CSS in the style property, but those styles aren't always applied as expected. I identified three suggestions that might make the styling experience of article authors and editors simpler and less error prone.

  1. Eliminate the use of !important with respect to any style which an author might reasonably want to override. Many uses of !important in the existing stylesheets are in reference to position, margins, or sizes. These are more-or-less fine (although it's always better for you to not need any !important). But in any cases where !important is used because some other CSS rule in the base stylesheet or elsewhere is overwriting the mobileapps stylesheets, it's worth taking the time to refactor that rule and remove the !important.
  2. Apply styles to IDs, tags, or classes separately. Avoid creating styles that target a mix of those selectors. For example, the rule "#mainpage a.pcs-edit-section-link" hits all three types of selectors, which can make it difficult to overwrite and maintain. Instead, consider whether it's possible to use fewer selectors to achieve the same result or whether the styles can be layered with selectors that target each type separately.
  3. Try to keep specificity of selectors as low as possible (ideally 0,1,0). Consider adopting approaches such as BEM (http://getbem.com/introduction/) to reduce the specificity of CSS selectors. Where there are complex rules now (such as #pcs .dablink), consider a server-side transform that identifies these cases and adds a new class to the affected elements that can be targeted at lower specificity.
  4. Minimize the use of the overridable "nonDefaultThemes" and "darkThemesOnly" utilities. Each of these utilities takes a rule (such as .mwe-math-fallback-image-inline) and converts it into something longer (.pcs-theme-dark :not(.notheme) .mwe-math-fallback-image-inline:not(.notheme)". That takes something with specificity (0,1,0) and turns it into something with specificity (0,4,0). This significantly higher specificity will make it harder to layer your CSS in ways you expect.
Mholloway added subscribers: Robocel, Mholloway.

Thanks, @Robocel. I'm sorry that this appears to have fallen through the cracks with no clear owner for follow-up. I'm going to punt it back to our Needs Triage column so that we get a chance to discuss it collectively in grooming next week.

@JoeWalsh To help us evalute this, are there specific pitfalls in practice with the .notheme approach that prompted this analysis? Is the goal to make it apply and exclude themes more accurately? If so, how much more accurately? Can a target be quantified, or alternatively, are there specific cases that need to be handled better?

@Mholloway this task is resolved. The notes from @Robocel above are guidelines to use when adding styles going forward, and if there's any action item to this, it would be to add that to a readme. I'll put up a patch for that.

JoeWalsh claimed this task.

Change 621317 had a related patch set uploaded (by Joewalsh; owner: Joewalsh):
[mediawiki/services/mobileapps@master] Update notheme documentation.

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

Change 621317 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] Update notheme documentation.

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