Page MenuHomePhabricator

It should be possible for a skin to share interface and skinStyles with another skin.
Closed, DeclinedPublicBUG REPORT

Description

Currently Vector operates in two different modes - Classic mode and modern mode.

In the future, it makes sense to separate these two experiences into two different skins.

One challenge of doing this is any attempt to rename the skin key, will result in existing skin styles and existing site styles from no longer applying.

While the site style is a minor inconvenience (the site admins can copy across to the new page), the skin styles is a bit more problematic. While we could update multiple repos that would be inefficient and error prone as we don't have capacity to test all 3rd party extensions.

It's proposed that a skin can register a skinStyles key they would like to inherit from:

Example:

"skinStyles": {
  "default": "default.css",
  "+vector": "vector.common.css",
   "+vector-modern": "vector.modern.css",
   "+vector-classic": : "vector.classic.css"
}

Vector classic would get vector + vector-classic + default.
Vector modern would get vector + vector-modern + default.

Ideally this would be registered in skin.json some how.

e.g.

	"SkinStyleKeys: {
		"vector-modern": [ "vector", "vector-modern" ],
		"vector-classic": [ "vector", "vector-classic" ]
	},

Event Timeline

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

The skin version is a non-standard concept local to Vector, one which I believe is already breaking expectations at various levels and a source of tension between software components. The proposed solution would not work as-is since there'd be no way for the module bundle context to consider "current user" preferences or "current page view" query parameters.

The feature you describe essentially exists already. This is what skinStyles is for. Vector currently registers its two variants as one skin with non-standard runtime switching between them, instead of informing MediaWiki natively about these variants as separate skin IDs. It would be my recommendation to remove the non-standard skinversion logic in favour of using the native affordances for this.

Note that this in no way has to change anything about T234907, and where or how the code is structured. It's entirely valid to have multiple entries in skins.json / ValidSkinNames. E.g. one would be "vector" and one would be "vector-modern". This would fix numerous current and future bugs around switching and caching, a well as add support for resourceloader features like the ones asked for here. Apart from that, I believe it would have two other fairly minor observable differences: 1) gadgets and user scripts would see them as different, and 2) the user preferences panel would by default render them as 2 radio buttons rather than one radio button with a temporal checkbox.

Regarding gadgets, it would mean gadget developers can choose to build gadgets targetting only Modern Vector. Though for existing gadgets it'd be trivial to add modern-vector to the handful of existing skin-limited gadgets with a one-time find/replace operation.

Regarding user scripts, it would mean users switching to Modern Vector get a fresh start with personal vector.js scripts. We've done this before during the original Monobook>Vector deployment, and this was quite useful I think. It avoided a lot of problems with already-broken code, or code that wasn't wanted by users who copied something there years ago. And for places where differences are needed, users could copy it over on their own terms and update it as-needed whilst still being able to easily switch back from time to time between periods of trying out the new skin. I expect this to be of more value now, as Modern Vector is still in development and has already had significantly more breaking changes than Monbook-Vector had.

Regarding preferences, I don't know if the current checkbox approach is a natural result of the skinversion implementation, or an intentional approach that design/product would prefer even on its own merit if the internals were different. If the latter, I'm sure we could make present them in the same manner as today.

I think we're on the same page that skin version is non-standard and we want to get away from that and that there are lots of benefits from doing that. No need to go into details.

The feature you describe essentially exists already. This is what skinStyles is for. Vector currently registers its two variants as one skin with non-standard runtime switching between them, instead of informing MediaWiki natively about these variants as separate skin IDs. It would be my recommendation to remove the non-standard skinversion logic in favour of using the native affordances for this.

I think you may have misunderstood or overlooked the ask here?

The main blocker for us using 2 different skin keys is that there are dozens of extensions (third party as well as Wikimedia) that provide styling for vector using the skinStyles key which currently has to be unique per skin.

We don't want to lose those or have the disruption of having to migrate those all to apply to the new skin.

What we need is something more aligned with SkinOOUIThemes (for example both MonoBook and Apex share the same theme key). Both Vector skins want to share the same "theme" (in this case skinStyle key "vector") yet still have unique skin keys. The existing SkinStyles system is not flexible enough to support this.

It's not a problem sharing the same key. As I've noted, it's okay for us to have two skin ''keys'' e.g. vector and vector2022

Take Echo for example.
https://github.com/wikimedia/mediawiki-extensions-Echo/blob/master/extension.json#L178

We need that style to apply to both skins. We do not have the resourcing to update all our production skins to duplicate that entry to apply to 2 different skin keys.

What are the challenges with making ResourceLoader's skin styles work more like SkinOOUIThemes ? My assumption is that the SkinStyle key could be declared in skin.json, falling back to the skin key if not present and looked up inside ResourceLoader.php via a static function. Am I underestimating the work here?

"skinStyles": {
  "default": "default.css",
  "+vector": "vector.common.css",
   "+vector-modern": "vector.modern.css",
   "+vector-classic": "vector.classic.css"
}

What you're looking for is:

"skinStyles": {
  "default": "default.css",
  "+vector-modern": "vector.modern.less",
  "+vector": "vector.classic.less"
}

These Less files can internally include the same common resource, that's entirely up to you. Or if prefer ResourceLoader do the concatenation for you, you can also set it as "+vector-modern": [ "common.css", "modern.css" ].

We do not have the resourcing to update all our production skins to duplicate that entry to apply to 2 different skin keys.

I don't understand this. What's the work? It'd be as simple as the various other find/replace operations we do all the time for the various deprecations and breaking changes. Certainly doesn't seem worth it to invent a new layer of complexity to build, teach, document, debug, and maintain indefinitely when our existing primitives cover this quite well already.

Modern Vector is the default skin in production for various wikis already (including French). Changing a skin key for either of the skins is going to be disruptive. While the fix is "trivial" in the sense that it probably requires an editor to change a key from vector to the new thing, we've been trying to avoid creating work for users as part of this project. There is an expectation that the new Vector is the old Vector evolved, not a new skin.

Given Modern Vector is going to see continued development, the plan would be to eventually break out the classic version into its own skin but in the interim, the two skins will live in the same repo. We plan for new keys for both skins as one of the goals of the project is to minimize disruption to editors by applying existing gadgets/scripts/styles to both skins.

I don't understand this. What's the work?

It really comes down to managing risk. While we can work around this tasks being declined, it just creates more work for us on the long term and more opportunities for manual error. I have no experience of writing find and replace scripts in Gerrit and no idea where to find such ones. I'm more likely to do this manually. It also goes without saying that the more work involved in migration, the longer the timeline of it actually happening.

What might be a good compromise is having a hook that allows us to copy any skinStyles definitions for "vector" to the new skin keys to do this programatically. Is that feasible?

I've captured the proposed work in T291098 for better visibility. It contains three stages to support our QA process around ensuring mission-critical instrumentation is working.

Krinkle closed this task as Declined.EditedOct 1 2021, 1:08 AM

I don't understand the full scope of this as you do, but I think some of those problems might have simpler solutions that would avoid extra work or risk. I'll keep an eye on T291098 and chime in there as needed.

I'm going to decline this task however. As I understand it, the work needed for skinStyles has two possible outcomes:

  1. We introduce a novel concept to map "real skins" to "fake skins" and complicate things in many places creating a situation with "no obvious problems" as opposed to "obviously no problems". E.g. how gaps are handled, and where else these fake skin keys may or may not be applicable or expected. How they are validated. Where they are registered. How the CDN cached backends would know which value to use given we can't read pageview or user preferences. In this case, the work for you is editing a handful of skin.json and extension.json files to add a "+vector-modern" key.
  1. We don't. In this case, the work for you is editing a handful of skin.json and extension.json files to set a "vector-modern" key – after that becomes a recognised skin.