The proof read page example will need some input from VisualEditor (editing team) about what the best way to disable the editor for proof read page without the hook would be (maybe namespace based?)

ProofReadPage extension uses this hook to disable the editor - this should be replaced by a configuration flag in Minerva.

Not sure, but isn't config flag has more maintenance overhead than hook? Maybe ProofReadPage should use one of the core hooks and inspect whether the skin is Minerva and remove the module with that. Is that even possible?

It's not possible to remove JS modules in extensions. Extensions can only add. Minerva's hook does allow modification but I think that's an anti-pattern as it creates unnecessary fragmentation in the code that runs on Minerva.

We already disable the MobileFrontend editor if the content model is editable. If the editor also was aware of editors it could not support e.g. proofread-page this would complete the puzzle. https://fr.wikisource.org/wiki/Page:Nerval_-_Les_Illumin%C3%A9s,_L%C3%A9vy,_1868.djvu/130?action=info

/**
 * Checks whether the editor can handle the existing content handler type.
 *
 * @param Title $title
 * @return bool
 */
protected static function isPageContentModelEditable( Title $title ) : bool {
        $contentHandler = MediaWikiServices::getInstance()->getContentHandlerFactory()
                ->getContentHandler( $title->getContentModel() );

        return $contentHandler->supportsDirectEditing()
                && $contentHandler->supportsDirectApiEditing();
}

The Editing team can't work on this at the moment. @ifried if you think this is deserving of more attention from the Editing team let us know.

@JTannerWMF Thanks for the heads-up! I'm not quite clear on the background/context, so let me first ensure that this assessment is accurate: From my understanding, this ticket states that the SkinMinervaDefaultModules hook will be deprecated. This would mean that, if no additional changes are made, wikis that use the ProofReadPage extension would not be able to disable the editor. Instead, all wikis with the extension would have the same, standard editor? Is that correct?

I'm tagging @SGill & @Samwilson to provide feedback, as this would impact the ProofReadPage extension for Wikisource users. What does this mean for users of Wikisource & what do you think would be the best course of action? Thanks in advance!

This would mean that, if no additional changes are made, wikis that use the ProofReadPage extension would not be able to disable the editor. Instead, all wikis with the extension would have the same, standard editor? Is that correct?

Correct.

Longer term, I think it would be better if the editors were configured in such a way that they can be disabled on certain namespaces / content models, although I defer to editing experts on what that might look like.

Thanks for clarifying, @Jdlrobson!

Pinging some of our Wikisource experts: @Samwilson, @SGill, and @Prtksxna What do you think about this proposed change? Any recommendations for how the team should approach this update?

In T256086#6484551, @ifried wrote:

… This would mean that, if no additional changes are made, wikis that use the ProofReadPage extension would not be able to disable the editor. Instead, all wikis with the extension would have the same, standard editor?

You may already be aware, but since the phrasing here is ambiguous let me clarify anyway… If we're talking about Visual Editor the issue on the Wikisources is not a matter of preference: Visual Editor does not work with ProofreadPage and will break pages if used in PRP-managed namespaces! Or at least that was the state of affairs last I checked. If it worked with PRP I believe many Wikisourcen would enable it by default for its greater user-friendliness for new and inexperienced users.

Also, for whatever it's worth, in general terms, from the perspective of a technical contributor and admin on a Wikisource, I would expect to be able to enable or disable things like editors both on a per-namespace and per-content model basis. For PRP a namespace toggle would be sufficient (I think), but for lots of other things (in MediaWiki: or Template: or User:, say) the content model would be the sane way to control it.

In T256086#6515130, @Xover wrote:

In T256086#6484551, @ifried wrote:

… This would mean that, if no additional changes are made, wikis that use the ProofReadPage extension would not be able to disable the editor. Instead, all wikis with the extension would have the same, standard editor?

You may already be aware, but since the phrasing here is ambiguous let me clarify anyway… If we're talking about Visual Editor the issue on the Wikisources is not a matter of preference: Visual Editor does not work with ProofreadPage and will break pages if used in PRP-managed namespaces! Or at least that was the state of affairs last I checked. If it worked with PRP I believe many Wikisourcen would enable it by default for its greater user-friendliness for new and inexperienced users.

@Xover That isn't what this task is about... It's about deprecating a hook that specifically allows the proofreadpage extension to disable the javascript-based mobile editor. I don't think the visual editor and/or the standard editing configuration for ProofreadPage will change in any way for desktop users if this hook is deprecated.

In T256086#6515146, @Soda wrote:

… That isn't what this task is about... It's about deprecating a hook that specifically allows the proofreadpage extension to disable the javascript-based mobile editor. …

Thanks for clarifying. My main point, though, was that if this is just an issue similar to the 2007 wikitext editor vs. the 2010 wikitext editor it probably isn't a big deal; but if ProofreadPage currently disables the "javascript based editor" I imagine that's because it actually breaks stuff in PRP's namespaces, like the situation with VE on desktop, which would be a Very Bad Thing™.

My impression is that editing on mobile is so limited by form factor, and PRP isn't great on mobile to begin with, that apples-to-apples swaps of editor are unlikely to be a huge deal for these users (unlike with the editor wars on desktop). Actually breaking mobile editing for all PRP pages is a completely different matter though.

@ifried @Jdlrobson It seems that even on removing the hook, the (JS based) mobile editor is not loaded for Index: and Page: pages and the interface falls back on the non-javascript editor by default. Is this some unintended consequence or is it supposed to happen ? (Screen cap of chrome remote debugger attached)

Change 632346 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Flow@master] Remove SkinMinervaDefaultModules hook noop

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

Thanks a bunch for investigating this @Soda - yes it seems the ProofRead extension hook is not doing anything. MobileFrontend already checks the content model and disables the JS editor if the content model is not wikitext.

THe ProofRead hook appears to have no effect. It does:

unset( $modules['editor'] );

but there is no module group with that key so this doesn't do anything

Flow unsets toggling and there are no modules that match that either.

So looks like the following needs to happen:

• Remove the SkinMinervaDefaultModules hook declaration in ProofreadPage
• Remove the instance in Flow
• Begin the deprecation.

I've written the patch for Flow. @Soda can you make a change for ProofRead ?

Change 632359 had a related patch set uploaded (by Sohom Datta; owner: Sohom Datta):
[mediawiki/extensions/ProofreadPage@master] Remove SkinMinervaDefaultModules hook

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

In T256086#6519529, @Jdlrobson wrote:

I've written the patch for Flow. @Soda can you make a change for ProofRead ?

Sure, uploaded the patch!

Change 632346 merged by jenkins-bot:
[mediawiki/extensions/Flow@master] Remove SkinMinervaDefaultModules hook noop

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

Change 632359 merged by jenkins-bot:
[mediawiki/extensions/ProofreadPage@master] Remove SkinMinervaDefaultModules hook

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

Thanks all for unblocking this one!

Change 637538 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Deprecate SkinMinervaDefaultModules hook

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

Change 637538 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Deprecate SkinMinervaDefaultModules hook

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