Page MenuHomePhabricator

Skin: skin-invert-image does not work with nested figures, e. g. image in figure caption
Open, Needs TriagePublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

  • browse to german article for David Baldacci
  • switch to dark mode if inactive
  • observe the signature in the figure caption of the portrait image

What happens?:

  • the signature background is dim white, despite the class skin-invert-image set for the signature image

What should have happened instead?:

  • the signature image should display inverted, i. e. white on black in dark mode (the usual effect when setting skin-invert-image class)

Related source link
The lesscss code in https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/refs/heads/master/resources/src/mediawiki.skinning/content.media-dark.less does not deal well with nested figures/images. The compiled CSS rule figure[ typeof^='mw:File' ]:not( .skin-invert-image ) img currently corresponds to

  • <figure><a><img /></a></figure> as well as
  • <figure><a><img /></a><figcaption>lorem ipsum <figure><a><img /></a></figure></figcaption></figure>`

That is, the presence (or abscence) of class=skin-invert-image with respect to the first figure element 'wins' over the nested one. To fix this, the CSS selector in place needs to be more specific, for instance recode with child rather than descendant css selector.

Event Timeline

Jdlrobson-WMF subscribed.

The rules only target the standard case, they are not intended to be comprehensive. You will need to update the styles of the associated template or refactor the template to not put an image inside a figcaption.

This is a follow-up on T391800 with more things done right. The specifity of the CSS was bogus in the past and there is nothing wrong with having images in figcaptions.

The issue is not related to templatestyles, it is more a prerequisite to it. We have exemptions for the brightness filter in dark mode, because of the two ways images are rendered in this mode: 1) dim and 2) inverted. The keyword skin-invert-image has been coined to work with 2) and its usage should be consistent across not so common cases as well. That keyword should work as advertised on the help pages, thus the patch.

There is an uploaded patch that deals with the problem, see https://gerrit.wikimedia.org/r/1182627.

The patch does not invert your argument. It still targets the standard case, it just does so in a more predictable and consistent manner. If you ever have authored template styles with the wiki extension, you do appreciate consistent help pages.

The skin-invert feature of the software is not confined solely to templatestyles (read 'per page css') but to UI skinning of the mediawiki software itself. For instance, cp. to Vector2022 there is at least one more (UI) skin in production that depends on its correct handling. Its a feature used by various skin developers, justifying its presence at the core component.


In the future, if breaking browser compatibility with pre-2021 browsers is less an issue the CSS code can be condensed employing more recent CSS primitives, by employing :is(). While the below patch is larger, the CSS generated by a production mediawiki instance with this will be smaller, bandwidth-friendly, because lesscss does not expand/repeat the rule for the relevant selector combinations. However, the difference should be neglectable and the computation time spent on the client roughly equivalent.

--- a/resources/src/mediawiki.skinning/content.media-dark.less
+++ b/resources/src/mediawiki.skinning/content.media-dark.less
@@ -2,19 +2,25 @@
 @import 'mediawiki.skin.variables.less';

 .dark-mode-image-styles() {
-       /* Desktop legacy HTML */
-       .mw-parser-output,
-       /* Parsoid HTML (mobile) */
-       section .mw-heading ~ div,
-       /* Parsoid HTML (desktop) */
-       section,
-       /* e.g. https://en.wikipedia.org/wiki/Pullback_(category_theory)#Universal_property */
-       dd,
-       p {
-               > span.mw-default-size,
-               > figure[ typeof^='mw:File' ] {
+       :is(
+               /* Desktop legacy HTML */
+               .mw-parser-output,
+               /* Parsoid HTML (mobile) */
+               section .mw-heading ~ div,
+               /* Parsoid HTML (desktop) */
+               section,
+               /* e.g. https://en.wikipedia.org/wiki/Pullback_(category_theory)#Universal_property */
+               dd,
+               p
+       ) {
+               :is(
+                       > span.mw-default-size,
+                       > figure[ typeof^='mw:File' ],
+                       :not( .noviewer ) > span.mw-default-size,
+                       :not( .noviewer ) > figure[ typeof^='mw:File' ]
+               ) {
                        &:not( .skin-invert-image ) {
-                               img {
+                               > * > img {
                                        background-color: @background-color-base-fixed;
                                        color: @color-base-fixed;
                                        filter: brightness( 0.8 );

EDIT: Since the lesscss php implementation of mediawiki lags behind that of lesscss.org (js) the above patch may to date be rejected by gerrit, i. e. if :is() is not part of the grammar and rejected. For instance, with the Less Preview at lesscss.org, the above stops working below 4.2.2 versions of lesscss.

The patch does not invert your argument. It still targets the standard case

That may be true, but the dark mode project hasn't been active for over a year ago, and these rules are pretty battle tested so this is a risky change and unlikely to be taken on by anyone any time soon. If you want this fixed, you should fix it in the template per my suggestion.

The rules are not completely battle tested, hence the bug report. The report is not here to offend anyone, but to fix a problem that has not been noticed at (initial) roll out of the feature. However, the replacement rules do not divert very much from the previous ones, they just work for nested cases too.

I cannot and won't fix it in "the template". There is no generic template where this could be fixed proper and, lets assume there were, it would merely fix a single language version, when each instance of mediawiki employing vector or minerva skin is affected.

If you feel it may produce regressions then either test in a non-production environment first, or, simply revert the change at a later point in time if substantial bug reports are submitted mid-term on phab.

The patch is small enough for easy peer review, imho.

It is better if it takes some time here to notice the problem and fix the bug at the proper spot, than to repeat a workaround-solution in the manifold instances of the mediawiki software installations.