Page MenuHomePhabricator

Minerva handles amboxes not using tables incorrectly
Closed, ResolvedPublic

Description

I've updated all amboxes on the Italian Wikipedia to use flexbox instead of tables. They work just fine in the desktop version, but Minerva doesn't seem to be able to handle them correctly. You can check this page as an example; you'll notice that the icon overlaps the text, and the text of the warning is truncated. The "Learn more" link is also not displayed.

I don't really know how Minerva's logic for amboxes works, but this seems to be caused by the new version of the template not using tables. If you go to the same example page and edit the HTML manually, replacing the outer <div> with <table><tr><td>, it'll start working again. In fact, it looks like the relevant Minerva stylesheet explicitly targets table elements; for instance, the font-size rule is only applied to td elements.

AFAICS, the new version of the template, just like the old one, follows all the recommendations for mobile-friendly amboxes, which say nothing about tables.

QA steps

Check the page issue shows on https://en.m.wikipedia.org/wiki/Lody_Kong like so:

image.png (638×2 px, 136 KB)

Event Timeline

I was able to fix this locally with the following rules:

.avviso {
  position: relative;
  padding: 8px 8px 8px 32px;
}
.avviso-testo {
  font-size: 0.8125rem;
}

This restores the layout to the one we had with tables. Still, this rule should be rewritten to avoid targeting td.

That's a good question... The itwiki version has the following structure:

<div class="ambox">
  <div class="mbox-image"><!-- Image here --></div> <!-- Not always present -->
  <div class="mbox-text">
    <div class="mbox-text-div">
      <!-- Main text here -->
      <div class="hide-when-compact"><!-- Details here --></div>
    </div>
  </div>
</div>

My local fix applies position and padding to the outer ambox, and font-size to the mbox-text element.

I'm guessing this may not work everywhere, though... It might be interesting to compare at the implementations of amboxes on all wikis, and perhaps add something to the recommendations.

Well a table ambox would look like table.ambox > tr > td OR table.ambox > tbody > tr > td (I think it's always the former). I think it would be good to add a class .ambox-column that could be used in the div equivalent markup. If you push a patch for Minerva, I'll update https://www.mediawiki.org/wiki/Recommendations_for_mobile_friendly_articles_on_Wikimedia_wikis#Making_page_issues_(ambox_templates)_mobile_friendly to reflect this as best practice.

Jdlrobson removed a project: Web-Team-Backlog.

Well a table ambox would look like table.ambox > tr > td OR table.ambox > tbody > tr > td (I think it's always the former). I think it would be good to add a class .ambox-column that could be used in the div equivalent markup. If you push a patch for Minerva, I'll update https://www.mediawiki.org/wiki/Recommendations_for_mobile_friendly_articles_on_Wikimedia_wikis#Making_page_issues_(ambox_templates)_mobile_friendly to reflect this as best practice.

What would that class be used for in the div version? I assume it would be on both div.mbox-image and div.mbox-text?

I think .client-js .ambox > div.mbox-text would be the appropriate selector here alongside the existing .client-js .ambox td
I've updated the documentation: https://www.mediawiki.org/w/index.php?title=Recommendations_for_mobile_friendly_articles_on_Wikimedia_wikis&diff=5985877&oldid=5933889

Could you take care of the CSS side of things (I can review!) ?

Change 930901 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/skins/MinervaNeue@master] Update ambox styles so that they also work on divs

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

I think .client-js .ambox > div.mbox-text would be the appropriate selector here alongside the existing .client-js .ambox td
I've updated the documentation: https://www.mediawiki.org/w/index.php?title=Recommendations_for_mobile_friendly_articles_on_Wikimedia_wikis&diff=5985877&oldid=5933889

Could you take care of the CSS side of things (I can review!) ?

Done, thanks!

@Jdlrobson Well, now I'm wondering if the class should be called mbox-text or ambox-text (with a leading "a"). There seems to be some inconsistency already, so I'm wondering if either version should be preferred over the other.

mbox-text was a preexisting class that was hooked into by Minerva's hacks and should be preferred. It serves the general purpose of marking up the text portion of any message box, including but not limited to amboxes.

mbox-text was a preexisting class that was hooked into by Minerva's hacks and should be preferred. It serves the general purpose of marking up the text portion of any message box, including but not limited to amboxes.

Thanks, this explains why the itwiki template was already using it then :)

I'm adding User-notice because there's a non-zero chance of breaking some on-wiki styles. I'm not sure what a good not-too-technical wording for the message could be, but the idea is:

The Minerva skin now applies more predefined styles to the .mbox-text CSS class. This enables support for amboxes not using tables. Make sure that the new styles won't affect other templates in your wiki.

Jdlrobson updated the task description. (Show Details)

Should probably not test on a wiki page using table for its ambox still...

Change 930901 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Update ambox styles so that they also work on divs

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

@Izno we need to double check the existing table implementations are not impacted since this change may have touched them.

@Daimona Thanks for the draft wording! I've added it to https://meta.wikimedia.org/wiki/Tech/News/2023/26 with the wording & links:

The Minerva skin now applies more predefined styles to the .mbox-text CSS class. This enables support for mbox templates that use divs instead of tables. Please make sure that the new styles won't affect other templates in your wiki. (1) (2)

For the final sentence, I tried to think of a standard localized link where editors could check, but I could not think of one. (i.e. 'ambox' isn't consistently the name in every language).
If anyone has any improvements to that entry text, please edit directly within the next ~24 hours. Thanks!