Page MenuHomePhabricator

Add Parsoid-compatible <link> tag to legacy parser output for redirects
Closed, ResolvedPublic

Description

On redirects, there is HTML of the following form (from WoW):

<div class="redirectMsg">
  <p>Redirect to:</p>
  <ul class="redirectText">
    <li><a href="/wiki/World_of_Warcraft" title="World of Warcraft">World of Warcraft</a></li>
  </ul>
</div>

Why is this a list? Redirects will never point to more than one page.

Separately noticed as part of looking at this: .redirectMsg p is being hidden in a way that is probably inaccessible and could/should probably use standard .sr-only or similar.

Credits to @stjn for seeing both.


Event Timeline

Izno updated the task description. (Show Details)

Some skins do style the redirect arrow e.g. for dark mode, which appears to be inserted with CSS. So that will need to be reviewed as part of a change.

Night mode compatibility in particular is achieved by more cursed means in CSS (invert via filter and then invert the link back):
https://gerrit.wikimedia.org/g/mediawiki/core/+/49b9cc4876b1f68366483fe7c424b09e06fa882d/resources/src/mediawiki.action/mediawiki.action.view.redirectPage.less#40

I guess having a separate element for the icon so it could be styled once and separately would be better.

Judging by code search, there are a number of tests that depend on having <div class="redirectMsg"> in code, so that part should be left unchanged. Other than that, HTML should be fine to adjust.

Also, currently Minerva and Vector 2022 have ‘Redirect page’ subtitle while other skins do not (weirdly, I don't see anything to suggest this is intentional). Maybe in the vein of UI-Standardization we should not hide ‘Redirect to’ part at all in all skins and ‘Redirect page’ subtitle can be instead removed from those two skins. Seems like a helpful tip to display everywhere.

Change #1094057 had a related patch set uploaded (by Saint Johann; author: Saint Johann):

[mediawiki/core@master] Improve semantics of redirect HTML

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

Also, currently Minerva and Vector 2022 have ‘Redirect page’ subtitle while other skins do not (weirdly, I don't see anything to suggest this is intentional). Maybe in the vein of UI-Standardization we should not hide ‘Redirect to’ part at all in all skins and ‘Redirect page’ subtitle can be instead removed from those two skins. Seems like a helpful tip to display everywhere.

This turned out to be some sort of oversight. It was previously in all skins, see Web Archive link. The patch above just tries to make ‘Redirect to’ message visible and deprecate the subtitle like suggested in the comment.

Why is this a list? Redirects will never point to more than one page.

There used to be an option that allowed "redirect chains", in which case there could actually be more than one item in the list. This was removed in T296430: Remove support for $wgMaxRedirects, so the list markup no longer makes sense.

So, what currently complicates this task is that a number of tests depend on <div class="redirectMsg">-style markup to know if they are handling redirects correctly. I think it would be better to move away from this by making markup independent of classes and tags, for example, typeof="mw:RedirectPage" or something. Then that markup could be added separately first and the tests could be adjusted so this doesn’t interfere with this work necessarily. I raised this possibility in the attached patch but introducing such an attribute should probably be coordinated with some team.

Yeah, if you try to make typeof attributes a convention outside of Parsoid, there are probably some teams who will want to be consulted about that. That seems like a big distraction (to you and everyone else).

Instead, I think you could more easily do something slightly different – just add a somewhat more unique/identifiable class than redirectMsg to the markup, and then adjust those tests to check for the presence of that class name anywhere in the HTML, instead of the exact markup. At a glance the tests don't really care about the markup, they just want to confirm that a redirect content was displayed.

You could be even lazier smarter than that, and just use the redirectMsg class in that check, or maybe the localisation message that is used in the redirect content. I think that would also be okay.

Well, the problem with current way the tests are written is that they rely on class="redirectMsg" specifically. So a separate attribute (and one that cannot be added by users on-wiki) seems like a natural way to discourage people from doing that.

It's a list because 20 years ago we used a # prefix for #REDIRECT and also used # to indicate an ordered list. The list is an artifact of processing this "as wikitext" in addition to the special-case code which recognizes this as a redirect. We've done this for 20+ years though, I'm not sure why we need to change it now.

Parsoid uses a <link rel="mw:PageProp/redirect"> tag here to indicate the redirect, given that we are moving all wikis to Parsoid in the next ~two years, it would make more sense just to rewrite the tests to look for the <link> tag. I'd like to tag this 'won't fix' for the legacy parser.

(See for example https://en.wikipedia.org/wiki/H2O?redirect=no&useparsoid=1 )

The list part was explained by @matmarex already: previously wikitext supported defining multiple redirect targets so list made sense before. Checking for <link> tag is fine, though then it needs to be added into HTML for legacy parser without duplication for Parsoid. If you know a way to do this, please tell me.

As far as I can tell, Parsoid does not do its own rendering of redirect pages apart from adding that tag, so fixing this in legacy parser would fix it in Parsoid. Are you sure the handling is in any way different for Parsoid? Changes introduced in patch also work the same in Parsoid: https://patchdemo.wmcloud.org/wikis/d0ef51c904/wiki/Redirect_example?useparsoid=1

I'd like to tag this 'won't fix' for the legacy parser.

Also, this would prevent further fixes to redirect page HTML down the line, like T385340, so I really don’t think this should be tied to Parsoid switchover, especially since 1) tests currently do not depend on Parsoid and 2) Parsoid as it seems does not have any special handling for redirect pages apart from what is present in legacy parser. So I’ll try to instead add <link rel="mw:PageProp/redirect" href="./Water" id="mwAg">-style markup to non-Parsoid HTML and rewrite tests so that, when the time comes, Parsoid team can transition to using whatever markup is desirable.

Change #1117590 had a related patch set uploaded (by Saint Johann; author: Saint Johann):

[mediawiki/core@master] Add <link rel="mw:PageProp/redirect"> to redirects in legacy parser

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

@ssastry and I discussed this and we're fine with this approach (patching core to emit <link> for forward-compatibility with Parsoid output).

We have a patch from @stjn but it fails CI and hasn't been updated in 2 months. I'm going to put on our WIP board for now to see if we can push this over the finish line.

cscott renamed this task from Redirect page text is a list and shouldn't be to Add Parsoid-compatible <link> tag to legacy parser output for redirects.Apr 11 2025, 5:57 PM

Change #1117590 merged by jenkins-bot:

[mediawiki/core@master] Add <link rel="mw:PageProp/redirect"> to redirects in legacy parser

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

I'm a bit confused about the status of this task now. Is it just about adding the <link> tag now (and presumably resolved), or still also about replacing the <li> tag, like it was originally filed?

Rescoped to just adding the <link> tag. (Not quite resolved, as this patch hasn't got out on the train yet.)

I don’t think it should’ve been rescoped, since adding the link tag was only part of the task intended to unblock further work on its original description. (Thanks for pushing it forward, btw!) The task description still is primarily about unnecessary list markup, so it makes more sense to restore the previous name (or a similar one, like Improve redirect HTML markup semantics).

I don't have much appetite for changing the markup for redirects -- it seems likely to break a bunch of existing tools, gadgets, etc for a purely pedantic improvement in HTML semantics. So I will probably untag Content-Transform-Team once the current patch is deployed.

Yeah, I understand that part. As far as breaking tools or gadgets goes, the plan was always to keep the legacy classes, so any disruption should be minimal unless they explicitly were relying on ul.redirectMsg etc. (which I doubt anyone really does).

Currently
https://en.wikipedia.org/w/index.php?title=H2O&redirect=no&useparsoid=1 contains <link rel="mw:PageProp/redirect" href="./Water" id="mwAg"> and
https://en.wikipedia.org/w/index.php?title=H2O&redirect=no&useparsoid=0 contains <link rel="mw:PageProp/redirect">

It appears that the href part wasn't part of the original request? I feel like perhaps it should be added just for consistency with Parsoid?

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

[mediawiki/core@master] Add `href` attribute to `<link rel="mw:PageProp/redirect">` in legacy parser

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

Change #1148913 merged by jenkins-bot:

[mediawiki/core@master] Add `href` attribute to `<link rel="mw:PageProp/redirect">` in legacy parser

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