Page MenuHomePhabricator

Investigate: Grey background for temporary usernames in signatures, mentions etc
Closed, ResolvedPublic

Description

Motivation

Temporary usernames will feature a grey background when they appear in history pages, log pages etc (T325768: Design the visual look for temp usernames). For consistency, it is ideal that temporary usernames feature the same grey background when they appear in other content areas and signatures.
This task is to investigate the work needed to achieve the suggested specs below.

Suggested specs

Expected outcome

  • Technical approach to achieve the specs suggested above.

Linked tasks

Related Objects

Event Timeline

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

Signatures and mentions are both just plain wikitext links:

  • Signatures: [[Special:Contributions:~2024-403|~2024-403]]
  • Mentions: [[User:2024-403|2024-403]]

Some options in no particular order:

1. Adding <span>s to the wikitext

Theoretically signatures could be updated to include a surrounding CSS class in the substituted wikitext:

<span class="mw-tempuser">[[Special:Contributions:~2024-403|~2024-403]]</span>

but this is getting quite cluttered for wikitext readers.

Mentions could be updated at least in DiscussionTools to generate more syntax, but mentions are also manually created by users or with templates, so that wouldn't catch all cases.

2. Modifying the parser

The parsers (both Parsoid and legacy PHP parser would need to be updated) can add CSS classes based on the target of a link, for example links to pages that don't exist get the new class.

Someone from the content transform team would be better placed to comment on the complexity and feasibility of this, but it would probably be the neatest solution, as the mw-tempuser link could be added at parse-time, if the target of the link is a user/user-talk/user-contributions page where the user is temporary.

(cc @cscott, @ssastry)

3. CSS hacks

Given the current output of this wikitext:

  • Signatures:
<a href="/wiki/Special:Contributions/~2024-403" title="Special:Contributions/~2024-403">~2024-403</a>
  • Mentions:
@<a href="/wiki/Benutzer:~2024-403" title="Benutzer:~2024-403">~2024-403</a>

we could look for the fairly rare strings /~2 or :~2 in the link href:

a[ href *= "/~2" ], a[ href *= ":~2" ] {
    background-color: #eaecf0;
    padding: 2px 8px 2px 8px;
    border-radius: 2px;
}

Cons:

  • Very hacky, and maybe a bit flaky
  • Could be false positives as it will target any link that contains /<tempusername>, e.g. [[Arbcom investigation/~2024-403]]

Pros:

  • By far the easiest of the three, could be deployed now with the code above.

The GetLinkColours hook was pretty much made for this -- adding extra classes to links, based on some function of the link title (prefixed DB key). There are four places in the code where onGetLinkColours is called, you could either change all four of those places to add the extra case for temporary account, or else just directly register a 'GetLinkColours' hook. If you went the later route, you could probably avoid some code duplication by handling redirects in the same generic hook.

$ git grep '>onGetLinkColours'
includes/api/ApiQueryInfo.php:          $this->getHookRunner()->onGetLinkColours(
includes/parser/LinkHolderArray.php:                    $this->hookRunner->onGetLinkColours( $pagemap, $classes, $this->parent->getTitle() );
includes/parser/LinkHolderArray.php:            $this->hookRunner->onGetLinkColours( $pagemap, $classes, $this->parent->getTitle() );
includes/parser/Parsoid/Config/DataAccess.php:          $this->hookRunner->onGetLinkColours(

You could also refactor to add a new LinkColor.php class with a LinkColor::getLinkColors() method which handles (a) redirects, (b) temporary accounts, and then (c) calls the onLinkColour() hook, all in one place. Then replace all the places which currently call the GetLinkColours hook with calls to the new centralized LinkColor::getLinkColors() method.

There's also LinkRenderer::getLinkClasses() but that doesn't seem to be consistently called by all the places (it's not used by Parsoid, eg) -- it seems to *just* do the redirect "color", which is done in a different way by Parsoid and ApiQueryInfo. Whatever place you put the new code, you'd want to be sure that all four places that currently call the GetLinkColours hook (which includes Parsoid in its DataAccess class) also add your new 'temporary account' class.

The catch is that the existing GetLinkColors hook takes as an argument a map from page ids to page title (prefixed DB key), and Special pages (like the temporary accounts) don't have a page ID and so can't get passed to this hook. Gah.

So I think just adding special handling to the four different places which handle link colors is probably the best route: LinkHolderArray already has a special case for ns == NS_SPECIAL, so adding another if statement which tests for temporary accounts should be straightforward, then the other two places (ApiQueryInfo and DataAccess) both do special case handling for redirects already so you'd just special case handling for temporary accounts in the same place. Maybe refactor those into a common function for added points.

An alternative we discussed is adding a new Hook, "GetLinkColorsToo" (or whatever) that just inverted the map, mapping from title to pageid instead of the reverse. Then the Disambiguator extension (the only user of the GetLinkColours hook in production) could just call array_values() instead of array_keys() to get the page ids for its db query, and we could pass in *all* the links (not just "links which have pageids") to the hook. But not sure it's worth doing all of that.

From slack:

@Esanders : It looks like in the legacy parser, special page links aren't up for replacement at all

<p><!--LINK'" 0:0-->
</p><p><!--LINK'" 0:1-->
</p><p><a href="/MediaWiki/core/index.php/Special:Contributions/~2024-123" title="Special:Contributions/~2024-123">Special:Contributions/~2024-123</a>
</p><p><!--LINK'" 2:2-->
</p>

^This is the pre-regex-replace HTML
My other links (to an existing page, a non-existing page and a user page) show up as replaceable comments
Interestingly the code for Special pages adds the "new" class, which would be incorrect, but the code never runs, so not sure what it is for...

@cscott: huh, weird. Parsoid just bypassed the whole "LinkHolder" abstraction. We'll probably have to grapple with it someday, but it's a big old ball of complexity.
So: straightforward to add classes to the ApiQueryInfo class and to Parsoid -- I verified that Parsoid passes special pages through to DataAccess. Might be a little trickier to do it for the legacy parser because special page links don't go through the "normal" link color mechanism.

@Esanders : Also LinkRenderer::makeBrokenLink hard codes [ 'class' => 'new' ], so there is no way to add a CSS class to a red link....
actually.. maybe with extraAttribs...
(very hacky though)

Change 1003080 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/core@master] [WIP] Add mw-tempuserlink to signatures and mentions in Parser

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

In T325768, the appearance was specified for temp user links in history pages, which adds 2px/8px padding, and this was implemented in the .mw-tempuserlink class as the default.

While this padding works on history pages, I think we need less in content pages, as it may be adjacent to other text, e.g.

in mentions:

image.png (38×305 px, 4 KB)

and to a lesser extent in signatures:

image.png (68×444 px, 13 KB)

Discussed scope briefly with Editing team.

  • VE maintains own link cache of “link color” for newly-added links
  • If Parsoid moved link color to a read-views-only postprocessing, /probably/ VE could just use its already-existing link cache to fetch /all/ link colors, instead of only newly-added (link cache also stores disambig, redirects, known/hidden/image url, etc)
  • Wrt temporary accounts, if Parsoid added link colors for temp accounts in read-view only, it would work fine for read view and for discussion tools (which is already working on read view html), but link color wouldn’t be correct for a newly-added link to a temp account – but articles generally don’t have these anyway.
  • If these existing GetLinkColors hook was used to add a class for temp accounts, it would ‘just work’ in parsoid read views and discussion tools; the only issue would be that newly-added links in VE wouldn’t get the colors (or the VE mechanism could be taught to use the hook as well)
  • Unlike red links, the user account "color" won't change after the page is parsed.
  • T374868 is another link color bug which is unrelated (other than touching the same locations in the codebase)

This is conservatively 2 weeks of work on the CTT side, with additional time on the editing side if the correct color is wanted in VE for newly-created links. @MSantos can help with scheduling and priority.

Discussed scope briefly with Editing team.

  • VE maintains own link cache of “link color” for newly-added links
  • If Parsoid moved link color to a read-views-only postprocessing, /probably/ VE could just use its already-existing link cache to fetch /all/ link colors, instead of only newly-added (link cache also stores disambig, redirects, known/hidden/image url, etc)
  • Wrt temporary accounts, if Parsoid added link colors for temp accounts in read-view only, it would work fine for read view and for discussion tools (which is already working on read view html), but link color wouldn’t be correct for a newly-added link to a temp account – but articles generally don’t have these anyway.
  • If these existing GetLinkColors hook was used to add a class for temp accounts, it would ‘just work’ in parsoid read views and discussion tools; the only issue would be that newly-added links in VE wouldn’t get the colors (or the VE mechanism could be taught to use the hook as well)
  • Unlike red links, the user account "color" won't change after the page is parsed.
  • T374868 is another link color bug which is unrelated (other than touching the same locations in the codebase)

This is conservatively 2 weeks of work on the CTT side, with additional time on the editing side if the correct color is wanted in VE for newly-created links. @MSantos can help with scheduling and priority.

Thanks @cscott. @MSantos please let us know what you all decide in terms of schedule, prioritization, etc.

There are a couple of implementation steps here:

  1. Use existing GetLinkColors hook to add a class for temp accounts. This "just works" for everything except newly-added links in VE.
    • If temporary accounts are implemented by an extension, it is straightforward to add this hook in the extension
    • If temporary accounts are in core, then this code would need to be copied to the 5 places where onGetLinkColours is called in core (probably some refactoring possible).
  2. In order for the link color to show up *for newly added links in VisualEditor*, the editing team would need to do some additional work to integrate with their link cache.

It's not clear if 2 is needed or a priority.

There are a couple of implementation steps here:

  1. Use existing GetLinkColors hook to add a class for temp accounts. This "just works" for everything except newly-added links in VE.
    • If temporary accounts are implemented by an extension, it is straightforward to add this hook in the extension
    • If temporary accounts are in core, then this code would need to be copied to the 5 places where onGetLinkColours is called in core (probably some refactoring possible).
  2. In order for the link color to show up *for newly added links in VisualEditor*, the editing team would need to do some additional work to integrate with their link cache.

It's not clear if 2 is needed or a priority.

Newly added links in VisualEditor (and DiscussionTools?) sounds like we should split into its own, separate task, and probably is nice-to-have for now (cc @Niharika).

Discussed this with editing, and we're good with the plan above (add class in the places that call GetLinkColors, eventually VE will use action=query&prop=info&inprop=linkclasses to properly color newly-added links. (This can't replace their redlink processing, but it could replace 'redirect'.) I'll create a subtask for that.

Noting that we got some feedback in favor of doing this:

temp accounts are easily identifiable in histories but read like normal users on talk pages; trolls could create fake profiles with fake letters

Discussed this with editing, and we're good with the plan above (add class in the places that call GetLinkColors, eventually VE will use action=query&prop=info&inprop=linkclasses to properly color newly-added links. (This can't replace their redlink processing, but it could replace 'redirect'.) I'll create a subtask for that.

@MSantos @cscott what is the timeline for this task?

In addition to the grey backgrounds, we also need to be able to support:

  • the "Show IP" button
  • IP auto-reveal mode
  • and support needed for T396709#11185798 -- in that task, we need to render the HTML used for the CheckUser-UserInfoCard button (hidden via CSS by default)

Would the proposed changes help us with those scenarios?

This was being implemented in T392775 and patches got merged. They were later reverted in Revert "Add user-related link colors to LinkRenderer::getLinkClasses" (1167244) · Gerrit Code Review due to T398714: "Show IP" appearing twice, T398717: IPInfo button only appears for the first temporary account, T398952: Inconsistent/confusing styles for temporary account links.

tl;dr it turned out that adding user-related classes to all user-related links caused tool breakage. A lot of non-article pages (histories, tools, special pages) use the same LinkRenderer methods which code uses to render links in wikitext.

It's possible that a narrower patch could be made to add classes to user links specifically in Parsoid-generated wikitext, and not more generally.

Given that T398952#10994965 implies that the coloring should be done not only based on the link target, but also the text content of the link, I'm thinking if the approach from https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1139193 could be extended so that LinkRenderer::getLinkClasses accepts a second, optional parameter for the link label, and the gray background is based on its value. The text would be passed from make{Known,Broken}Link(), which already have access to it. Is this feasible?

I think this approach shouldn't cause the side effects that appeared originally (T398714, T398717, T398952), because none of these places used link text equal to the temp. username.

Given that T398952#10994965 implies that the coloring should be done not only based on the link target, but also the text content of the link, I'm thinking if the approach from https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1139193 could be extended so that LinkRenderer::getLinkClasses accepts a second, optional parameter for the link label, and the gray background is based on its value. The text would be passed from make{Known,Broken}Link(), which already have access to it. Is this feasible?

I think this approach shouldn't cause the side effects that appeared originally (T398714, T398717, T398952), because none of these places used link text equal to the temp. username.

I think this makes sense to try.

There may be some edge cases where we'd be adding the class to links that shouldn't have it, which we're picked up last time due to being more obscure (though I can't think what these would be). If there are, we may be able to fix those via the IP reveal software in CheckUser.

Given that T398952#10994965 implies that the coloring should be done not only based on the link target, but also the text content of the link, I'm thinking if the approach from https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1139193 could be extended so that LinkRenderer::getLinkClasses accepts a second, optional parameter for the link label, and the gray background is based on its value. The text would be passed from make{Known,Broken}Link(), which already have access to it. Is this feasible?

I think this approach shouldn't cause the side effects that appeared originally (T398714, T398717, T398952), because none of these places used link text equal to the temp. username.

I think this makes sense to try.

There may be some edge cases where we'd be adding the class to links that shouldn't have it, which we're picked up last time due to being more obscure (though I can't think what these would be). If there are, we may be able to fix those via the IP reveal software in CheckUser.

I'll prepare such implementation, then. (But I'll use T392775: Add link color for temporary usernames in content and discussion pages to track the actual engineering work).

(btw, should we move this task to a differet column than Ready?)

I'll prepare such implementation, then. (But I'll use T392775: Add link color for temporary usernames in content and discussion pages to track the actual engineering work).

(btw, should we move this task to a differet column than Ready?)

@mszwarc If the technical approach has been identified we can close this task. I think we can use T392775 for adding the highlight (I'll add more details to it) and I can spin up other tasks to 1) support show IP and auto-reveal and 2) support user info card.
Does that sound okay?

mszwarc claimed this task.

@mszwarc If the technical approach has been identified we can close this task. I think we can use T392775 for adding the highlight (I'll add more details to it) and I can spin up other tasks to 1) support show IP and auto-reveal and 2) support user info card.
Does that sound okay?

Sounds good

Change #1003080 abandoned by Hashar:

[mediawiki/core@master] Add mw-tempuserlink to signatures and mentions in Parser

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

Change #1003080 restored by Thcipriani:

[mediawiki/core@master] Add mw-tempuserlink to signatures and mentions in Parser

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

Change #1003080 abandoned by Bartosz Dziewoński:

[mediawiki/core@master] Add mw-tempuserlink to signatures and mentions in Parser

Reason:

The associated task is resolved, so I think this is no longer needed.

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