Page MenuHomePhabricator

Categories as link tags cause navboxes to have a rendering difference
Open, HighPublic

Description

Parsoid is outputting a link tag for categories being transcluded (and maybe raw category tags, I don't know). This is causing a negative impact for the CSS that causes navboxes to "collapse" to a single border.

Ex, what is displayed is a double border (mediawiki wiki):

image.png (255×980 px, 19 KB)

What should be displaying is a single border (mediawiki wiki):

image.png (254×982 px, 18 KB)

The relevant CSS is

.navbox + .navbox-styles + .navbox {
	/* Single pixel border between adjacent navboxes */
	margin-top: -1px;
}

There's no viable way to fix this via CSS. I anticipate this change should be noticeable by visual diffing but perhaps it is being treated as noise, but it will be noticed on en.wp when read views come along. This particular styling is deliberate for at least the past 15 or so years.

See also: On English Wikipedia at least, users aren't supposed to include content categories via templates, but there are other ways to cause this effect I would guess, perhaps including raw categories in the wikitext between navboxes, definitely between amboxes (which should have a similar collapsing going on), maintainence categories (which do get put out by amboxes), and so forth; and I'm pretty sure our wiki is one of the few with this specific practice and so I would guess there are others that allow templates to carry content categories.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I don't think we need <link> tags in not-top-level content in order to enable selective updating; we're going to have to store that data out-of-band (in data-parsoid) anyway.

We could investigate stripping <link> tags for read views. I'm not sure it's a good idea, but it wouldn't be too hard if we decided to do it.

Note also that the template could potentially be modified: moving the category tag in the wikitext will change where the category tag ends up in the HTML, possibly avoiding the issue.

I'd also like more information about how wide-spread this issue is: is the CSS coming from a global stylesheet like vector, or a site-specific stylesheet? Is this particular template used on one wiki, or copied to all, etc. It's fair to say the parsing team is heavily backlogged already, so an issue which can be solved by editors changing CSS or changing a template helps distribute the work more broadly than making fundamental changes to how Parsoid represents explicit wikitext-generated category tags in our output.

This is template:Navbox, the styles are in Common.css or in TemplateStyles depending on the project, Navbox is copied everywhere. It's also ambox. Which is also copied everywhere.

so an issue which can be solved by editors changing CSS or changing a template

The first I already said is not viable - there is no replacement CSS for this effect. The second is not viable for social reason at a minimum. Categories go after templates in the template definition, that's just one of Those Things that is an organizational requirement. (It's already an issue and a common FAQ type question that whitespace between the end of }} and the beginning of <noinclude> in a template definition is not removed and needs to be removed.)

The following CSS seems to work:

.navbox ~ .navbox-styles + .navbox {
	/* Single pixel border between adjacent navboxes */
	margin-top: -1px;
}

There are probably other ways to allow additional elements between the navbox and the navbox-styles.

The subsequent sibling combinator will cause issues that do not make it viable (and is precisely one of the solutions I referenced in the original task description about viability). Navboxes do not all appear in one place. Nor do amboxes, which share the issue presented here.

I already had to solve this problem and the solution was to add the navbox-styles container, because TemplateStyles does the same deal of leaving stray metadata around (see T200206 for the previous thought process). Fixing categories to have the same solution as there adds to template expansion size problems, can't be sold as a ready made solution, and has unknown numbers of existing templates that would need fixing, and which I suspect can't even be identified in this case.

Here is a more selective alternative:

<style>
  .mw-parser-output div.navbox {
	/* Single pixel border between adjacent navboxes */
	margin-top: -1px;
}
	.mw-parser-output :is(div.navbox-styles:first-of-type, div:not(.navbox, .navbox-styles) + .navbox-styles)+ .navbox {
               /* undo above rule for "first" navbox, or navbox preceded by non-navbox content */
		margin-top: auto;
	}
</style>

This style of solution can also be rewritten to avoid the need for the .navbox-styles container.

Change #1194292 had a related patch set uploaded (by Subramanya Sastry; author: Subramanya Sastry):

[mediawiki/services/parsoid@master] WIP: Migrate leading/trailing rendering-transparent nodes in templates

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

Change #1194292 merged by jenkins-bot:

[mediawiki/services/parsoid@master] Migrate leading/trailing rendering-transparent nodes in templates

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

ssastry triaged this task as High priority.

Change #1195699 had a related patch set uploaded (by Isabelle Hurbain-Palatin; author: Isabelle Hurbain-Palatin):

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.22.0-a27

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

Change #1195699 merged by jenkins-bot:

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.22.0-a27

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

The inter-navbox spacing issues on enwiki are fixed, but the example Izno provided above isn't because my patch narrowly targeted only newline wrapping instead of whitespace wrapping (and Izno's example has </div> [[Category:..]] So, the separator is a space char, not a newline like the common usage on enwiki.

@Izno I am inclined to close this task as resolved even though your specific example is not fixed. There is a simple wikitext fix solution available which is the change the to a \n and the patch we deployed will kick in. The patch already fixes all the ugly inter-navbox spacing on the test pages and your example looks like an edge case which has a relatively easy fix.

https://www.mediawiki.org/w/index.php?title=User:Izno/Sandbox/Category_holder&diff=prev&oldid=7945477 fixes it for example. I'm going to close this as resolved. Please leave a comment if you disagree. Since the patch is kind of a mild hack, we mostly want to keep it as narrow as possible.

https://www.mediawiki.org/w/index.php?title=User:Izno/Sandbox/Category_holder&diff=prev&oldid=7945477 fixes it for example. I'm going to close this as resolved. Please leave a comment if you disagree. Since the patch is kind of a mild hack, we mostly want to keep it as narrow as possible.

This will cause the same end result as described in this task for legacy parser. We've spent an absolute ton of time training people to put the noinclude on the same line, which isn't illustrated in this version but would be in the context of amboxs, which as described above will have the same problem as navbox.

Oh, there is no problem with having the </includeonly> or noinclude on the same line. It is the " " that is the problem. Do you need it? If not, it can be on the same line. Can you give me an example of a page with the ambox page (with a rendering diff, ideally) that I can look at?

I've added amboxes to the same page. (They are tables currently and probably a good chunk of the fleet, but I am incidentally working on making them divs. Which still doesn't fix the whole fleet.)

Ah, I had the fix narrowly target divs for navboxes. Can't do the same for tables because of fostering reasons. Short of hoisting deeper -- into the "last" <td> -- which feels very ugly, I don't have a good Parsoid-side solution at this point.

Alternatively, going back to 'adapt-the-css' route, If it helps, we could wrap all the category link tags in a div/span tag with a class ("mw-category-links-container") and an explicit display:none; in case that will help with adapting the CSS rules, just like the introduction of templatestyle .navbox-styles intermediate node.

I am going to reopen the task since we are chatting more here.

Ah, I had the fix narrowly target divs for navboxes. Can't do the same for tables because of fostering reasons. Short of hoisting deeper -- into the "last" <td> -- which feels very ugly, I don't have a good Parsoid-side solution at this point.

Yeah, it was a super-big parenthetical. (The infobox template does something similar to that kind of hoisting today but it's ugly and I plan to get rid of it at some point when that gets rewritten.)

We could wrap all the category link tags in a div/span tag with a class ("mw-category-links-container") and an explicit display:none; in case that will help with adapting the CSS rules, just like the introduction of templatestyle .navbox-styles intermediate node.

This would work great I think on our side. I have no complaints writing another + .container into the rule. I guess you'd do it for all categories transcluded from a template onto a page? It would actually have some other beneficial properties, right now the infobox template also has an incomplete hack in it to remove categories that would otherwise cause empty rows... it's in the same area as the hoisting mentioned above. Incidentally, that hack also tries to hit TemplateStyles tags for the same reason.... (... yeah...). Would be cool to get this hidden stuff all in a single place also though I can see this expanding the scope of this task (and for e.g. TemplateStyles hits the same issue-thought as below).

I guess the only concern I would have is to do with category sort keys, which are source dependent. I would guess this is a pretty rare case, but it does seem like a spot where you would want to guarantee wikitext source order insertion into the hidden span? And also, transclusions including other templates which may bring their own categories (uh, like Page transcluding Infobox which contains in Page a {{birthday}} template that adds a category for age, and the Infobox also including a category - maybe an example of this in reality for this specific question would be the {{multiple issues}} template which is a container for other amboxes, and each of those amboxes, including the multiple issues template, emit categories).

.mw-empty-elt

I'd like something specific here, or at least, in addition to .mw-empty-elt (which can carry the display: none CSS if we want, then we're just using the new class to carry semantic intent to onwiki). List items of this type are a dime a dozen.

I'll get back on this once we've worked through some details.

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

[mediawiki/services/parsoid@master] Limit transferring typeof to mw:Transclusion

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

Change #1198601 merged by jenkins-bot:

[mediawiki/services/parsoid@master] Limit transferring typeof to mw:Transclusion

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

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

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.22.0-a29

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

Change #1199040 merged by jenkins-bot:

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.22.0-a29

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

I've added amboxes to the same page. (They are tables currently and probably a good chunk of the fleet, but I am incidentally working on making them divs. Which still doesn't fix the whole fleet.)

We had a chat about this in our team meeting last week. If you are working to convert the table-amboxes to use divs instead, the existing patch in Parsoid will fix them all. So, at this time, we are inclined to wait it out over the next 6 months unless you anticipate you wont get to all the table-amboxes.

Changing status to blocked until we resolve what additional things we need to do here. For now, as noted above, our inclination is to wait it out for Izno's fixes to land.

This comment was removed by cscott.

We discussed this task in Content-Transform-Team tech forum today. I wrote up our discussions related to the expected contents of transclusions at https://www.mediawiki.org/wiki/Specs/HTML/2.8.0#Contents_of_transcluded_blocks .

Summarizing our discussion:

For cases like
</div> [ws]* (<link tag > [ws]*)* </outermost template wrapper> we handled these previously in Migrate leading/trailing rendering-transparent nodes in templates (1194292) by migrating the link tags and whitespace inside the immediately preceding element.

For <table …> … </table>(<span>[ws]</span><link/meta>)*<span>[ws]</span><table>.... things are more complicated: we can't stick the bad content inside the <table> because it would just get fostered out again.

Here are the options discussed, starting from our most preferred option, and ending with least preferred:

  1. Instead of migrating the problematic content, add a single span wrapper around it all: </table><span class=”mw-ignore-me">....</span><table>. The CSS uses sibling selectors, but they already accommodated a single anomalous sibling because of how legacy handled TemplateStyles. This conversion guarantees that we only have a single anomalous <span> in the markup, no matter how many link, meta, or templatestyles elements there may be. If this is successful, we want to revert the previous migration patch (1194292) and instead use this span wrapper consistently for the <div> case as well as the <table> case.
  2. If this approach doesn't work, then the next option is just to use the migration strategy of 1194292 which worked for <div>s, "just trying harder" to find an appropriate node. Instead of tucking it inside the table, we need to find the table > tr > td or table > tr > th that we can tuck the extra content in to avoid fostering.
  3. No one (except @cscott) wants this, but if neither of the above works, the last alternative is just to strip trailing <link>, <meta>, and whitespace inside a template. #Contents of transcluded blocks in the spec doesn't *require* that these be kept, and T419703: Parsoid should track metadata at fragment level to allow selective update will eventually provide another way to access this metadata. TemplateStyles need to be kept, but this is an issue with legacy as well and is already accomodated by the CSS (it helps that there is just one template styles element to worry about, instead of a potentially unbounded set of category links etc).

Bikeshedding the class name/marker for option #1: In T378906#11286762, I had proposed mw-category-links-container as a possible class name for the container. Alternatively, we can introduce a typeof="...". In either case, we'll either need to add a style="display:none;' OR add update CSS in core (preferred) with the right CSS selector.

Change #1261512 had a related patch set uploaded (by Isabelle Hurbain-Palatin; author: Isabelle Hurbain-Palatin):

[mediawiki/services/parsoid@master] Gather more template-generated rendering-transparent nodes into a span

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

Change #1261512 merged by jenkins-bot:

[mediawiki/services/parsoid@master] Gather more template-generated rendering-transparent nodes into a span

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

@Izno that patch now adds a span.mw-empty-elt around all the rendering-transparent-content. You should be able to use this specific span.mw-empty-elt selector to adapt the CSS. I just realized that you had previously said you preferred something more specific, but span.mw-empty-elt is the specific selector. mw-empty-elt has previously been used on p, tr, and li tags. There is no real semantic information being conveyed by the extra span wrapper beyond: this is just a convenient wrapper to hide a bunch of rendering-transparent (and effectively empty) content to get the out of the way of CSS selectors. So, span.mw-empty-elt seems reasonable enough.

Change #1277201 had a related patch set uploaded (by Subramanya Sastry; author: Subramanya Sastry):

[mediawiki/services/parsoid@master] Don't wrap nodes in fosterable positions

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

Thanks a lot for working on this!

@Izno that patch now adds a span.mw-empty-elt around all the rendering-transparent-content. You should be able to use this specific span.mw-empty-elt selector to adapt the CSS.

I will try to take a look at this soon. And by soon I mean I'll need a deploy from Parsoid to some wiki or another. I will probably look on MediaWiki wiki since that's quiet enough compared to the several million transclusions of the ambox and navbox styles on English Wikipedia.

I just realized that you had previously said you preferred something more specific, but span.mw-empty-elt is the specific selector. mw-empty-elt has previously been used on p, tr, and li tags. There is no real semantic information being conveyed by the extra span wrapper beyond: this is just a convenient wrapper to hide a bunch of rendering-transparent (and effectively empty) content to get the out of the way of CSS selectors. So, span.mw-empty-elt seems reasonable enough.

Ok, this one isn't a big deal. I do like having specific-ish names (vice CSS specificity in case that was unclear heh) because they communicate intent that (in this case) might be weird but is also to be expected. I'll have to explain in the context of the rules what is going on, since indeed .mw-empty-elt is a dime a dozen, even if span.mw-empty-elt is not.

It looks like the change in the pwrapping test takes care of another question I was going to have, which was dodging pwrapping of the span (which is normally one of the kinds of content that gets wrapped).

Did my question(s) at T378906#11287150

I guess the only concern I would have is to do with category sort keys, which are source dependent. I would guess this is a pretty rare case, but it does seem like a spot where you would want to guarantee wikitext source order insertion into the hidden span? And also, transclusions including other templates which may bring their own categories (uh, like Page transcluding Infobox which contains in Page a {{birthday}} template that adds a category for age, and the Infobox also including a category - maybe an example of this in reality for this specific question would be the {{multiple issues}} template which is a container for other amboxes, and each of those amboxes, including the multiple issues template, emit categories).

get solved during design/implementation?

  1. ... TemplateStyles.

Since it's mentioned here, does this direction also potentially fix T303378: MobileFrontend removal of elements takes <templatestyles> tags with it for Parsoid? (It's not what I was trying to get out of this task but I would guess it could be extended if it wasn't deliberately added.)

  1. No one (except @cscott) wants this, but if neither of the above works, the last alternative is just to strip trailing <link>, <meta>, and whitespace inside a template. ... TemplateStyles need to be kept, but this is an issue with legacy as well and is already accomodated by the CSS (it helps that there is just one template styles element to worry about, instead of a potentially unbounded set of category links etc).

Reminder/FYI quality of interest: Stripping any <link> emitted in the context of TemplateStyles has a task for it at T200206: Omit `<link rel="mw-deduplicated-inline-style">` from page view HTML.

@Izno What's our current status on T378906#11286730 ?

Apologies for silence here, I did see both this comment and the one from a month earlier. I felt a bit blocked on responding to the question, but finding a different solution than what was item 2 above makes that issue irrelevant. To answer it now, it has progressed little enough locally due to other commitments in the movement, and having to fight with small templates here and there that I was ambitious about ([[Template:Archives]] in this case is the current one on the list, though you can follow along for others at https://en.wikipedia.org/wiki/Module:Message_box/div/doc ), and because I did already say I didn't know what the timeline would be. And lastly, even if enwiki fixes, other wikis would have had the same issue for some arbitrary long tail period (though it seems several have not even noticed so I imagine you haven't hit the big ones that might have).

Change #1277201 merged by jenkins-bot:

[mediawiki/services/parsoid@master] Don't wrap nodes in fosterable positions

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

[did]

I guess the only concern I would have is to do with category sort keys, which are source dependent. I would guess this is a pretty rare case, but it does seem like a spot where you would want to guarantee wikitext source order insertion into the hidden span? And also, transclusions including other templates which may bring their own categories (uh, like Page transcluding Infobox which contains in Page a {{birthday}} template that adds a category for age, and the Infobox also including a category - maybe an example of this in reality for this specific question would be the {{multiple issues}} template which is a container for other amboxes, and each of those amboxes, including the multiple issues template, emit categories).

get solved during design/implementation?

I'm not going to guarantee "wikitext source order" for this because of pesky things like table fostering, footnotes, and that sort of things, which tend to wreak havoc on that. What I am REASONABLY sure of is that the current order of the corresponding HTML tags stays the same with this patch.

As far as "transclusions including other templates" - yeah, this was taken into account. The TL;DR of this patch is: "if we find category links, style tags or newline wrapper spans inside a template range, and if the group (possibly reduced to 1) of these elements is between divs or tables, or at the start or the end of said template range, we either put them at the corresponding extremity of the div or we create a span to stash them where they are."

This should guarantee that there's at most two .mw-empty-elt between two template-generated amboxes/navboxes-or-similar (if one such span ends up at the end of a template - say catlinks - and one at the beginning of the following template - say style links).

I don't believe that it directly solves any templatestyle-related issue directly; for one thing, we've limited the scope of this patch to "stuff that is/that can end up between divs & tables", so this wouldn't be a general solution for this. (I'll be honest: I definitely don't know enough about TemplateStyles to make informed decisions here; but the Parsoid rendering should make it easy to identify these nodes and add post-processing to mobile as we see fit. Grain of salt, though, since I'm not familiar with how this works/should work.)

Change #1277671 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.23.0-a29

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

Change #1277671 merged by jenkins-bot:

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.23.0-a29

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

Thanks a lot for working on this!

@Izno that patch now adds a span.mw-empty-elt around all the rendering-transparent-content. You should be able to use this specific span.mw-empty-elt selector to adapt the CSS.

I will try to take a look at this soon. And by soon I mean I'll need a deploy from Parsoid to some wiki or another. I will probably look on MediaWiki wiki since that's quiet enough compared to the several million transclusions of the ambox and navbox styles on English Wikipedia.

This is now fully deployed to all wikis and we have ironed out most of the kinks. Can you give this a go and report back?

@ihurbain added https://www.mediawiki.org/wiki/Parsoid/Parser_Unification/Instructions_for_editors#To_handle_rendering-transparent_elements_(%3Clink%3E,_%3Cmeta%3E,_%3Cstyle%3E)_in_template_output to help you all in tweaking your CSS.

@Izno: please feel free to tweak that section based on your enwiki CSS fixes so it is clearer and provides better guidance to editors on other wikis.

This is now fully deployed to all wikis and we have ironed out most of the kinks. Can you give this a go and report back?

The wrapping transform seems to work. There is a weird case though and I'm not sure it's intended.

On https://en.wikipedia.org/wiki/User:Izno/Sandbox/T378906 , the two amboxes with categories that are situated alone do not wrap their categories (but do wrap their templatestyles). When they are next to each other, they seem to wrap things as expected. Except if the categories are in the last instance as in Mix 2 full.

It's fine from the perspective of this task that these act like that I think, since this task was focused on inter-content of coincidentally-the-same-templates. It's just not consistent and may lead to weird expectations elsewhere (I couldn't say what or why or where).

I'll have to see if this affects navbox + Portal bar negatively.

On https://en.wikipedia.org/wiki/User:Izno/Sandbox/T378906 , the two amboxes with categories that are situated alone do not wrap their categories (but do wrap their templatestyles). When they are next to each other, they seem to wrap things as expected. Except if the categories are in the last instance as in Mix 2 full.

indeed, there's still an empty span after that that prevents said wrapping

let me check

Change #1295960 had a related patch set uploaded (by Isabelle Hurbain-Palatin; author: Isabelle Hurbain-Palatin):

[mediawiki/services/parsoid@master] Fix end of template detection for shouldStashRenderingTransparentNodes

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

Change #1295960 merged by jenkins-bot:

[mediawiki/services/parsoid@master] Fix end of template detection for shouldStashRenderingTransparentNodes

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