Page MenuHomePhabricator

Allow Skins to register themselves as "responsive"
Closed, ResolvedPublic

Description

From T254848#6315804

Timeless, Minerva and Vector (when configured) add a viewport to the article:

https://github.com/wikimedia/mediawiki-skins-Timeless/blob/master/includes/SkinTimeless.php#L23
https://github.com/wikimedia/Vector/blob/c7e9719b9b39a06990f7524bf0f0596c9248c368/includes/Hooks.php#L50
https://github.com/wikimedia/mediawiki-skins-MinervaNeue/blob/master/includes/Skins/SkinMinerva.php#L138

which looks a little like this:

$out->addMeta( 'viewport', 'initial-scale=1.0, user-scalable=yes, minimum-scale=0.25, ' .
				'maximum-scale=5.0, width=device-width'
		);

Unfortunately the value of viewport is never consistent and it creates challenges for template editors trying to target styles specifically to those that use it

Many template editors are adding rules to target responsive skins such as:

body.skin-timeless,
body.skin-minerva {
...
}

proposal

  • Skins will be able to pass an option responsive [1]. When set it will automatically add a viewport and will also add a class to the body 'skin--responsive'
  • the option defaults to false
  • Minerva updated to use it
  • Timeless updated to use it

[1] https://github.com/wikimedia/mediawiki/blob/master/includes/skins/Skin.php#L152

Event Timeline

Jdlrobson added subscribers: Izno, Isarra.

@Isarra would you adopt this if added ?

@Isarra would you adopt this if added ?

Every responsive skin I maintain uses the same viewport, and if it doesn't, it usually winds up on the same sooner or later out of paranoia, as I don't know where any of them even came from and I don't understand how it works, just that this one apparently does. So being able to just grab something from mw instead would probably make a bit more sense, yes.

the option defaults to false

Might make more sense to do it the other way around, as html is by default responsive and it's only when you start doing weird styles with it that it stops being so. Modern skin developers are hopefully not doing this, as mobile is now a primary target (it was not when monobook et all were made, and as those were also targetting things like IE, they really did have to do some weird things to get their layouts to work at all at times), and it would be best not to penalise them for following best practices. So I would recommend making it true by default, and then explicitly marking the older skins as false.

Would it be possible to do it this way while still allowing the current method of adding it to override it, for backwards compatibility?

Would it be possible to do it this way while still allowing the current method of adding it to override it, for backwards compatibility?

Making it the default would definitely be possible, but it would be a breaking change and need to be done in stages - adding the method and defaulting to false, updating all skins (at least Modern, CologneBlue and Monobook) to use or not use it, and then defaulting it to true.

It would be much easier to default it to true on the SkinMustache's constructor which is a new class and doesn't suffer from the legacy problems of other skins - which Minerva and Vector will be adopting - but I'm not sure if Timeless has plans to adopt it? (if only to get away from PHP string stitching and having multiple classes for a skin).

Here's Vector's adoption of it for example which will hopefully land in the next 2 weeks: https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/593070 - is that something Timeless would be interested in adopting in some form on the long term?

Are there any simpler examples? Do you have any plans to migrate the Example skin?

Are there any simpler examples? Do you have any plans to migrate the Example skin?

I'd love to migrate the Example skin to show what that would look like. I was just a little reluctant too as I'm not sure who would review and merge :)
The Modern port is probably the simplest example right now, but it also requires dependencies in Vector to work:

https://gerrit.wikimedia.org/r/c/mediawiki/skins/Modern/+/585834

I've made a note to do better with this regard - T258292
I know @Mainframe98 has been working on a skin (possibly using SkinMustache?) but I don't know if there's anything shareable there (yet)

Izno renamed this task from Allow Skin's to register themselves as "responsive" to Allow Skins to register themselves as "responsive".Jul 17 2020, 7:11 PM

I know @Mainframe98 has been working on a skin (possibly using SkinMustache?) but I don't know if there's anything shareable there (yet)

Migrating as we speak (write), but got stuck on something I haven't had the time to investigate into yet. Once that's done I'll see what I can do to upload a prototype.
In any event, if you need help, feel free to add me to any patches.

I've also defined the viewport attribute, but with only width=device-width, initial-scale=1.0, user-scalable=yes set; I'm not exactly sure what the difference is.

@Mainframe @Isarra given Minerva is more stress tested (more users) I'd personally suggest using its value for the viewport attribute, unless you can suggest reasons why we would not want to use it.

initial-scale=1.0, user-scalable=yes, minimum-scale=0.25, maximum-scale=5.0, width=device-width

I can't recall the reasons for using minimum and maximum scale but am open to dropping those if we feel strongly about it.

@Mainframe98 @Isarra given Minerva is more stress tested (more users) I'd personally suggest using its value for the viewport attribute, unless you can suggest reasons why we would not want to use it.

initial-scale=1.0, user-scalable=yes, minimum-scale=0.25, maximum-scale=5.0, width=device-width

I can't recall the reasons for using minimum and maximum scale but am open to dropping those if we feel strongly about it.

Reading the mdn article about it, it seems to have to do with safari's default behaviour of zooming when changing from portrait to landscape. In any event, I'd rather defer to the experts who worked on Minera (given that I end up with a similar mobile view in my skin) and just use that.

Look, as long as it works, I'm fine with any sane default. :P

Change 618132 had a related patch set uploaded (by Mainframe98; owner: Mainframe98):
[mediawiki/core@master] Skin: Add a skin option for responsiveness

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

Change 618133 had a related patch set uploaded (by Mainframe98; owner: Mainframe98):
[mediawiki/skins/Timeless@master] Specify the responsive option rather than manually specifying viewport

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

Change 618134 had a related patch set uploaded (by Mainframe98; owner: Mainframe98):
[mediawiki/skins/MinervaNeue@master] Specify the responsive option rather than manually specifying viewport

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

Change 618135 had a related patch set uploaded (by Mainframe98; owner: Mainframe98):
[mediawiki/skins/MonoBook@master] Specify the responsive option rather than manually specifying viewport

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

Change 618132 merged by jenkins-bot:
[mediawiki/core@master] Skin: Add a skin option for responsiveness

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

Change 618134 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Specify the responsive option rather than manually specifying viewport

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

Change 618135 merged by jenkins-bot:
[mediawiki/skins/MonoBook@master] Specify the responsive option rather than manually specifying viewport

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

Change 618133 merged by jenkins-bot:
[mediawiki/skins/Timeless@master] Specify the responsive option rather than manually specifying viewport

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

Jdlrobson updated the task description. (Show Details)

Neato! Thanks @Mainframe98 for taking care of the patches here!

The double dash is kind of not in keeping with the style of the other selectors (in general as well as the classes on body). Can a quick followup be done to use one dash instead? Or some rationale provided for why two were used?

Per @Jdlrobson's comment on the patchset:

we don't often use '--' and use '-' if a skin is called responsive it will get the class 'skin-responsive' so this ensures it won't be targetted

Change 622469 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/skins/Timeless@master] Update required core version

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

Change 622469 merged by jenkins-bot:
[mediawiki/skins/Timeless@master] Update required core version

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