Page MenuHomePhabricator

Add a Vector skin version preference
Closed, ResolvedPublic5 Estimated Story Points

Description

Background

As spiked in T237561, add a Vector skin user preference:

  • Add a configuration for controlling whether to display the new Vector preferences section or not, something like $wgVectorShowPreferences. This flag will initially be used to prevent the legacy option from being presented until some new user visible functionality is actually available in Vector. The initial rollout to test wikis is planned to coincide with the header and logo changes. The default skin value should be true so that third-parties get the functionality by default. A patch to mediawiki-config should be made to set the default to false on all projects and all wikis.
  • Add a configuration for controlling the default Vector version preference, something like $wgVectorDefaultSkinVersion. When determining whether to display legacy Vector (version '1') or modern Vector (version '2'), the value of $wgVectorDefaultSkinVersion will be used when the user has not made a preference selection. The default skin value should be 2 so that third-parties get the modern functionality by default. A patch to mediawiki-config should be made to set the default to 1 on all projects and all wikis.
  • Add a configuration for controlling the default Vector version preference for new accounts, something like $wgVectorDefaultSkinVersionForNewAccounts. This configuration will be used to control the skin version set for newly created accounts. The default skin value should be 2 so that third-parties get the modern functionality by default. A patch to mediawiki-config should be made to set the default to 1 on all projects and all wikis.
  • Add hooks:
    • onGetPreferences(): This hook will actually show (or not show) the preference. The preference should appear in a new section below the skins section. The whole section should be shown when the Vector skin is selected under skin preferences and $wgVectorShowPreferences is true, and be hidden otherwise. An example showing / hiding just a single checkbox:
onGetPreferences()
private const SKIN_PREFERENCES_SECTION = 'rendering/vector';

/**
  * Add Vector preferences to the user's Special:Preferences page directly underneath skins.
  *
  * @param User $user User whose preferences are being modified.
  * @param array[] &$prefs Preferences description array, to be fed to a HTMLForm object.
  */
public static function onGetPreferences( User $user, array &$prefs ) {
  // [todo] early exit if not configured to show Vector preferences.
  $pref = [
    'type' => 'toggle',
    'label-message' => 'skinname-vector-1',
    'help-message' => 'vector-1-skin-desc',
    'section' => self::SKIN_PREFERENCES_SECTION,
    'hide-if' => [ '!==', 'wpskin', 'vector' ]
  ];

  $skinPosition = array_search( 'skin', array_keys( $prefs ) );
  $vectorPrefs = [
    Constants::PREF_KEY_SKIN_VERSION => $pref,
  ];

  if ( $skinPosition !== false ) {
    $prefs = array_splice( $prefs, $skinPosition + 1, 0, $vectorPrefs );
  } else {
    $prefs += $vectorPrefs;
  }
}

Which looks like:

Shown (Vector skin selected):

Screenshot_2020-01-07 Preferences - box.png (1×1 px, 358 KB)

Hidden (Non-Vector skin selected):

Screenshot_2020-01-07 Preferences - box(1).png (1×1 px, 334 KB)

  • onUserGetDefaultOptions(). Something like:
onUserGetDefaultOptions()
/**
 * Called whenever a user wants to reset their preferences.
 *
 * @param array &$defaultOptions
 */
public static function onUserGetDefaultOptions( array &$defaultOptions ) {
	/** @var Config $config */
	$config = MediaWikiServices::getInstance()->getService( Constants::SERVICE_CONFIG );
	$default = $config->get( Constants::CONFIG_KEY_SKIN_VERSION ); // [todo] actually use the configuration for user default value here.
	$defaultOptions[ Constants::PREF_KEY_SKIN_VERSION ] = $default;
}
  • onLocalUserCreated(): E.g.:
onLocalUserCreated()
/**
 * Called one time when initializing a users preferences for a newly created account.
 *
 * @param User $user Newly created user object.
 * @param bool $isAutoCreated
 */
public static function onLocalUserCreated( User $user, $isAutoCreated ) {
	$user->setOption( Constants::PREF_KEY_SKIN_VERSION, Constants::SKIN_VERSION_MASTER ); // [todo] use config for new account default value here.
}
  • onResourceLoaderGetConfigVars(): Present the configs as needed to the client. At least $wgVectorSkinVersion will be needed. See PopupHooks.php for an example. onMakeGlobalVariablesScript() may also be needed.

As an aside and not a requirement, additional modern Vector-only options may appear here. That is, these are options that would only appear if legacy mode was disabled. These could use something like 'hide-if' => [ '!==', 'wpskinVersion', '1' ] to control whether they appear in this section or not.

AC

todo: get copy and confirm mediawiki-config default values with Olga. Do we want to note under the checkbox that skin preview uses the stored / default value of skin version?
todo: other AC

  • Preference should apply in an identical way to skin preference
  • Copy

Checkbox label: Use Legacy Vector
Text: Over the next few years, we will be gradually updating the Vector skin. Legacy Vector will allow you to view the old version of Vector (as of December 2019). To learn more about the updates, go to our project page.

QA

  • Go to the beta cluster as a logged in user
  • Go to the preferences page
  • Ensure a preference appears under the heading "Vector Preferences" and take a screenshot
  • Note the position of the preference relative to the "Skin" preference. Should be immediately afterwards

Signoff criteria

  • Make a task for identifying and adding global preference behavior

QA Results

ACStatusDetails
1T242381#5937113
2T242381#5937113

Related Objects

Event Timeline

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

Hello @AronManning! (Sorry for the slightly redundant reply.)

To clarify: I would fork Vector and treat the two as completely separate skins. No new concepts to learn and implement. Operators can easily install the legacy skin if they choose to as any other skin by checking out the VectorLegacy repository.

My understanding is that this was an input into T234907 but that there were concerns over the increased maintenance of an additional project. I was not present for most of the discourse of the RFC but there were also unknowns for bidirectional implicit dependencies in Vector and Core (e.g., conditionals operating on magic strings) that would need to be accounted for, deploying the new (or old, depending on how the fork was done) skin everywhere (all wikis, all projects, the MediaWiki tarball, etc), and sharing code that would be challenging in the current ecosystem, among other concerns.

From a developer perspective to avoid "special cases" I would list the old version as a separate skin called "Vector Legacy" and implement showing a short comment below any skin (visible only when selected).

Your note about special cases makes sense to me but thorough feature management is necessary for wiki configurable and user configurable features, deployment, and A/B tests. "Legacy Vector" and "Vector 2" are just bags of features or modes in this system that will be needed regardless. This scenario has been explored in depth for the advanced user project in the MobileFrontend feature manager which is now being considered for Core (T242835). Maybe forking later is still an option if this work proves to be harder than the RFC posits.

That's not to say that any of this work will be easy regardless of the approach taken but this ticket is only about changing the Vector user preferences shown on Special:Preferences and assumes the outcome of T234907.

My understanding is that this was an input into T234907 but that there were concerns over the increased maintenance of an additional project.

Thank you for clarifying, I was unaware that a final decision was made to use feature branching. If I may, I'd like to have a few more questions about that in T236176: [EPIC] Build opt-in/opt-out mechanism for desktop improvements project.
As Legacy Vector won't be a separate skin, disregard my mockup above, it's not applicable reasonably. I've just noticed T237561#5784710 Option 1 has already explored that solution.

I do note for anybody reading that the plan to implement this setting seems to be using the GetPreferences hook in /core/includes/preferences/DefaultPreferencesFactory.php:180.
If I understand correctly, the setting will be implemented in onGetPreferences() method in /skins/Vector/includes/Hooks.php, therefore the setting will be part of the skin, not the core (I've had the impression initially).
It took some research to put these pieces together. Please correct it if something is wrong in it.

Change 566881 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/Vector@master] [WIP] Add skin version user preference and configs

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

FWIW I just recalled Monobook has a skin specific preference that only displays when you have selected the Monobook skin:

Screen Shot 2020-01-28 at 8.36.54 PM.png (832×986 px, 147 KB)

Note "enable responsive Monobook design".

^Neat! It doesn't look like this preference even displays unless the skin preference has been saved!

Change 570733 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/core@master] WIP: show / hide skin preferences based on availability

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

I've added some skin preferences section display logic to Core as hide-if only works for field elements not sections.

Change 572039 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[operations/mediawiki-config@master] [prod] [Vector] Set skin version defaults

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

Config patch is up. I've submitted a revision of the hooks patch that adds tests for the different branches.

Remaining:

  • Questions to bounce with @phuedx:
    • Where do you think the skin version JavaScript client configuration should live? I think it makes more sense for this to be part of the feature manager but then again, it's either '1' or '2'. I don't actually need it for any code at the moment so we can punt this too.
    • Do you think I should add support for a "useskinversion" query parameter? This would kind of give us the ability to have URLs that are fully skin + version aware which I think would be very useful and may help with testing too. If so, should that be in the feature manager task, a new task, or somewhere else?
  • Bunch of tidy ups.
  • Get review from @phuedx.
  • Questions to bounce with @phuedx:
    • Do you think I should add support for a "useskinversion" query parameter? This would kind of give us the ability to have URLs that are fully skin + version aware which I think would be very useful and may help with testing too. If so, should that be in the feature manager task, a new task, or somewhere else?

Yes! This is an excellent idea!

Could you add a note to the feature management task that the ability to enable "sets" and individual features via querystring parameters should be considered a requirement?

Thank you, @phuedx!

Could you add a note to the feature management task that the ability to enable "sets" and individual features via querystring parameters should be considered a requirement?

Done in the description of T244481. I've worded it as a generic feature dependency requirement. Let me know if it's unclear (or feel free to edit!).

@phuedx, I've left a comment for you in code review regarding preference conversion. This still "needs work" but is pending your feedback so I'm moving over to code review and assigning to you temporarily

This comment was removed by phuedx.

As I was staging this change for review, I uncovered a radio alignment regression affecting wide-screen Firefox JavaScript users only. The issue is reproducible on the stock Special:Preferences page by simply infusing the skin section. E.g., OO.ui.infuse( $( '#mw-input-wpskin' ) ):

Screenshot_2020-02-20 Preferences - Wikipedia.png (1×2 px, 376 KB)
Screenshot_2020-02-20 Preferences - Wikipedia(1).png (1×2 px, 375 KB)

In my patches, the issue is triggered by using hide-if on the PHP side (which invokes an infusion on the JavaScript side) _or_ observing radio state changes by infusing the skin choice radios (similar to above).

I'm looking into it and have pinged Moriel for some insights. I'll follow up when I know more.

Ok, we have a workaround for T245793 (thanks @Mooeypoo!). New issue: there's a race scenario with Popups to insert the new skin preferences section directly underneath skin choice section. I can't think of a way to handle this scenario in the current architecture except:

  1. Unconditionally add a mw-prefsection-rendering-skin-prefs section. Since Vector is a default skin, it will then be guaranteed to exist. Right now, the whole section including the legacy mode option is conditionalized. This would move the $wgVectorShowSkinPreferences conditional to the option itself so that the section could be depended upon. Changes should be minimial: the proposed show / hide JavaScript logic should work for this scenario to hide it when $wgVectorShowSkinPreferences is disabled but non-JavaScript users will see an empty section and I'll have to update the section ID in Popups to look for skin-prefs not skins.
  2. Update Popups to try to insert after skin or skin-prefs and favor the latter. This adds an extra conditional and more tests to Popups but feels less risky to me since there's no skin-prefs coordination and it doesn't have the non-JavaScript display issue.

I'm thinking I'll probably go with #2 unless someone has a better idea. That'll be one checkbox, five repos and counting: efficiency, elegance, excellence :P

  1. Update Popups to try to insert after skin or skin-prefs and favor the latter. This adds an extra conditional and more tests to Popups but feels less risky to me since there's no skin-prefs coordination and it doesn't have the non-JavaScript display issue.

This is the more elegant of the two options IMO. Happy to help with review in the Popups repo.

Change 574542 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/Popups@master] [Special:Preferences] [PHP] Position Popups' prefs below skin prefs

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

Thanks, @phuedx!

Patches are in Gerrit and staged here https://stephen.wmflabs.org/wiki/Special:Preferences^1.

^1 Requires registering for an account which requires HTTPS. Note: new account redirect doesn't work--just hit back after signing up.

  1. Update Popups to try to insert after skin or skin-prefs and favor the latter. This adds an extra conditional and more tests to Popups but feels less risky to me since there's no skin-prefs coordination and it doesn't have the non-JavaScript display issue.

This is the more elegant of the two options IMO. Happy to help with review in the Popups repo.

For your consideration.... how about a core change?

Guaranteeing preference position / menu position seems like a recurring problem in our codebase. Given we entered this project keen to pay off technical debt I wonder if it would be useful to allow preference groups to define an order key for sorting purposes?

For each entry we could define an optional key order where the value is a float.

$defaultPreferences['skin'] = [
+                               'position' => 0,

We could then use this key to sort individual sections. If a section has no position key it could be allocated one based on its array size after the hook has run inside the giant blob that is the value returned by DefaultPreferencesFactory::getFormDescriptor - sorting would apply on any item which is in the same grouping.

e.g. position numbers on rendering/skin and rendering/files would be taken into account when ordering the appearance section. When ordering the tabs however these would be ignored but rc and rendering would be sorted accordingly.

Then Popups would no longer have to array_slice ... instead it would define a float. Any changes in order would then require the change of a float number. In the case of this new Vector skin option we could use position 0.25 for example.

diff --git a/docs/ui/README.md b/docs/ui/README.md
deleted file mode 100644
index d56f2bc..0000000
--- a/docs/ui/README.md
+++ /dev/null
@@ -1,2 +0,0 @@
-Run `npm run docs`!
-
diff --git a/includes/PopupsHooks.php b/includes/PopupsHooks.php
index c20f751..faca7a8 100644
--- a/includes/PopupsHooks.php
+++ b/includes/PopupsHooks.php
@@ -66,7 +66,6 @@ class PopupsHooks {
 				'Special:Preferences#mw-prefsection-gadgets' ];
 		}
 
-		$skinPosition = array_search( 'skin', array_keys( $prefs ) );
 		$readingOptions = [
 			PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME => $option,
 		];
@@ -76,6 +75,7 @@ class PopupsHooks {
 			!$config->get( 'PopupsReferencePreviewsBetaFeature' )
 		) {
 			$readingOptions[PopupsContext::REFERENCE_PREVIEWS_PREFERENCE_NAME] = [
+				'position' => 0.5,
 				'type' => 'toggle',
 				'label-message' => 'popups-refpreview-user-preference-label',
 				'section' => self::PREVIEWS_PREFERENCES_SECTION,
@@ -83,14 +83,7 @@ class PopupsHooks {
 			];
 		};
 
-		if ( $skinPosition !== false ) {
-			$injectIntoIndex = $skinPosition + 1;
-			$prefs = array_slice( $prefs, 0, $injectIntoIndex, true )
-				+ $readingOptions
-				+ array_slice( $prefs, $injectIntoIndex, null, true );
-		} else {
-			$prefs += $readingOptions;
-		}
+		$prefs += $readingOptions;
 	}
 
 	/**

I can understand this might be overkill for something so simple and expands the scope, but I can imagine us adding more preferences in a similar fashion and I worry we will revisit this time and time again until we have a trail of code checking dependencies on other extensions.

@alexhollender, @ovasileva does the new skin preference section need to appear directly below the skin section (see https://stephen.wmflabs.org/wiki/Special:Preferences#mw-prefsection-rendering for demo or screenshot below)? It seemed best to me but may require more work per @Jdlrobson's comment above.

@Jdlrobson's, thank you for the insight. It's also possible this work could occur as a follow-up so as not to block DIP if there's a commitment to it.

Screenshot_2020-02-24 Preferences - devwiki.png (1×2 px, 226 KB)

@alexhollender, @ovasileva does the new skin preference section need to appear directly below the skin section

Yes it should appear under the skin section, but not necessarily under the name of the skin itself. What would be our other option here?

Change 570733 merged by jenkins-bot:
[mediawiki/core@master] [JavaScript] show / hide skin Special:Preferences

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

Yes it should appear under the skin section, but not necessarily under the name of the skin itself. What would be our other option here?

@ovasileva, sorry, I don't follow. We can give the new Vector skin preferences section any title you like. If we don't require a specific order, that section may appear anywhere on the page.

Change 574819 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/core@master] [doc] [JavaScript] [Special:Preferences] Clarify selector

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

Change 574819 merged by jenkins-bot:
[mediawiki/core@master] [doc] [JavaScript] [Special:Preferences] Clarify selector

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

Per discussion in standup, I've removed the dependency on the Popups' ordering and will open a new task for consideration.

Change 574542 abandoned by Niedzielski:
[Special:Preferences] [PHP] Position Popups' prefs below skin prefs

Reason:
Abandoning until needed.

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

@alexhollender there's a couple of things here I'd like to design review - preferably after standup.

Change 572039 merged by jenkins-bot:
[operations/mediawiki-config@master] [prod] [beta] [Vector] Set skin version defaults

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

Mentioned in SAL (#wikimedia-operations) [2020-02-26T00:14:28Z] <jforrester@deploy1001> Synchronized wmf-config/InitialiseSettings.php: T242381 Set Vector skin version defaults so they can be changed on Beta Cluster (duration: 01m 04s)

Change 575031 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/core@master] [CSS] [Special:Preferences] Remove redundant OOUI override

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

Change 566881 merged by jenkins-bot:
[mediawiki/skins/Vector@master] [Special:Preferences] [PHP] Add skin version user preference and configs

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

Change 575031 merged by jenkins-bot:
[mediawiki/core@master] [CSS] [Special:Preferences] Remove redundant OOUI override

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

This still needs QA steps. Might have fallen off your radar.

ovasileva updated the task description. (Show Details)
ovasileva added a subscriber: olga.

Test Result

Status: ✅ PASS
OS: macOS Catalina
Browser: Chrome
Device: MBP

Test Artifact(s):

QA

Go to the beta cluster as a logged-in user
Go to the preferences page
❌ AC1: Ensure a preference appears under the heading "Vector Preferences" and take a screenshot
The heading says "Skin preferences.

✅ AC2: Note the position of the preference relative to the "Skin" preference. Should be immediately afterwards

en.wikipedia.beta.wmflabs.org_wiki_Special_Preferences(iPad).png (2×1 px, 564 KB)

QA

Go to the beta cluster as a logged-in user
Go to the preferences page
❌ AC1: Ensure a preference appears under the heading "Vector Preferences" and take a screenshot
The heading says "Skin preferences.

This is okay :)

Sorry for the late feedback.

Adding screenshots from beta cluster for English and Hebrew

Screen Shot 2020-03-03 at 2.39.33 PM.png (810×1 px, 218 KB)
Screen Shot 2020-03-03 at 2.39.03 PM.png (810×1 px, 192 KB)

Two other things:

  1. In the task description the heading for this new section says "Vector preferences" however currently it says "Skin preferences" on beta cluster. I think "Skin preferences" is perhaps too general and it would be better to call it "Vector preferences". Thoughts @ovasileva?
  1. Noticing that the spacing above the new section doesn't match the spacing between the other sections. This seems appropriate/intentional since the new setting is sort of a sub-setting of that first "Skin" section (the responsive monobook setting does the same thing, though it doesn't have a header/title). Mostly just wanted to make note.

image.png (296×335 px, 28 KB)

Two other things:

  1. In the task description the heading for this new section says "Vector preferences" however currently it says "Skin preferences" on beta cluster. I think "Skin preferences" is perhaps too general and it would be better to call it "Vector preferences". Thoughts @ovasileva?

@alexhollender - I actually think that skin preferences seems feels more reasonable since we couldn't place it immediately under the vector option. As in, we would have "Skins" (one of which is vector) and then "vector preferences" which refers to an option within the previous section. Not super passionate about it either way though. Both seem fine.

QA

Go to the beta cluster as a logged-in user
Go to the preferences page
❌ AC1: Ensure a preference appears under the heading "Vector Preferences" and take a screenshot
The heading says "Skin preferences.

This is okay :)

To get it to say "Vector preferences" we will need to revisit the solution. The solution was simple as it could be generalized. You'll note the same skin preferences section displays when Monobook is selected.

If we need custom headings for those it's doable but I'll have to suggest a new solution and we will need to reestimate - i reckon just over a weeks work.

QA

Go to the beta cluster as a logged-in user
Go to the preferences page
❌ AC1: Ensure a preference appears under the heading "Vector Preferences" and take a screenshot
The heading says "Skin preferences.

This is okay :)

To get it to say "Vector preferences" we will need to revisit the solution. The solution was simple as it could be generalized. You'll note the same skin preferences section displays when Monobook is selected.

If we need custom headings for those it's doable but I'll have to suggest a new solution and we will need to reestimate - i reckon just over a weeks work.

Let's keep it as is.

To get it to say "Vector preferences" we will need to revisit the solution. The solution was simple as it could be generalized. You'll note the same skin preferences section displays when Monobook is selected.

Noting that from what I can see the Monobook-specific preference doesn't have a heading:

image.png (548×964 px, 140 KB)

Add a configuration for controlling whether to display the new Vector preferences section or not, something like $wgVectorShowPreferences.

It ended up being called $wgVectorShowSkinPreferences.

Add a configuration for controlling the default Vector version preference for new accounts, something like $wgVectorDefaultSkinVersionForNewAccounts.

@Niedzielski also added $wgVectorDefaultSkinVersionForExistingAccounts, which controls the Vector version preference for users that haven't yet set a preference.

Whoo! All done :)

In T242381#5941249, @alexhollender wrote:

To get it to say "Vector preferences" we will need to revisit the solution. The solution was simple as it could be generalized. You'll note the same skin preferences section displays when Monobook is selected.

Noting that from what I can see the Monobook-specific preference doesn't have a heading:

image.png (548×964 px, 140 KB)

This looks like production.
I don't see this on beta cluster
https://en.wikipedia.beta.wmflabs.org/wiki/Special:Preferences#mw-prefsection-rendering

Screenshot_20200305-070940_Chrome.jpg (2×1 px, 430 KB)