Page MenuHomePhabricator

[GOAL] Add Feature Default Values Configuration Options in Vector and Minerva Skins
Closed, DuplicatePublic

Description

Background

Parent: T360092

To enhance the flexibility and customisation capabilities of developers working on the Vector and Minerva skins, this task is to expand the configuration options. The goal is to allow developers to define multiple default values for various features such as page width, font size, and themes depending on specific conditions like main page status, page title, namespace, or query string actions. This approach builds upon the previous exclusion logic and introduces the ability to specify exceptions within certain namespaces. This task is designed to refine the developer experience so it would be easier to tailor the user experience more accurately based on content type or user interactions, thereby enhancing accessibility and usability across Wikimedia sites.

User story

As a developer of Vector and Minerva skins, I want the ability to configure multiple default feature values conditionally based on page characteristics such as title, namespace, or specific user actions, so that I can deliver a more customised and effective user experience.

Requirements

  • Purpose and Discuss the suggested configuration scheme, and get feedback and agreements from other team members.
  • Enhance the configuration system to allow developers to dynamically set multiple and different default values for a feature in the skin.json file.
  • Configuration options should support:
    • Conditional Application: Capability to apply settings based on the main page status, specific page titles, namespaces, and query string parameters.
    • Flexibility and Control: Provide developers with the ability to specify exceptions in settings within defined namespaces.
  • Utilise, refactor and enhance the ConfigHelper class in the PHP code base to develop logic that reads and enforces these settings from the configuration file during runtime.
  • Ensure the configuration changes are parsed efficiently and do not impact the performance of the skins.
  • Ensure compatibility with both Vector and Minerva skins, with configurations being easy to manage and update

Acceptance criteria

  • Developers can specify and save conditional configurations for feature defaults in the skin.json file.
  • The system must accurately apply these settings based on the conditions outlined in the configuration.
  • Documentation must be clear, providing sufficient examples to assist developers in setting up and customising configurations.
  • The following pages should get Standard as the default font size: Namespaces=0,4,12,118.
  • All other pages would get Small as the default font size.

Open questions

  • Is this for mobile and desktop or just desktop?
  • When I visit a page where small is the default font size, can I change font size in the appearance menu? Is it disabled?
  • When I visit a page where small is the default font size would small be selected?
  • When I visit a page where small is the default should there be a banner saying "This page always appears as small?"

Event Timeline

A suggested configuration is as follows:

I have a few concerns with the configuration.

  1. Too expressive.

Configuration that changes appearance should not be added lightly as it increases the amount of variants we need to test on. The feature options seems to go beyond what we need here - for example I would be concerned about allowing this level of granularity per page.

The current system allows us to modify Special:RecentChanges to use large font size, Special:Preferences to use regular and Special:Watchlist to use small. I don't think we need that - it seems from requirements we need just standard (default) or small (exceptions)
An alternative might be something like wgFontSizeSmall which explicitly expresses pages which receive small font size instead of default, however I'd still question whether we need it to be as expressive as limited width - perhaps a list of pages as a first pass might be sufficient for example?

  1. Conflict resolution It's not clear to me how we would deal with clashes with this proposed spec - with the current model I can set both 1.for.mainpage and 2.for.mainpage to true and it's not clear what I'd end up with.
  1. Coupling with feature management system

Coupling with feature management (e.g. exclude) will likely complicate the feature management further. Since the feature management system only determines whether the feature is enabled or not, I wonder if this could be contained separately (e.g. when enabled what do the defaults look like?

A suggested configuration is as follows:

I have a few concerns with the configuration.

  1. Too expressive.

Configuration that changes appearance should not be added lightly as it increases the amount of variants we need to test on. The feature options seems to go beyond what we need here - for example I would be concerned about allowing this level of granularity per page.

The current system allows us to modify Special:RecentChanges to use large font size, Special:Preferences to use regular and Special:Watchlist to use small. I don't think we need that - it seems from requirements we need just standard (default) or small (exceptions)
An alternative might be something like wgFontSizeSmall which explicitly expresses pages which receive small font size instead of default, however I'd still question whether we need it to be as expressive as limited width - perhaps a list of pages as a first pass might be sufficient for example?

  1. Conflict resolution It's not clear to me how we would deal with clashes with this proposed spec - with the current model I can set both 1.for.mainpage and 2.for.mainpage to true and it's not clear what I'd end up with.
  1. Coupling with feature management system

Coupling with feature management (e.g. exclude) will likely complicate the feature management further. Since the feature management system only determines whether the feature is enabled or not, I wonder if this could be contained separately (e.g. when enabled what do the defaults look like?

I agree I was trying to show a quick sample that came to my mind while writing the ticket.

I guess the 1st item of the requirement is suggesting and getting a discussion and agreement on a proposed configuration scheme.

the part that was removed from task description based on this discussion is:

A suggested configuration is as follows:

"VectorFeatureOptions": {
  "value": {
    "exclude": {
      "mainpage": false,
      "pagetitles": [],
      "namespaces": [],
      "querystring": {}
    },
    "include": [],
    "defaults": {
      "1": {
        "for": {
          "mainpage": true,
          "pagetitles": [],
          "namespaces": [],
          "querystring": {
            "action": "edit"
          }
        },
        "except": []
      },
      "2": {
        "for": {
          "mainpage": false,
          "pagetitles": [
            "Special:Preferences"
          ],
          "namespaces": [
            12
          ],
          "querystring": {
            "action": "history"
          }
        },
        "except": [
          "Article_Whatever"
        ]
      },
      "3": {
        "for": {
          "mainpage": false,
          "pagetitles": [],
          "namespaces": [
            14
          ],
          "querystring": {
            "action": "view"
          }
        },
        "except": [
          "Special:Search"
        ]
      }
    },
    "description": "Configuration settings to determine feature compatibility and default values based on page type, namespace, or query string. These settings allow for dynamic customisation of user experience."
  }
}

NOTE: The values in the configuration example above are random and do not correlate to our projects, it is just a sample to refine and think about the output configuration in the skin.json file

Jdlrobson added subscribers: JScherer-WMF, ovasileva.

@ovasileva @JScherer-WMF is this for just desktop or mobile and desktop? (please see this and other open questions and remove and move r to "acceptance criteria")

I was looking into suggesting a newer configuration suggestion, especially adhering to feedback from @Jdlrobson :

"VectorFeatureOptions": {
  "value": {
    "system": {
      "changeValueAllowed": true, // Allows changes of the value. similar to disable/enable
      "defaultValue": "standard" // If not configured, the first value of the enumeration is the default.
    },
    "mainPage": {
      "changeValueAllowed": true,
      "defaultValue": "non-standard-1"
    },
    "namespaces": {
      "12": {
        "changeValueAllowed": false,
        "defaultValue": "non-standard-2",
        "exclude": ["PageName"]
      },
      "14": {
        "changeValueAllowed": true,
        "defaultValue": "wide"
      }
    },
    "querystrings": {
      "action": {
        "changeValueAllowed": true,
        "values": {
          "view": "standard",
          "edit": "non-standard-2",
          "history": "non-standard-1"
        }
      },
      "diff": {
        "changeValueAllowed": true,
        "matchPattern": ".+",
        "defaultValue": "non-standard-3"
      }
    },
    "pagetitles": {
      "Special:Preferences": {
        "changeValueAllowed": true,
        "defaultValue": "non-standard-1"
      },
      "Special:Search": {
        "changeValueAllowed": false,
        "defaultValue": "standard"
      }
    },
    "description": "Configuration settings structured to define feature compatibility and default values for main pages, namespaces, query strings, and page titles. Includes a system fallback to handle unconfigured features, ensuring broad default customisation with the flexibility to adapt."
  }
}

This gives us a set of flags that can be driven by key value rather than processing, and it can utilise the key "DefaultUserOptions" in skin.json or replace it.

Feedback is appreciated until our next SHDT.

At a high level, can someone explain why the feature options and feature configuration key are separate? Personally I would consider these exclusion options as properties of a feature. Currently, we have the following two keys in skin.json:

"VectorNightMode":...
"VectorNightModeOptions":...

It's more intuative to me to think of these as a single object (since they are related), e.g:

"VectorNightMode": {
    "options": {...}
};

Is this because of technical limitations? The separation can also lead to confusion, where the options and feature key can be named differently, like in the case of the "VectorMaxWidthOptions" and the "vector-limited-width" user setting, e.g:

"DefaultUserOptions": {
	"vector-limited-width": 1,
},
"VectorMaxWidthOptions":
ovasileva set the point value for this task to 5.Thu, May 2, 5:36 PM

I think we should also remove the suggested

"changeValueAllowed": true

as we never set a different default value expecting it to be changeable, so an updated version should be like:

"DefaultUserOptions": {
  "FeatureName": {
    "defaultValue": "standard",
    "mainPage": {
      "defaultValue": "non-standard-1"
    },
    "namespaces": {
      "12": {
        "defaultValue": "non-standard-2",
        "exclude": [
          "PageName"
        ]
      },
      "14": {
        "defaultValue": "non-standard-1"
      }
    },
    "querystrings": {
      "action": {
        "defaultValue": {
          "view": "standard",
          "edit": "non-standard-2",
          "history": "non-standard-1"
        }
      },
      "diff": {
        "matchPattern": ".+",
        "defaultValue": "non-standard-3"
      }
    },
    "pagetitles": {
      "Special:Preferences": {
        "defaultValue": "non-standard-1"
      },
      "Special:Search": {
        "defaultValue": "standard"
      }
    },
    "description": "Configuration settings structured to define feature compatibility and default values for main pages, namespaces, query strings, and page titles. Ensuring broad default customisation with the flexibility to adapt."
  }
}

The core functionality should return the defaultValue and an overrideDefaultValue, and as long the override parameter is empty it means just normal behaviour, and otherwise we utilise the exception handling workflow

Lastly for this

Is this because of technical limitations? The separation can also lead to confusion, where the options and feature key can be named differently, like in the case of the "VectorMaxWidthOptions" and the "vector-limited-width" user setting, e.g:

"DefaultUserOptions": {
	"vector-limited-width": 1,
},
"VectorMaxWidthOptions":

I agree, that we should standardise using either but not both, let me know if the snake case is preferable to the camel case, but we can change from one to another easily on the code without confusing the configuration.

I prefer camel case myself if it is a vote here is mine.

@Jdrewniak when we started the feature management system it was only based on user - whether they were anon or logged in. Over time our feature management got conflated with the concept of per page features. As a result of this, we now have a lack of common understanding about what a feature (see ticket in backlog [jon to replace with link later when he's found it]) . I think it's important we establish one before making any decisions here around architecture - personally I would advocate for creating a specification describing a new system that meets our new needs, and migrating to it over time.

On the short term, I should note VectorNightMode is temporary - it will be removed when the feature is completed. VectorNightModeOptions isn't.

Note about preference keys: these are typically hyphenated in MediaWiki,whereas configuration flags are snake-cased. Changing preferences is super disruptive to existing users so make sure not to do that as part of this task.

Change #1027479 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/skins/Vector@master] feature(POC:vector): Refactor for DefaultPageOptions for enhanced flexibility

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

Change #1027483 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/skins/MinervaNeue@master] feature(POC:minerva): Adapt Vector config method for night mode

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

Change #1026891 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/core@master] feature(POC:core-skin): Preserve configured overrides in clientprefs.js

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

Jdlrobson added a subscriber: NBaca-WMF.

I will get context today on why this is blocked and talk to @NBaca-WMF today.

Hey @Mabualruz I'd like for us to talk about this in SHDT on Wednesday. Given the proximity to the night mode deploy, and the fragility of our feature management system(s) these changes seem like they could be significant and risky.

I'd like for us to think about what's the absolute minimum work we need to do here to address the primary user story which I believe is "Use a small font size for certain pages. Use standard for other pages."
Right now it seems like we only need the standard font size on: 0,4,12,118

This task also does not seem like it was ready for development/estimation given there are still significant unanswered "Open questions" so I'm moving this to back to to "Backlog" until we've got some clarity on those points.

Jdlrobson renamed this task from Add Feature Default Values Configuration Options in Vector and Minerva Skins to [GOAL] Add Feature Default Values Configuration Options in Vector and Minerva Skins.Tue, May 14, 5:47 PM
Jdlrobson removed the point value for this task.

After chatting with Justin T364887 apparently does everything we need so we can decline this ticket for now <3