warn/xfd: Replace jquery.chosen menus with select2 #692
Conversation
There was a problem hiding this comment.
This cannot be merged as-is for obvious reasons. I think getting Select2 into ResourceLoader requires WMF intervention (maybe?), so I've added the "Depends on MW" tag. The other possible solution is to use the Toolforge CDN, if it happens to get whitelisted.
|
Just noting that I agree with @MusikAnimal on this. Loading external code from non-WMF sites is something I think Twinkle users would be fairly uncomfortable with, and given the wide usage of TW, simply isn't safe. |
|
@MusikAnimal any suggestion on how to move ahead with this, considering that we can all agree that is an improvement. (Also, the delsort stuff added in #143 is turning out to be widely popular among users - it's time we provide a more pleasant UX for that.) Why isn't wmflabs.org a whitelisted domain, when the Toolforge CDN homepage says it's a "Collection of very useful JS / CSS libraries served in a fast, Wikimedia Privacy Policy friendly way." ? Should I open a phab ticket for inclusion of select2 into ResourceLoader? Or would it be shot down with the standard "please use OOUI instead" response? I am not very sure whether OOUI can be used to do what we're doing with select2 here, but select2 definitely can be used for a lot of stuff that can't be done using OOUI at present. |
T223840: Can/should *.wmflabs.org be added to the default-src Content Security Policy? |
Didn't go over it thoroughly. Would the reasons why *.wmflabs.org is disallowed also apply to tools-static.wmflabs.org, noting that tools-static only hosts trustworthy libraries and no user-created tools? |
|
There's arguments to be made for whitelisting certain domains, but to some folks * means * so ¯_(ツ)_/¯. I suppose, per @MusikAnimal, the interim use of the cdn is okay-ish? I dunno, I don't think the CSP is going to get implemented for a while, as there are plenty of *.wmflabs things that are relied upon. |
|
I also don't think the CSP headers are going to be implemented in the very near future, or at least we'll have some sort of solution for whitelisting domains by then (and I concur /cdnjs should be whitelisted for everyone). I should note tools-static.wmflabs.org does go down sometimes... but lots of things break when this happens. The only weird bit here is that Twinkle is an on-wiki gadget, so one wouldn't expect it to break when everything on-wiki is otherwise working fine. I guess we could upload the select2 assets to the wiki, and declare them as a dependency in the gadget definition. This would put them behind ResourceLoader. However I'm not sure how acceptable this practice is, as it is third-party code. I'm assuming the licensing is fine, so long as it's prominently stated at the top, otherwise it inherently is published under CC-BY-SA. I think we can give the Toolforge CDN a try for now. My only real concern was pulling from Cloudfare or anything outside WMF. I have not tested this PR. |
siddharthvp
force-pushed
the
select2x
branch
3 times, most recently
from
23eeda0 to
2cc441b
Compare
- Unify the code for highlightening search results from warn and xfd modules into Morebits.select2SearchHighlights - Don't load select2 if it has already loaded. - Don't apply select2 transformation to the select field if select2 failed to load, such as when the CDN server is down.
See commit 2: The code where select2 acts on the interface is put into a |
This is the only way to prevent it from being loaded twice (once each time from twinklewarn and twinklexfd), resulting in double the number of console warnings.
|
Ok, a new insanity has come up: as reported on WT:TW, the behaviour of the jquery.chosen-based select menu has changed for the worse (at least on Chrome in Win10 - quite alarming as this is probably the most popular browser-OS combination). I haven't bothered to investigate the reason (could be due to an update in chosen or something else). Returning to this after 2 months, I've resolved the bug above (just a loader problem), and I think this is almost ready for merge. Before that, a couple of things need to be settled:
|
|
Apologies for taking so long to get around to this @siddharthvp and @MusikAnimal, I misread some emails and thought you two had agreed to wait. I see now I was very wrong. I can test, review, and help make changes in the next couple of days, but wanted to make some broad comments to the above beforehand:
|
|
Regarding Regarding users: 4 enabled it when the early chosen menu was very bad, and 1 now since the new issues started appearing. There's just 1 person who enabled it in between! |
There was a problem hiding this comment.
This is fabulous, @siddharthvp! Thanks as always for the thorough work. There are a few issues here but this is pretty damn good!
I didn't go into the visual changes/tweaks, as that way lies madness; I'll get around to it later, but they look fine.
| if (mw.config.get('wgRelevantUserName')) { | ||
| Twinkle.addPortletLink(Twinkle.warn.callback, 'Warn', 'tw-warn', 'Warn/notify user'); | ||
| if (Twinkle.getPref('autoMenuAfterRollback') && mw.config.get('wgNamespaceNumber') === 3 && | ||
| mw.util.getParamValue('vanarticle') && !mw.util.getParamValue('friendlywelcome') && !mw.util.getParamValue('noautowarn')) { | ||
| if (Twinkle.getPref('autoMenuAfterRollback') && |
There was a problem hiding this comment.
Maybe it's just me so don't change anything, but I generally like a couple together, even if they go over the mythical 80. I guess this is easier to read and makes indenting less annoying, but bleh.
I suppose the real answer is I should get a bigger monitor! 😆
|
Precipitous timing, the chosen folks just marked the latest as deprecated: harvesthq/chosen@91041bc That shouldn't affect anything (cdnjs doesn't really cull) but all the more reason for this. This PR is definitely going to go through, mutatis mutandis, so I've gone and opened #812 to take care of the work of adding select2 to the repo. |
- Add noemail and nocreate items when issuing just a partial template
- Fix handling of partial parameter, was bugging out when just issuing a template
- Skip saveFieldset for the select2 menus, easier to just do `.join('|')` for the query
- CSS tweaks a la wikimedia-gadgets#692
modules/twinklewarn.js
Outdated
|
|
||
| // Reduce font size |
There was a problem hiding this comment.
| // Reduce font size | |
| // Increase font size |
There was a problem hiding this comment.
This must be platform-dependent, because I was seeing a default font size of a massive 16px. Let's change the word to "Adjust".
There was a problem hiding this comment.
What percent was that? I can't check my system tonight but it was around 11.5px iirc.
|
@Amorymeltzer Dealt with mostly everything. Also did away with the hacky way of highlighting search hits with the more native-ish way using |
|
Thus far it all looks good? Would you mind explaining the Speaking of which, |
|
@siddharthvp @MusikAnimal Regarding #692 (comment), is it worth removing the |
Amorymeltzer
dismissed
their stale review
Changes look good AFAICT; one question about if removals, but otherwise LGTM!
Partial blocks (https://phabricator.wikimedia.org/T190350) were turned on following an RfC (https://en.wikipedia.org/wiki/Wikipedia:Requests_for_comment/Partial_blocks), so it's time to support them. Done so by adding a checkbox that toggles a "partial" status for both the blocking and templating behaviors. In order to support entering specific pages, the modules needs to support multiple custom user inputs, which can't be done nicely with chosen (See harvesthq/chosen#166). We can, however, use select2's tags system, recently added in wikimedia-gadgets#812 to make wikimedia-gadgets#692 (warn/xfd) happen. This is all an active WIP, including the policy (WP:PB) and the/any templates (right now, only {{uw-pblock}}) so a lot of this is likely to be in flux for a while. Some stray notes: - Adds a `defaultToPartialBlocks` preference option - Makes use of a hidden partial item to help build the query (like `reblock`) - Includes checks to prevent empty entries (T208645]) - Adds jumping boxes for `email`/`accountcreate` when just issuing a partial template - Will ignore the `email`/`accountcreate` template parameters if there's an `area` - CSS and select2 tweaks a la wikimedia-gadgets#692 - I've skipped proper processing in `saveFieldset` of the select2 menu items, it's easier to just do `.join('|')` for the query - This use a variable for formatted namespaces, namely "(Article)" rather than "". The empty string really is no good here; Morebits.wikipedia.namespacesFriendly previously handled this sort of thing, was removed in wikimedia-gadgets#600. - Compared to the rest of Twinkle, this is a weird module! Very different but pretty enjoyable all in all; @MusikAnimal built something really quite elegant here in wikimedia-gadgets#260. Closes wikimedia-gadgets#802.
| }, | ||
|
|
||
| /** Underline matched part of options */ | ||
| highlightSearchMatches: function(data) { |
There was a problem hiding this comment.
I said this already, @siddharthvp, but these really are quite well done. Way more concise than I ever would have thought possible.
There was a problem hiding this comment.
The idea comes from https://forums.select2.org/t/how-can-i-highlight-the-results-on-a-search/52/2. I just condensed down the code.
That should also explain the queryInterceptor (they've used the implicitly global query variable). Too bad that templateResult doesn't expose the search query as a parameter. This hack is unlikely to break, unless select2 makes some deliberate breaking changes.
Amorymeltzer
changed the title
This fully fixes the bug [described here](https://en.wikipedia.org/wiki/Wikipedia_talk:Twinkle/Archive_41#Long_previews_overlap_and_block_%22Submit%22_button) (closes #665), also eliminating the stop-gap solution of removing the resize handles. The problems associated with using jquery.chosen within a modal dialog (like in all TW scripts) are not there in select2. Also, select2 is better both in terms of customizability and programmatic control, not to mention having a proper documentation (unlike chosen). select2 added in #812. This also adds Morebits.select2 with functions to enhance the select2 menus: - matcher: show entire optgroup if matching the optgroup title - highlightSearchMatches underline the search term in matches - autostart: begin the search upon keydown
* 692-useselect2-notchosen-fixup: warn/xfd: replace jquery.chosen with select2 (#692)
|
Merged as 4a1330c |
|
Thanks for all your hardwork and research on getting select2 to load! Just tried this out on enwiki. Smooth and amazing! |
Per follow-up in wikimedia-gadgets#971 from the addition in wikimedia-gadgets#692, these appear unnecessary.
Per follow-up in wikimedia-gadgets#971 from the addition in wikimedia-gadgets#692, these appear unnecessary.
Replace jQuery Chosen select menu with select2.
This fully fixes the bug described here (closes #665), also eliminating the stop-gap solution of removing the resize handles. The problems associated with using jquery.chosen within a modal dialog (like in all TW scripts) are not there in select2. Also, select2 is better both in terms of customizability and programmatic control, not to mention having a proper documentation (unlike chosen). Select2 unfortunately doesn't natively support highlighting of search results, so I have implemented that manually (as I did in Tag module).
Select2 is being loaded using the CDN, which is obviously non-ideal, especially since the support for loading of scripts from non-WMF sites may likely be removed in the future. Before that happens, we should get select2 added to the ResourceLoader, maybe as a replacement to chosen.