Page MenuHomePhabricator

Remove non-critical path interface styles out of legacy feature into more appropriate homes
Closed, ResolvedPublic

Description

The following rules are generated by interfaces that use the Linker class (e.g. Linker::revComment, Linker::formatComment, Linker::revDeleteLink, Special:Undelete, Linker::userLink, Special:RevisionDelete.

Checklist

  • One possible home for these might be mediawiki.interface.helpers.styles making sure that module is loaded on the Special:Undelete and Special:RevisionDelete pages.
span.comment {
	font-style: italic;
	unicode-bidi: -moz-isolate;
	unicode-bidi: isolate;
}

li span.deleted,
span.history-deleted {
	text-decoration: line-through;
	color: #72777d;
	font-style: italic;
}


/* The auto-generated edit comments */
.autocomment,
.autocomment a,
.autocomment a:visited {
	color: #72777d;
}

span.mw-revdelundel-link,
strong.mw-revdelundel-link {
	font-size: 90%;
}
span.mw-revdelundel-hidden,
input.mw-revdelundel-hidden {
	visibility: hidden;
}
  • and...
td.mw-revdel-checkbox,
th.mw-revdel-checkbox {
	padding-right: 10px;
	text-align: center;
}
  • The following are used on Special:NewPages. Suggested > mediawiki.special/newpages.less
.not-patrolled {
	background-color: #ffa;
}
.unpatrolled {
	font-weight: bold;
	color: #d33;
}
div.patrollink {
	font-size: 75%;
	text-align: right;
}
  • Used on changelist. Suggested > mediawiki.special.changeslist/changeslist.less
.mw-input-with-label {
	white-space: nowrap;
	display: inline-block;
}
.newpage,
.minoredit,
.botedit {
	font-weight: bold;
}

Event Timeline

Change 675194 had a related patch set uploaded (by Mainframe98; author: Mainframe98):
[mediawiki/core@master] Move several legacy style rules to existing dedicated modules

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

For the first block of style rules, the problem (excluding .mw-revdel-checkbox) is that they can appear everywhere because they are also generated by Linker (and some by Xml). Simply adding them to mediawiki.interface.helpers.styles won't be enough because there are extensions not loading that, and calls to Linker can occur from anywhere.

Could Linker add the stylesheet?

I see Linker::generateRollback adds a module to the page (but.. that has access to context):

			$context->getOutput()->addModules( 'mediawiki.misc-authed-curate' );

if the concern is breaking extensions, and updating callers, we can manage that separately by making sure legacy continues to import these styles. We'll have plenty of time to update extensions before removing the legacy feature.

Change 675194 merged by jenkins-bot:
[mediawiki/core@master] Move several legacy style rules to existing dedicated modules

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

Jdlrobson updated the task description. (Show Details)

Let's worry about migrating code separately. I suggest moving the rules to better homes and marking the rules as duplicates in legacy.less with a comment. We can use T278896 to catch any regressions.

Change 675991 had a related patch set uploaded (by Mainframe98; author: Mainframe98):

[mediawiki/core@master] Move more legacy styles to existing modules

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

Change 675991 merged by jenkins-bot:

[mediawiki/core@master] Move more legacy styles to existing modules

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

@Mainframe98 what did we decide to do with the remaining styles? Does mediawiki.interface.helpers.styles make sense?

I'd like to find a better home for this in 1.36 and then during in 1.37 as we move skins away from legacy, we'll make sure that new stylesheet is added to all the appropriate places.

Change 681138 had a related patch set uploaded (by Mainframe98; author: Mainframe98):

[mediawiki/core@master] Move most remaining legacy styles to interface.helpers.styles

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

@Mainframe98 what did we decide to do with the remaining styles? Does mediawiki.interface.helpers.styles make sense?

I don't recall a decision, but that makes sense.

I'd like to find a better home for this in 1.36 and then during in 1.37 as we move skins away from legacy, we'll make sure that new stylesheet is added to all the appropriate places.

With the patch above excluded, remaining are: directionality styles, a.new (the old colour), .error, .warning, .success, .xdebug-error. Everything else has either been moved, or been deprecated.

Change 681138 had a related patch set uploaded (by Mainframe98; author: Mainframe98):

[mediawiki/core@master] Move most remaining legacy styles to interface.helpers.styles

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

This change is a bit iffy though. I can't find mw-revdelundel-hidden anywhere, suggesting that that style rule is obsolete. That's easily resolvable, the bigger issue is the styles used by Linker. Most uses do include mediawiki.interface.helpers.styles, but a lot of it is layered and really difficult to find out if they receive the right styles. We'd best coordinate with User-notice/Tech news to make sure we have everything.

With the patch above excluded, remaining are: directionality styles, a.new (the old colour), .error, .warning, .success, .xdebug-error. Everything else has either been moved, or been deprecated.

Agreed. No worries about those.

This change is a bit iffy though. I can't find mw-revdelundel-hidden anywhere, suggesting that that style rule is obsolete.

Let's consider this deprecated then as I'd rather not inherit undocumented code. We will provide a User-notice as we transition away from legacy in different skins (probably starting with Monobook). Nothing will break until that happens.

Jdlrobson claimed this task.

(handling code review)

Change 681369 had a related patch set uploaded (by Jdlrobson; author: Mainframe98):

[mediawiki/core@REL1_36] Move most remaining legacy styles to interface.helpers.styles

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

Change 681369 merged by jenkins-bot:

[mediawiki/core@REL1_36] Move most remaining legacy styles to interface.helpers.styles

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

Change 681138 merged by jenkins-bot:

[mediawiki/core@master] Move most remaining legacy styles to interface.helpers.styles

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

Change 681488 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Create the `content-links` ResourceLoaderSkinModule feature

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

I'm revisiting the links inside elements based on the regression in T279693.

This change is a bit iffy though. I can't find mw-revdelundel-hidden anywhere, suggesting that that style rule is obsolete. That's easily resolvable, the bigger issue is the styles used by Linker. Most uses do include mediawiki.interface.helpers.styles, but a lot of it is layered and really difficult to find out if they receive the right styles. We'd best coordinate with User-notice/Tech news to make sure we have everything.

Let's consider this deprecated then as I'd rather not inherit undocumented code. We will provide a User-notice as we transition away from legacy in different skins (probably starting with Monobook). Nothing will break until that happens.

I searched the Git history and the only use of that CSS class has been removed in be057fb89e337055d2b229503e8e1f7d57dd2411 (2009). I submitted https://gerrit.wikimedia.org/r/c/mediawiki/core/+/681587 to remove the styles.

phuedx removed Edtadros as the assignee of this task.
phuedx moved this task from Code Review to QA on the Web-Team-Backlog (Kanbanana-FY-2020-21) board.
phuedx added a subscriber: Edtadros.
phuedx subscribed.

@Edtadros: Before I merged @Jdlrobson's patch, I ran a visual regression test against the top 10 pages by pageview and all special pages for both the Vector and Timeless skins. If that's enough, then LMK. If you'd like me to take you through how I ran the test, then I'll set up a 30 minute meeting.

@Jdlrobson: If there are specific instructions to test your patch, then could you put them in the description?

Change 681488 merged by jenkins-bot:

[mediawiki/core@master] Create the `content-links` (and -external) ResourceLoaderSkinModule feature

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

Most of this should be a no-op, so provided we've QAed the Timeless issue I think this should be enough.