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

Izno attached a referenced file: F57674898: image.png. (Show Details)
Izno updated the task description. (Show Details)
Izno updated the task description. (Show Details)

Looks like the CSS has been designed assuming category and other such metadata markers will be stripped from output which is a bad assumption for Parsoid. Anyway, this one is tricky. Parsoid emits explicit markup for metadata found in wikitext. For top-level pages, this is essential for accurate roundtripping. For templated content, this is essential to ensure that we treat page fragments as true representatives of what a template outputs. Without that, things like selective updates and other interventions will not work because in order to update the page-level metadata, we need to accumulate metadata from all generated fragments by walking the DOM. We can think of alternative representations where metadata is stored out-of-band, but it complicates logic because we have to look at different places and do different things for top-level and non-top-level content, so it is non-ideal.

Alternatively, we would have to add OutputTransforms for page views that strip these link tags -- T272331: Evaluate and recommend strategies for ensuring Parsoid HTML payload doesn't degrade performance in low-resource contexts. was investigating that in a different context. So, that ensures that these are only stripped on read view paths but the canonical Parsoid output itself isn't changed.

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.

@lzno what's our current status?