Page MenuHomePhabricator

VE support for skins should be done by adding appropriate anchors/ids/styles to the skins, and not by editing VE itself
Closed, InvalidPublic

Description

Apparently the only way to make a skin work with VE is to edit VE itself. This makes it very hard to make new skins compatible with VE, despite more and more third-party projects desiring such compatibility.

It should be the other way around - compatibility should be in the skin, not VE. The skin should be edited to meet VE's expectations (maybe have appropriate ids or whatever on things where it should be showing up around the content, or a js snippet specifying how it is supposed to show up, or whatever?), and modified and styled appropriately to look right with it.

And how to do this should be documented somewhere so we can actually, well, do it.

Event Timeline

Isarra created this task.Jan 17 2017, 9:32 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 17 2017, 9:32 PM

This proposal is selected for the Developer-Wishlist voting round and will be added to a MediaWiki page very soon. To the subscribers, or proposer of this task: please help modify the task description: add a brief summary (10-12 lines) of the problem that this proposal raises, topics discussed in the comments, and a proposed solution (if there is any yet). Remember to add a header with a title "Description," to your content. Please do so before February 5th, 12:00 pm UTC.

Tgr updated the task description. (Show Details)Feb 4 2017, 10:37 AM

Yeah, I would say this is definitely blocked on T28918: Provide clean interface to register content editing interfaces and probably most of T29292: Skinning system improvement (tracking). Right now the concept of editing in MediaWiki is very fuzzy.

Lack of documentation is another thing, but technically, I think this is possible. Essentially, your skin's HTML structure should be like Vector or MonoBook ;), then you can add yourself to $wgVisualEditorSupportedSkins. A few modules have custom styling for Vector or MonoBook or other, but you can achieve that with $wgResourceModuleSkinStyles.

I'm not sure the assertion of this bug is correct. As both skins and VE are plugins to MW core it is not clear cut one which side the code should go. You could argue that the code shouldn't be loaded unless VE is installed, or that the code shouldn't be loaded unless the skin is installed.

Apparently, we actually have this documented: https://www.mediawiki.org/wiki/VisualEditor/Skin_requirements :o

So that looks like it's actually all we need, at least for the basics. And I just didn't know it was there, or some such... be nice if we didn't have to add it to the array - if all the expected ids are there, just have it try, and if something goes horribly wrong, let it go horribly wrong. Obvious horrible-looking bugs are often much easier to fix than why is there no support at all.

I'm not sure the assertion of this bug is correct. As both skins and VE are plugins to MW core it is not clear cut one which side the code should go. You could argue that the code shouldn't be loaded unless VE is installed, or that the code shouldn't be loaded unless the skin is installed.

Well, ideally special handling for a skin for an extension should be a separate module depending on both, regardless of which it's actually in. That does, of course, assume it's a loadable module, but from the look of it that should be about all will be needed anyway? Personally I'm leaning toward putting the module in question in the skin, since skins are generally full of weird styles and stuff for all the other bits already, and that way anyone making a new skin also doesn't need to go editing all these random extensions, but obviously I'm biased as a skin creator...

Actually no, the specific styles, at least, should always be in the skin. That way variables and mixins for colours and fonts and such can be reused and it's up to the skin to handle all interactions between the non-skin component and the rest of the interface (and the component only handles everything in it, itself). Which should result in most agnostic styles in components.

I mean, the skin-specific styles for the extension.

Apparently, we actually have this documented: https://www.mediawiki.org/wiki/VisualEditor/Skin_requirements :o

So that looks like it's actually all we need, at least for the basics. And I just didn't know it was there, or some such... be nice if we didn't have to add it to the array - if all the expected ids are there, just have it try, and if something goes horribly wrong, let it go horribly wrong. Obvious horrible-looking bugs are often much easier to fix than why is there no support at all.

I think in most cases, if we just removed the check for $wgVisualEditorSupportedSkins and the skin did not have the expected ids/classes, the effect would be that nothing would happen. (Or perhaps we'd get as far as adding the "Edit"/"Edit source" tabs, but upon clicking "Edit", nothing would happen.) We mostly rely on jQuery for modifying DOM and that is what usually happens when jQuery is told to modify non-existing elements. That probably wouldn't be helpful for debugging things either.

If we wanted to have something actually useful, then we should probably replace the check for $wgVisualEditorSupportedSkins with a check for all the expected elements, and perhaps log a warning to the console if it doesn't pass (with a list of missing ids/classes) or something.

Or perhaps we'd get as far as adding the "Edit"/"Edit source" tabs, but upon clicking "Edit", nothing would happen.

That's what used to happen. This is just as confusing, really, since now all the edit links turn to edit source, but that's the only option anyway. Basically, either isn't ideal, but if the skin is using non-standard ids, that's their own fault, and ours a bit for not making it clear what ones are needed in the first place.

If we wanted to have something actually useful, then we should probably replace the check for $wgVisualEditorSupportedSkins with a check for all the expected elements, and perhaps log a warning to the console if it doesn't pass (with a list of missing ids/classes) or something.

This would be good. And probably relatively trivial to do.

If the elements are there and the visuals just come out looking weird (like what you described when you tried setting all the source bits in Timeless), that's easy enough to deal with. That's what we're normally dealing with, with skin/extension compatibility.

OK, so I guess this task is invalid. But instead, we should do these:

  • Add links to https://www.mediawiki.org/wiki/VisualEditor/Skin_requirements from some more places related to VisualEditor and/or skinning
  • Replace the check for $wgVisualEditorSupportedSkins with a check for all the elements we expect a skin to provide
  • Move out skin-specific stylesheets for Vector, MonoBook, Apex and Minerva to their respective repositories (using $wgResourceModuleSkinStyles)
Isarra closed this task as Invalid.Mar 25 2017, 6:30 AM

Superseded by the above.