Page MenuHomePhabricator

Put the class media option on the wrapper or media element?
Closed, ResolvedPublic

Description

A few options to consider,

  1. Continue to do what Parsoid is doing and put the class on the wrapper element. This will cause disruption when we enable the flag in T314318, as this task attests, but will keep the class in a selector friendly location.
  2. Move the class back to the media element. This should cause less disruption, since that's where the class used to be, but probably not no disruption, since we've changed the media structure. Specifically, we've added wrapper tags where none existed before and those may need to be factored in to previous selectors and logic. Since the class isn't in a selector friendly location, additional support may need to be added since :has() isn't widely supported, T287963#8198665.
  3. The idea from T287963#8205920. Put the class back on the media element, as in 2., but add an additional class to the wrapper element with a prefix (ex. mw-wrap-). This anticipates the problems of 2. and gets out ahead with a solution so that the entire structure can targeted if desired. It adds redundancy though, which may not be desirable.
  4. Do 2., but add a |wrapperClass= media option so that the wrapper can be targeted, if necessary, without all the duplication.

Note that doing anything other than 1. is going to break VE when ported to Parsoid, but that's not a blocker to rolling it out in T314318.

If we ultimately wanted the class option to be on the wrapper, as in 1., we could duplicate it there with a prefix, as suggested in 2. Then get all the wikis to migrate to explicitly asking for the wrapper placement with, |class=prefix-, and then eventually just drop the img class and cleanup all the prefixes.


I've noticed an important different between the two formats and it relates to where the emitted class is located. Before this class was applied directly to the img object, but now it seems this is on the wrapping class. While this is probably a rare use case, it's an important change and I know at least some image headers on mediawiki.org etc which likely will have to be adapted to take this into account.

[[File:Onion diagram.svg|thumb|right|300px|In this example onion, we test image placement.|class=djtest]]
<figure class="mw-halign-right djtest" typeof="mw:Image/Thumb"><a href="/wiki/File:Onion_diagram.svg" title=""><img alt="" src="//upload.wikimedia.org/wikipedia/commons/thumb/e/e1/Onion_diagram.svg/300px-Onion_diagram.svg.png" decoding="async" width="300" height="197" srcset="//upload.wikimedia.org/wikipedia/commons/thumb/e/e1/Onion_diagram.svg/450px-Onion_diagram.svg.png 1.5x, //upload.wikimedia.org/wikipedia/commons/thumb/e/e1/Onion_diagram.svg/600px-Onion_diagram.svg.png 2x" data-file-width="875" data-file-height="575"></a><figcaption>In this example onion, we test image placement.</figcaption></figure>
<div class="thumb tright"><div class="thumbinner" style="width:302px;"><a href="/wiki/File:Onion_diagram.svg" class="image" title=""><img alt="" src="//upload.wikimedia.org/wikipedia/commons/thumb/e/e1/Onion_diagram.svg/300px-Onion_diagram.svg.png" decoding="async" width="300" height="197" class="djtest thumbimage" srcset="//upload.wikimedia.org/wikipedia/commons/thumb/e/e1/Onion_diagram.svg/450px-Onion_diagram.svg.png 1.5x, //upload.wikimedia.org/wikipedia/commons/thumb/e/e1/Onion_diagram.svg/600px-Onion_diagram.svg.png 2x" data-file-width="875" data-file-height="575"></a>  <div class="thumbcaption"><div class="magnify"><a href="/wiki/File:Onion_diagram.svg" class="internal" title="Enlarge"></a></div>In this example onion, we test image placement.</div></div></div>

Event Timeline

I think this is an improvement in output. CSS allows me to style the inner <img> based on the class applied to the other <figure>, but there's no "ancestor of" operator in CSS so I can't possibly change the style of the outer <figure> if the class is only applied to the inner <img>. That is, all things mentioned by selectors should be in generally be applied to the outermost wrapper maximum flexibility in applying styles.

I don’t particularly care either way , I’m mostly concerned that this is a breaking change for assumptions made by Common.css and template styles authors and a very hard one to detect at that.

Arlolra triaged this task as Medium priority.Dec 9 2021, 5:48 PM

While doing a visual diff run for T310510, @ssastry reported that "one of the topmost diffs in talk page visual diff testing" comes from the carousel gadget (T297447)

https://en.wikivoyage.org/api/rest_v1/page/html/Talk%3AAsia
https://en.wikivoyage.org/wiki/Talk%3AAsia

The classes from [[File:Interactive icon.svg|frameless|link=|class=nolink interactiveIcon]] are targeted by,

.banner-image img.nolink { display: none; }
.banner-image img:not(.nolink) { display: none; }
.interactiveIcon { ... }

From https://en.wikivoyage.org/w/index.php?title=Template:Banner/styles.css&action=edit

These could be adapted for the new structure,

.banner-image .nolink img { display: none; }

.banner-image img { display: none; }
.banner-image .nolink img { display: inline-block; }

.interactiveIcon img { ... }

but maybe we should get a handle on how often the |class= option is used.

...
but maybe we should get a handle on how often the |class= option is used.

But, that is a probably an overly conservative upper bound on pages that will be impacted. I think you are looking for template invocations that have a templatestyles css and also generate a |class= attribute which is a less conservative upper bound.

I think this is an improvement in output. CSS allows me to style the inner <img> based on the class applied to the other <figure>, but there's no "ancestor of" operator in CSS so I can't possibly change the style of the outer <figure> if the class is only applied to the inner <img>. That is, all things mentioned by selectors should be in generally be applied to the outermost wrapper maximum flexibility in applying styles.

That no longer seems to be the case, with :has()
https://webkit.org/blog/13096/css-has-pseudo-class/

Support for :has() is quite low,
https://caniuse.com/css-has

In a team meeting, @cscott suggested putting the class on both the wrapper and media element, but that can have subtle and unintended consequences.

Another option could be to put the class on the media element and then put a prefixed class on the wrapper. That would let editors opt into using the wrapper class, at the expense of some redundancy. Picking the prefix would be the hard part.

For example, [[File:Foobar.jpg|class=123]] with a prefix "mw-wrap-" could be,

<span typeof="mw:File" class="mw-default-size mw-wrap-123">
 <a href="./File:Foobar.jpg" class="mw-file-description">
  <img class="123" resource="./File:Foobar.jpg" src="http://upload.wikimedia.org/wikipedia/commons/3/3a/Foobar.jpg"
       width="1941" height="220">
 </a>
</span>

My gut sense is to go with option 2 in the updated description. If a wikitext snippet adds an explicit class that is targeted by CSS, my expectation is (a) this is rare (b) likely to directly target the class with a .my-special.class { .. some css ...} rather than qualify the class with additional path selections that might be impacted by the wrappers. So, I expect this to be less of a disruption. And, if on rollout, this turns out to be a disruption on some wiki, we could add the wrapper class as proposed. Eventually, when the .has() selector support is more widely available, editors can target parents, etc.

But, I am no UI / CSS person and my thought process might not be capturing out this is actually being used on wikis.

Change 830715 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/core@master] [WIP] Move the class back to the media element

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

I think I favor option 3, but with a shorter prefix, maybe even none. My feeling is that *something* is going to need to target the outer wrapper, and since :has() support can't really be used yet, we're going to end up adding the class to the outer wrapper anyway as a workaround for /whatever it is we're going to break/. We might as well bite that bullet now, rather than force someone to wait a whole deploy cycle once we discover what we've broken.

We should then document in the docs for the wikitext image options ([[File:]] etc) that the inner class is deprecated and you *should* target the outer class in any new styles, and for those cases we know about we can update the CSS.

"At some future time" we can remove the inner class as a separate migration, probably after using visual-diff-testing to lint any remaining breakage. It's no big harm done if we never get around to doing this, or if it waits a Long Time until we have proper tools to lint CSS rules.

My argument for using no prefix is that none of the other class= options adds a prefix ({{#tag|foo|class=bar}} for example, or <span class=bar>[[wikitext]]</span>) , and it will be a future wart on wikitext if we add a prefix to only this particular case, especially if we succeed in eventually removing the inner unprefixed option. I understand that it would make it a little easier to determine with codesearch or wikitext search if a particular CSS rule has been "updated" but I don't think it helps all that much; I think you just just as easily add some other ad-hoc marker (/* T287963 */ in the CSS, or |class=T287963 foo in the wikitext) just for linting/conversion purposes.

I'm reluctant to apply the same class in two different locations (sans prefix). Wouldn't that result in styles being applied gratuitously? For example, .class { margin: 1em; }

Since we ultimately want the class on the outer wrapper, I think we should just bite the bullet and try to do option 1. We can get a sense of how difficult a migration it will be with some of the early adopter wikis.

I've updated the FAQ,
https://www.mediawiki.org/wiki/Parsoid/Parser_Unification/Media_structure/FAQ#The_|class=_media_option_is_now_applied_to_the_wrapper

While doing a visual diff run for T310510, @ssastry reported that "one of the topmost diffs in talk page visual diff testing" comes from the carousel gadget (T297447)

https://en.wikivoyage.org/api/rest_v1/page/html/Talk%3AAsia
https://en.wikivoyage.org/wiki/Talk%3AAsia

I've requested a change to those template styles,
https://en.wikivoyage.org/wiki/Template_talk:Banner#Prepare_for_T314318

Change 830715 abandoned by Arlolra:

[mediawiki/core@master] [WIP] Move the class back to the media element

Reason:

I'd like to avoid this, if we can

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