Page MenuHomePhabricator

Add a Vector skin version preference
Open, HighPublic5 Estimate 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):

Hidden (Non-Vector skin selected):

  • 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

todo

Signoff criteria

  • Make a task for identifying and adding global preference behavior

Details

Related Gerrit Patches:

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 9 2020, 10:19 PM

@ovasileva, @alexhollender: this is a little technical but hopefully this kind of makes sense. There are three configs:

  • wgVectorShowPreferences: show / hide Vector preferences.
  • wgVectorDefaultSkinVersion: if a user hasn't set a legacy / modern preference, this is the default used.
  • wgVectorDefaultSkinVersionForNewAccounts: similar to wgVectorDefaultSkinVersion, but for new accounts.

The changes for hooks are to add the UI and wire everything together. Feel free to follow up here or over Hangouts.

Notes from @Jdlrobson:

  • Is there another word we can use instead of "enable"? "Enabling" legacy mode doesn't add functionality but the word has kind of a positive connotation. Maybe "freeze" or "force" or even "disable modern Vector"?
  • It might be practical to improve the styling in the new section to more closely resemble the original mock but will take further research.
Ammarpad added a subscriber: Ammarpad.EditedJan 10 2020, 2:46 AM

Notes from @Jdlrobson:

  • Is there another word we can use instead of "enable"? "Enabling" legacy mode doesn't add functionality but the word has kind of a positive connotation. Maybe "freeze" or "force" or even "disable modern Vector"?

I think this question arises because you're trying to do things in the reverse. You shouldn't say "Enable legacy vectors" It's already enabled with wgVectorDefaultSkinVersion = 1 and actually it gives subtle impression like you're forcing the new version on users. (even though that's not the case technically), it's on the surface.

The ideal option is

  • Enable Modern version of Vector
  • Enable Advanced Vector
  • Enable Improved Vector or
  • Enable Vector version 2

This makes it clear you're introducing something new and giving users option to use it or not.

Another minor issue with the second image. If Vector is not selected, then the whole new preference should not be shown. I notice in the image, only the description is hidden (leaving stray heading)

@Ammarpad, the initial state of the checkbox would be $wgVectorDefaultSkinVersion. I'm not sure but I think the bold idea is that most users would eventually prefer an actively developed skin (version 2) and that for this reason the legacy skin is a sort of special configuration (quite literally as a checkbox) and the default unconfigured state is 2. This reasoning is aligned with most versioned software where this is an official mainline master version that is actively developed and various older versions that are eventually frozen.

Another minor issue with the second image. If Vector is not selected, then the whole new preference should not be show. I notice in the image, only the description is hidden (leaving stray heading)

Agree. The second image is a screenshot from a kind of experiment. We hope to hide the whole section. It's covered under "The whole section should be shown when the Vector skin is selected under skin preferences and $wgVectorShowPreferences is true, and be hidden otherwise." There's a little more background in T237561.

ovasileva raised the priority of this task from Medium to High.Jan 10 2020, 11:11 AM

I like "Disable Modern Vector" or maybe perhaps "Use Legacy Vector"?

Niedzielski updated the task description. (Show Details)Jan 10 2020, 9:28 PM

@Ammarpad, the initial state of the checkbox would be $wgVectorDefaultSkinVersion. I'm not sure but I think the bold idea is that most users would eventually prefer an actively developed skin (version 2) and that for this reason the legacy skin is a sort of special configuration (quite literally as a checkbox) and the default unconfigured state is 2. This reasoning is aligned with most versioned software where this is an official mainline master version that is actively developed and various older versions that are eventually frozen.

Thanks, I see that.

I discussed this task with @pmiazga, mostly about how it relates to T237635, and the configuration changes and the approach proposed. We think this work will be pretty loosely coupled from the feature manager but it would be nice to deliver legacy and v2 "modes" in the initial (Vector) feature manager patches whenever they come (Piotr is hoping for next week). No other concerns regarding approach but Piotr has some ideas about using the BetaFeatures extension, when available, in conjunction with the $wgVectorDefaultSkinVersion config to start more users on v2 that he'll be discussing with @ovasileva but that can be decided independently of this task.

phuedx updated the task description. (Show Details)Jan 15 2020, 5:16 PM
ovasileva updated the task description. (Show Details)Jan 15 2020, 5:17 PM
ovasileva updated the task description. (Show Details)Jan 15 2020, 5:20 PM
ovasileva updated the task description. (Show Details)Jan 15 2020, 5:23 PM
ovasileva set the point value for this task to 5.
phuedx updated the task description. (Show Details)Jan 15 2020, 5:26 PM
ovasileva updated the task description. (Show Details)Wed, Jan 22, 4:04 PM

@Niedzielski - discussed copy with @alexhollender and added to task description

Demian added a subscriber: Demian.EditedThu, Jan 23, 6:09 AM

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).

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.

Imho this is the simplest and most consistent handling of the fork. Was this option considered?

Demian added a comment.EditedThu, Jan 23, 6:34 AM

It would look like this:


#mw-input-wpskin label + div { margin: 0em 3em 0.5em; display: none; }
#mw-input-wpskin label.oo-ui-optionWidget-selected + div { display: block; }

The <div> for the message is after the <label> for the skin.

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.

Demian added a comment.EditedThu, Jan 23, 8:47 PM

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:


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