Wikipedia:Code review/EasyLinks

EasyLinks

The following discussion is closed. Please do not modify it. Subsequent comments should be made on the appropriate discussion page. No further edits should be made to this discussion.

Code review of: EasyLinks
Review requested by Xover at 06:59, 12 May 2019 (UTC)[reply]

Proposed new code: User:Xover/EasyLinks.js

Specific issues / areas for feedback: Not a change to an existing script, but rather a new user script for which I would appreciate review and feedback (both at the level of code and functionality and docs, if you're so inclined).

I've written a user script to make it easier to grab internal wikilinks to diffs, specific page revisions, and sections. Mainly to scratch my own itch, but hopefully it'll be useful for others as well. However, since this is my first user script, I would appreciate a sanity check on the code (and docs, functionality, etc., if you're inclined) before I subject others to it.

Obvious stuff that might be considered problematic or for which there may be better solutions that I'm not aware of:

It uses a global to hold a reference to the instantiated OO.ui.WindowManager, so that the various .click() event handlers can use it to open the dialog window. It seemed odd to me that there is no apparent way to get a reference to an existing OO.ui.WindowManager instance; so perhaps there is and I just wasn't able to find it? Is a global ok for this purpose, or should I be trying to pass these references around some other way (attach it as data to the DOM nodes? pass it in the jQuery.event object?)? Is the pseudo-namespace approach I've taken a good way to handle globals? Is it likely to break anything else? Are there perhaps conventions for this in user scripts that I'm not following?
Because the (async) Clipboard API is not yet usable in production, EasyLinks relies on the old quasi-API for cut&paste: document.execCommand("copy"). Because this API mimics the interactive "Copy" command (vs. manipulating the clipboard directly and programatically) it inherently requires that the data to be put on the clipboard must first exist somewhere as a text selection. This in turn means EasyLinks must insert the links into the page somewhere before selecting and copying. In addition to this, even the old clipboard API has limitations in browser support (e.g. Safari added support in a 2016 tech preview).
The approach I've picked is to pop up a OO.ui.MessageDialog with an embedded OO.ui.TextInputWidget to hold the wikilink. This is both to be transparent to the user about what's happening, and to provide a manual fallback if copying fails: the dialog remains up so the user can hit Ctrl-c and dismiss the dialog with the mouse (or hit Enter). Is popping up such dialogs appropriate for this interaction? When automatic copying works it will only briefly flash up and then disappear, which is not a particularly good user interface. Would it be better to insert a simple text field or span somewhere unobtrusive and remove it afterwards?
Is the way I'm reusing the MessageDialog windows and WindowManager correct, or am I leaking objects or creating other such problems?
The current code has almost no actual error handling and mainly just relies on fallbacks for missing browser support. Are there any obvious cases I should be checking for or add error handling for? Conversely, are there any cases that EasyList could or should support but don't? e.g. there are some situations where the &diff= parameter in the URL is prev or cur (instead of an actual diff or rev id). I haven't figured out in what situations that happens and I'm not sure if there's any way to make EasyLinks' various functions work in those situations.
Is p-cactions the right place to put the portlets? And I'm just throwing them in there, so random timing and JS load order determines where in the menu the EasyLinks functions end up. Should I be trying to add a specific position or order? Should I be looking at creating my own group like Twinkle does?
Is the way I'm inserting the JS pseudo links in section headings (after the section edit links) appropriate? Maybe I should have them float above or below the heading, or progressively display in a popup when the heading is :hover'ed? Does it create any problems (e.g. for a11y) to stuff them into the heading?
I'll probably want to use EasyLinks on my other Wiikimedia projects (enWS, Commons, other language Wikipedias, etc.). Anything I should think about in terms of using this globally? Is it doing anything that is enwp-speciific? It won't work on Flow-based talk pages, but is it going to fall down and break badly there?
Anything else I should / should not do to be a good user script citizen? Anything I should be doing to avoid interfering with other user scripts? Are there any facilities I should be providing for other user scripts (no idea what that would be though. something to support introspection maybe?)?
Is the documentation adequate? Properly and clearly structured? Both for end users, admins and technical admins, and other script writers? I plan to add a couple of screenshots just to have a visual reference when the docs talk about the various functions, but I'm letting it bake a little first in case I need to change the user interface bits. Anything else I should think of here?
Any other feedback? Any and all thoughts would be appreciated (including "Sure, it looks fine." or "Meh." etc.; even stuff like that will tell me the author doesn't think this is horribly misguided, will set people's computers on fire, and probably destroy Wikipedia for good).

Thanks, Xover

Discussion

@Xover: I'll start this off with a note about a potential issue that I have come across - both times that you call mw.util.addPortletLink, you set the "url" target to #. I suggest changing it to javascript:void(0) - neither should take you anywhere, but using # has (for me) caused some issues with it working as a link to # and/or conflicting with other scripts that also use # for their triggers, since you might inadvertently trigger other code in other scripts too. --DannyS712 (talk) 22:52, 15 May 2019 (UTC)[reply]
- @DannyS712: Hmm. So far as I know there is no way for the href value to collide with other code, unless it is polling window.location or triggering off a .onfocus event somewhere awkward. As for following the link when clicked that's a problem when the handler returns something other than false (which the undefined returned by void(0) coerces to), for example because it errors out before getting to a return false; at the end of the handler (or because you forget to add it, of course). But in anything post-IE6 I believe the preferred way to handle that is with event.preventDefault() which explicitly tells the JS engine to suppress normal link behaviour. Granted I'm a rank amateur at this, but, anecdotally, this has worked fine in those cases I've had occasion to test (very small sample size, admittedly). --Xover (talk) 14:36, 16 May 2019 (UTC)[reply]

The discussion above is closed. Please do not modify it. Subsequent comments should be made on the appropriate discussion page. No further edits should be made to this discussion.