Page MenuHomePhabricator

Watchlist tabs don't have semantically specific IDs or classes
Open, MediumPublic

Description

Each of the watchlist tabs (View watchlist / Edit watchlist / Manage labels / Edit raw watchlist / Clear watchlist) can be targeted by a selector only through a ca-special-specialAssociatedNavigationLinks-link-n ID or :nth-child().

<li id="ca-special-specialAssociatedNavigationLinks-link-0" class="selected vector-tab-noicon mw-list-item">
	<a href="/wiki/Special:Watchlist">
		<span>View watchlist</span>
	</a>
</li>
<li id="ca-special-specialAssociatedNavigationLinks-link-1" class="vector-tab-noicon mw-list-item">
	<a href="/wiki/Special:EditWatchlist">
		<span>Edit watchlist</span>
	</a>
</li>
<li id="ca-special-specialAssociatedNavigationLinks-link-2" class="vector-tab-noicon mw-list-item">
	<a href="/wiki/Special:WatchlistLabels">
		<span>Manage labels</span>
	</a>
</li>
<li id="ca-special-specialAssociatedNavigationLinks-link-3" class="vector-tab-noicon mw-list-item">
	<a href="/wiki/Special:EditWatchlist/raw">
		<span>Edit raw watchlist</span>
	</a></li>
<li id="ca-special-specialAssociatedNavigationLinks-link-4" class="vector-tab-noicon mw-list-item">
	<a href="/wiki/Special:EditWatchlist/clear">
		<span>Clear watchlist</span>
	</a>
</li>

I had #ca-special-specialAssociatedNavigationLinks-link-3 { display: none; } to hide "Clear watchlist", but this now hid "Edit raw watchlist" instead with the addition of "Manage labels". As long as the tabs only have these serial-number IDs, this sort of thing is bound to happen again. Please add a semantic and unique ID or class to each tab.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Thanks for raising this. This is the first time where we've added a new item so clearly this wasn't thought out too much. What if the IDs were derived from the title?

e.g. we change this line https://github.com/wikimedia/mediawiki/blob/50216b3986088cc8853a10beaf17253acbb3675a/includes/Skin/SkinTemplate.php#L1509

to

			$id = 'special-specialAssociatedNavigationLinks-link-' . str_replace( '/', '-', $relatedTitle->getText() );
			$specialAssociatedNavigationLinks[$id] = [

This would replace IDs with IDs tied to the name e.g. ca-special-specialAssociatedNavigationLinks-link-EditWatchlist-raw

The downside of this is for pages where special page subpage can be any title this would mean you'd need to rely on attribute selectors, but I assume that's okay? https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Selectors/Attribute_selectors

The downside of this is for pages where special page subpage can be any title this would mean you'd need to rely on attribute selectors, but I assume that's okay?

Sounds okay, though I don't exactly understand what you mean. Do you have an example? The lis all have the same attributes except the ID and selected, so there's no point using an attribute selector (unless you're willing to use :has(), but that's the case already anyway).

I'm proposing:

  • ca-special-specialAssociatedNavigationLinks-link-0 would become ca-special-specialAssociatedNavigationLinks-watchlist
  • ca-special-specialAssociatedNavigationLinks-link-1 would become ca-special-specialAssociatedNavigationLinks-editwatchlist
  • ca-special-specialAssociatedNavigationLinks-link-2 would become ca-special-specialAssociatedNavigationLinks-watchlist-labels
  • ca-special-specialAssociatedNavigationLinks-link-3 would become ca-special-specialAssociatedNavigationLinks-editwatchlist-raw
  • ca-special-specialAssociatedNavigationLinks-link-4 would become ca-special-specialAssociatedNavigationLinks-link-editwatchlist-clear

How are those "pages where special page subpage can be any title"? What you're proposing is precisely what this task is asking for; I'm just confused about the "downside of this is" bit.

That's still an improvement to the status quo, isn't it. It does sound like allowing, if not requiring, specifying a class or classes when registering an "associated navigation link" would be a more robust approach, however.

@Jdlrobson So you're not proceeding with your proposal, which I said is still an improvement to the status quo?

I am recommending we proceed with my proposal and am saying I'd welcome a patch here.

From my experience as the primary maintainer I find it usually gets bugs fixed faster if others provide it (I cant merge my own patches). Also given this is not urgent to fix it seems like a good opportunity to grow maintainers in this area of the codebase (which doesn't have many). Hope that makes sense?