Page MenuHomePhabricator

Move mobile editor code from Minerva skin to MobileFrontend, use it for all skins on mobile (and none on desktop)
Closed, ResolvedPublic

Description

As discussed on T197834:

The mobile wikitext editor is always used on Minerva skin, in both mobile and desktop mode, even though the desktop editor is much more useful on desktop. (…)
The desktop editor is always used on all other skins, even in mobile mode, even though it seems to me that the mobile editor would work fine with other skins, (…).
Can we instead change this behavior, so that the mobile editor is used on mobile regardless of skin, and the desktop editor is used on desktop regardless of skin?

Developer notes

The following code is the minimum needed to get editor showing on the Timeless mobile skin with some obvious challenges to work out:

mw.config.set('wgMinervaMenuData', {})
mw.loader.using('skins.minerva.editor').then(function() {
  mw.mobileFrontend.require('mobile.startup/Overlay').prototype.appendToElement = 'body'
  window.location.hash = '#/editor/0';
} )

Goals

  1. Stop loading the mobile editor on desktop Minerva (✔ done T198765#4698861)
  2. Move the code from Minerva to MobileFrontend (✔ done T198765#4744389)
  3. Allow loading the mobile editor on other mobile skins (❌ see T198765#4744472)

Related Objects

StatusAssignedTask
OpenNone
OpenNone
OpenNone
OpenJdlrobson
OpenNone
OpenNone
DeclinedNone
ResolvedDannyH
OpenNone
DuplicateNone
Openmarcella
OpenNone
OpenNone
OpenNone
Resolvedmatmarex
Resolvedmatmarex
Resolvedovasileva
ResolvedABorbaWMF
ResolvedNone

Event Timeline

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

Change 461852 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/skins/MinervaNeue@master] Move redlink CTA out of skins.minerva.editor

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

Change 461853 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/skins/MinervaNeue@master] Move edit link enabling/disabling out of skins.minerva.editor

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

Change 461854 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/skins/MinervaNeue@master] Move pageIssues code out of skins.minerva.editor

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

Change 461855 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/skins/MinervaNeue@master] Make Minerva section editing more like other skins

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

Change 461856 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/skins/MinervaNeue@master] skins.minerva.newusers: Remove dependency on skins.minerva.editor

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

matmarex added a comment.EditedSep 21 2018, 4:59 AM

I propose we do this in three steps (separate patches for each, and we can stop after step 1 or 2 if we think that's better):

  1. Move the code from Minerva to MobileFrontend, stop loading the mobile editor on desktop, but keep it "whitelisted" to Minerva

I spent the day today working on this, and I think step 1 is ~80% done. I ended up producing a dozen smaller patches rather than a single big one, hopefully that makes review easier. (There are also a few cleanup patches I didn't tag against this task, submitted as dependencies in Gerrit.)

All relevant patches: https://gerrit.wikimedia.org/r/q/topic:T198765

I think only the following remains:

  • Move 'wgMinervaReadOnly' mw.config option to MediaWiki core
  • Move the singleton 'skins.minerva.scripts/overlayManager' to MobileFrontend (the class 'mobile.startup/OverlayManager' is already there)
  • Move the singleton 'skins.minerva.scripts/skin' to MobileFrontend (it's actually an alias for 'mobile.init/skin' defined there)
  • Figure out why the module 'skins.minerva.editor' depends on 'skins.minerva.icons.images.scripts', 'skins.minerva.scripts', and 'skins.minerva.toggling', since it doesn't seem to use any of them, and hopefully remove these dependencies
  • Move the module 'skins.minerva.editor' itself to MobileFrontend!

Move 'wgMinervaReadOnly' mw.config option to MediaWiki core

🎆

Move the singleton 'skins.minerva.scripts/overlayManager' to MobileFrontend (the class 'mobile.startup/OverlayManager' is already there)
Move the singleton 'skins.minerva.scripts/skin' to MobileFrontend (it's actually an alias for 'mobile.init/skin' defined there)

I'm not sure about these moves. It feels too risky to me as it requires changing all the require paths.
For skin, I'd advise exploring updating the editor to make use of 'mobile.init/skin'.
For OverlayManager - I'd advise rewiring the code so that OverlayManager has a getSingleton method that caches an instance of OverlayManager. Continue to export 'skins.minerva.scripts/overlayManager' but tie it to that singleton method instead.

Figure out why the module 'skins.minerva.editor' depends on 'skins.minerva.icons.images.scripts', 'skins.minerva.scripts', and 'skins.minerva.toggling', since it doesn't seem to use any of them, and hopefully remove these dependencies

Seems a bit weird to me and probably incorrect. Looks like these dependencies exist before 2cc9516c (so old).
skins.minerva.scripts is likely a required dependency because of all the scaffolding that provides - skin and overlayManager. skins.minerva.toggling and skins.minerva.icons.images.scripts' seems unnecessary and can be removed (pending some testing!)

Change 461854 abandoned by Bartosz Dziewoński:
Move pageIssues code out of skins.minerva.editor

Reason:
OK, let's skip this change due to the ongoing A/B test (looks like T200792 is the relevant task). Hopefully this code will be just removed after it ends.

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

Change 461852 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Move redlink CTA out of skins.minerva.editor

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

Change 461853 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Move edit link enabling/disabling out of skins.minerva.editor

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

Change 461855 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Make Minerva section editing more like other skins

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

Change 461856 abandoned by Bartosz Dziewoński:
skins.minerva.newusers: Remove dependency on skins.minerva.editor

Reason:
Looks like we want to remove this code instead (T205382).

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

All the patches here have been merged. Can we move this on from CodeReview?

Change 470087 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/skins/MinervaNeue@master] Disable mobile editor (skins.minerva.editor) on desktop

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

Change 470087 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Disable mobile editor (skins.minerva.editor) on desktop

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

Change 461854 restored by Bartosz Dziewoński:
Move pageIssues code out of skins.minerva.editor

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

Change 461854 abandoned by Bartosz Dziewoński:
Move pageIssues code out of skins.minerva.editor

Reason:
Superseded by https://gerrit.wikimedia.org/r/c/mediawiki/skins/MinervaNeue/ /467842 (on "page-issues-cleanup" branch, to be merged into master soon T208514).

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

Change 472038 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] ResourceLoaderStartUpModule: Add 'wgReadOnly'

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

Change 472477 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/MobileFrontend@master] mobile.startup/OverlayManager: Add #getSingleton

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

Change 472478 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/skins/MinervaNeue@master] Merge remote-tracking branch 'gerrit/page-issues-cleanup'

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

Change 472479 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/skins/MinervaNeue@master] skins.minerva.scripts/init: Use singleton OverlayManager from MF

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

Change 472480 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/skins/MinervaNeue@master] skins.minerva.editor: Remove remaining uses of 'skins.minerva.scripts'

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

Move 'wgMinervaReadOnly' mw.config option to MediaWiki core

🎆

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

Krinkle has objected to adding new mw.config options. Meh, I guess we can just leave it in Minerva only. In the worst case, the editor will not see the read-only warning before beginning their edit, only when attempting to save it.

Move the singleton 'skins.minerva.scripts/overlayManager' to MobileFrontend (the class 'mobile.startup/OverlayManager' is already there)
Move the singleton 'skins.minerva.scripts/skin' to MobileFrontend (it's actually an alias for 'mobile.init/skin' defined there)

I'm not sure about these moves. It feels too risky to me as it requires changing all the require paths.
For skin, I'd advise exploring updating the editor to make use of 'mobile.init/skin'.
For OverlayManager - I'd advise rewiring the code so that OverlayManager has a getSingleton method that caches an instance of OverlayManager. Continue to export 'skins.minerva.scripts/overlayManager' but tie it to that singleton method instead.

https://gerrit.wikimedia.org/r/472477
https://gerrit.wikimedia.org/r/472479
https://gerrit.wikimedia.org/r/472480

Krinkle has objected to adding new mw.config options.

Yeh I saw that. FWIW I think many of the default configuration options are not useful on Minerva... and I feel this one is! Maybe in future we can explore pushing for this again when the current config state is in a cleaner state (see T186062 for example).

I am not sure if wgMinervaReadOnly/wgReadOnly is that useful anyway. Currently we use it to prevent opening the editor, but…

I would expect that read-only mode usually lasts for at most a few minutes (when it is triggered by database lag or something). So it is likely that a wiki that was read-only when the view mode page was opened is no longer read-only when the user tries to open the edit mode.

Instead of using a mw.config option, I think it would be better to query for read-only mode with the API request that loads the page wikitext, and treat it as a warning instead of an error. But since this isn't as trivial as just moving/renaming the option, we should probably discuss this in a separate task.

Change 472038 abandoned by Bartosz Dziewoński:
ResourceLoaderStartUpModule: Add 'wgReadOnly'

Reason:
Possibly this isn't a great idea anyway (see T198765#4732369). But regardless, it can stay in Minerva for now.

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

Change 472504 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/MobileFrontend@master] mobile.startup/Overlay: Compatibility with skins other than Minerva

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

Change 472505 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/skins/MinervaNeue@master] skins.minerva.editor: Remove unused dependencies

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

After all the new patches above are merged, I think we should be able to move 'skins.minerva.editor' to MobileFrontend without creating any weird dependencies on Minerva :D

Running mw.loader.load( 'skins.minerva.editor' ) will now load the mobile editor on any skin in mobile mode, and it will actually work and be able to edit pages!

Many things still look obviously broken when using it (a bunch of styles for the toolbar and buttons seem missing), but it kind of works, and the rest should be easy to track down (if perhaps tedious).

Here's how the mobile editor looks like on Vector right now ;)

matmarex changed the task status from Stalled to Open.Nov 8 2018, 6:11 PM
matmarex moved this task from In progress to Code review on the VisualEditor (Current work) board.

Please move this code to the "mobile.init" module when making that migration. That's designed to load on all pages running in mobile mode.

Change 472478 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Merge remote-tracking branch 'gerrit/page-issues-cleanup'

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

Change 472477 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] mobile.startup/OverlayManager: Add #getSingleton

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

Change 472479 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] skins.minerva.scripts/init: Use singleton OverlayManager from MF

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

Change 472480 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] skins.minerva.editor: Remove remaining uses of 'skins.minerva.scripts'

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

Change 472505 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] skins.minerva.editor: Remove unused dependencies

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

matmarex updated the task description. (Show Details)Nov 13 2018, 9:47 PM

Please move this code to the "mobile.init" module when making that migration. That's designed to load on all pages running in mobile mode.

Currently 'skins.minerva.editor' is actually not used on all pages. It is not loaded on special pages and on pages with content models that require a specialized editor (for example, on Wikisource: try editing https://en.m.wikisource.org/wiki/Page%3ATragedies_of_Euripides_(Way_1896)_v2.djvu/238). So I think it needs to remain a separate module.

Change 473288 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/MobileFrontend@master] Add 'mobile.editor' module (moved from Minerva)

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

Change 473289 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/skins/MinervaNeue@master] Remove 'skins.minerva.editor' module (moved to MobileFrontend)

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

Change 472504 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] mobile.startup/Overlay: Compatibility with skins other than Minerva

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

Goals

  1. Stop loading the mobile editor on desktop Minerva
  2. Move the code from Minerva to MobileFrontend
  3. Allow loading the mobile editor on other mobile skins

I added this to the task description just now, but it describes what I've been trying to do here from the start. But after some consideration, I'm going to remove #3 from the scope of this task.

I've done some work towards this (mostly in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/472504 and https://gerrit.wikimedia.org/r/c/mediawiki/skins/MinervaNeue/+/461855) and at this point, it is possible to use mw.loader.load( 'skins.minerva.editor' ) (or mw.loader.load( 'mobile.editor' )) to load the mobile editor on any skin, and it mostly works and edits pages. However, it looks horribly broken without a bunch of styles that are only present in Minerva (not even related to the editor itself, but more generally to mobile overlays – I suspect things like the category overlay would also be broken in a similar way).

The missing styles look like a mess and I think someone more familiar with the mobile site's code would be able to fix them up more easily than myself. Main goal here was tech debt cleanup rather than functional changes, so I feel good about this still. I think that right now, allowing other mobile skins is not really a priority for anyone, anyway.

Jdlrobson added a subscriber: Isarra.

Thanks! This is looking way cleaner! Maybe @Isarra can help us move this forward by consuming these in Timeless and giving us feedback on how they could work better?

matmarex updated the task description. (Show Details)Nov 13 2018, 10:45 PM

I'm not sure what you mean here by consuming. Does the skin needs to explicitly load it or something for it to activate? Or is this more of a VE-like deal where we just need to make sure the structure is actually, uh, what we expect?

Change 473288 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Add 'mobile.editor' module (moved from Minerva)

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

Change 473289 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Remove 'skins.minerva.editor' module (moved to MobileFrontend)

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

matmarex removed a project: Patch-For-Review.

This is done from my perspective. (The subtasks are also done, waiting on QA.) Thanks for all the reviews, @Jdlrobson!

@Esanders Can you take a look at the end result to see if it seems sane?

Change 475147 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] View's now pass isBorderBox as a property

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

(Unrelated patch, it is actually for T209007)

ppelberg closed this task as Resolved.Mar 13 2019, 6:57 PM