Page MenuHomePhabricator

Define language CSS rules in a common module shared by all skins (Mobile skin (Minerva) seems to lack definition for .mw-content-ltr)
Open, Needs TriagePublic

Description

The class definition in Minerva seems to be missing; see the difference in rendering in Hebrew Wiki when trying to declare a specific block as LTR.

In desktop (good rendering) :

https://he.wikipedia.org/wiki/%D7%94%D7%AA%D7%99%D7%A7%D7%95%D7%9F_%D7%9C%D7%A9%D7%95%D7%95%D7%99%D7%95%D7%9F_%D7%96%D7%9B%D7%95%D7%99%D7%95%D7%AA#.D7.A0.D7.95.D7.A1.D7.97_.D7.94.D7.AA.D7.99.D7.A7.D7.95.D7.9F

In mobile (bad rendering; see the period at the end as the indication; the class is missing the required direction: ltr;) :
https://he.wikipedia.org/wiki/%D7%94%D7%AA%D7%99%D7%A7%D7%95%D7%9F_%D7%9C%D7%A9%D7%95%D7%95%D7%99%D7%95%D7%9F_%D7%96%D7%9B%D7%95%D7%99%D7%95%D7%AA#.D7.A0.D7.95.D7.A1.D7.97_.D7.94.D7.AA.D7.99.D7.A7.D7.95.D7.9F

The issue appears to be the fact the Minerva skin intentionally does not load mediawiki.legacy.shared (Minerva doesn't and shouldn't load any legacy code)

Suggested solution:

  • Let's move all i18n related CSS into a separate module and add it to an i18n key inside SkinTemplate::getDefaultModules. This will trickle down to all skins regardless of whether they are wiping out the legacy CSS modules.

Event Timeline

Mooeypoo created this task.Jul 6 2017, 5:30 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 6 2017, 5:30 PM

Is this related to T161399 in any way? Feel free to submit a patch - I'll happily merge.. lacking expertise in how RTL should work!

@Catrope and I checked into this, and the definition is in the mediawiki.legacy.shared module. Two conclusions strike me as action items:

  • short term: have Minerva depend on that module
  • long term: having directionality rules in a legacy module strikes me as pure insanity. We need to move those to common definition that works on all skins. Tagging @Volker_E too

I'm in a conference, but I'll try to at least looking into the long term issue. As for short term, I'll update Vagrant per the instructions to get Minerva on its own so we can add the module dependency, but since my access is limited for the next couple of days, it will probably wait on my part to next week.

Minerva made a choice not to depend on anything with legacy in the title to make hacks like this more important to fix. It does mean things like this take longer to fix but they help the project better on the long term (this is also the reason for the targets system). The long term option is the correct one. :)

Jdlrobson renamed this task from Mobile skin (Minerva) seems to lack definition for .mw-content-ltr to Define language css rules in a common module shared by all skins (Mobile skin (Minerva) seems to lack definition for .mw-content-ltr).Jul 6 2017, 10:41 PM
Jdlrobson updated the task description. (Show Details)

@Volker_E I have added a proposal patch for this, making language a new module and setting it as a dependency of mediawiki.skinning.content.

We should examine if this is the right place to put it, and if it is sufficient. As explained in the commit message, it could also be split to language and ltr/rtl separate files, or include accessibility issues as well.

Either way, the fact directionality rules are in legacy module has to be fixed...

Change 363888 had a related patch set uploaded (by Mooeypoo; owner: Mooeypoo):
[mediawiki/core@master] [proposal] Split language/dir CSS rules to own module and make dependency

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

As @matmarex points out, there already is a ticket to split out legacy modules to non-legacy ones. It seems to not be high priority, but this behavior in RTL/LTR mixes in mobile makes at least that part a priority itself. see T89981: Split the mediawiki.legacy.shared module into non-legacy manageable parts for context.

The commit above adds the files to the 'skinning' modules, but I am not sure this is the place to load all of them automatically in all shared installations; where else should these be included to have them in all skins?

Originally I said adding it to one of the mediawiki.skinning.* modules is a good idea, but then I realized we'd be duplicating the CSS in skins that load both these and shared.css. So, with this in mind, I think we should split these off to an entirely new module, and add that module to the page in SkinTemplate::setupSkinUserCss() (and WebInstallerOutput::getCSS()).

Volker_E updated the task description. (Show Details)Jul 11 2017, 1:53 PM
Jdlrobson moved this task from Backlog to Bugs on the MinervaNeue board.
Volker_E renamed this task from Define language css rules in a common module shared by all skins (Mobile skin (Minerva) seems to lack definition for .mw-content-ltr) to Define language CSS rules in a common module shared by all skins (Mobile skin (Minerva) seems to lack definition for .mw-content-ltr).Aug 22 2017, 11:14 PM
Jdlrobson moved this task from Bugs to Blocked on the MinervaNeue board.
Jdlrobson moved this task from Inbox to Tracking on the User-Jdlrobson board.Dec 7 2017, 10:10 PM
Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptMay 8 2019, 12:08 PM