Page MenuHomePhabricator

Enable access to OOUI elements for DWIM gadgets (and maybe others)
Open, Needs TriagePublic

Description

Hebrew and Russian Wikipedias each have a nifty gadget that will generate extra autocomplete results that can catch “wrong-keyboard” input. Wrong-keyboard input happens when, for example, you have the US English keyboard enabled, but you type as if you expect Russian Cyrillic. So you could get ,jutvcrfz hfgcjlbz on the screen when you meant to type богемская рапсодия.

The way the DWIM (“do what I mean”) gadgets work is that if autocomplete gets fewer than 10 results, they do the transformation, and tack on any additional results, up to 10 total.

On Hebrew Wikipedia, you can see it action by typing tufk in the upper corner search box—you get one Latin result (“Turkish Airline”) and then a bunch of Hebrew results, starting with “אוכל”. (On Hebrew Wikipedia, DWIM is enabled by default. On Russian Wikipedia it must be turned on in preferences. This makes it easier to illustrate some things with Hebrew, some with Russian.)


As is, it works well in the top corner search, but not on the search on the main Special:Search page. It’s easier to play around with it on Russian Wikipedia because it is off by default and you can load any version of it you want in the Javascript console. The code for the Russian version is here. (The code for the Hebrew version, which has the same main process but loads a little differently, is here.)


I think DWIM might have once worked on the main Special:Search input box—where it would still be equally useful to have, as well as being more consistent across different parts of the UI. In the Russian gadget code, if you add the selector [name=search] to the list in the variable $searchBoxes, it sort of works, but not actually correctly.

For a Russian example—assuming you've enabled the gadget (“Показывать дополнительные подсказки в «малом» поле поиска при использовании неверной раскладки” in your preferences) or loaded the code in the console—ehfk gives one Latin suggestion ("EHF") and then a bunch of Cyrillic ones, starting with “Урал”.

What goes wrong in the main search box is that it does the wrong-keyboard updates, but the old results are there, too! It’s easiest to see with a query that gets 10+ results (and thus no wrong-keyboard results), like John, where you can see both sets of results—the gadget-generated black text results are on top, more closely spaced together, while the OOUI blue text results, more widely spaced, are poking out below the other list.

This screenshot shows the modified set of selectors in the Javascript console, along with the gadget results overlaid on the OOUI results.

I tried digging into the OOUI code to figure out how to insert/replace results in the right element, but I just got lost. I’m not up on Javascript mixins or OOUI, or even the mechanism of the DWIM code, so I have no idea where to go next.

Is there is some obvious way to make the code do the right thing for the Special:Search main search input box?

A working hack might be good enough right now. A proper way to access and modify this functionality via OOUI would be great. Perhaps the solution requires more fundamental work on OOUI to make that possible.

Event Timeline

TJones created this task.Feb 5 2019, 8:45 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 5 2019, 8:45 PM

Well, users can do var searchInputWidget = OO.ui.infuse($('#searchText')); to get a handle on the OOUI widget. Is that not sufficient?

TJones added a comment.Feb 5 2019, 9:32 PM

Well, users can do var searchInputWidget = OO.ui.infuse($('#searchText')); to get a handle on the OOUI widget. Is that not sufficient?

I don't know, is it? This is relatively far outside my area of expertise, and I'm just the conduit for the question. Have you tried it?

Supporting gadgets and other community-supported code is outside the Foundation's remit, sorry. If there's a specific request for a change to allow something I can look at that.

Mooeypoo added a comment.EditedFeb 5 2019, 9:46 PM

Well, users can do var searchInputWidget = OO.ui.infuse($('#searchText')); to get a handle on the OOUI widget. Is that not sufficient?

Unfortunately, no. The problem as I understood it while looking at the gadgets is that the gadgets need to override the way that the menu filters the results. Instead of going for "simple" filtering showing results fitting the given string, the gadget seems to first "grab" that string, translate it to a different charset (so tufk becomes "אוכל") and then requests the item filtering.

Off the top of my head, there are two problems here:

  1. The gadget creates its own dropdown, which is then shown on top of the OOUI one. This should go away. But it can only go away if we resolve 2 --
  2. The gadget needs to do something that is currently an internal operation of the widget (we should let the gadget define (or refine?) the result set.

Again, off the top of my head and very generally without delving into the feasibility of this, here are two options I see

Option 1: Slightly hacky -- extend the widget + add a hook

  • For the search bar, create an mw.widget that extends OO.ui.MenuSelectWidget
  • Add a hook either in #updateItemVisibility or deeper (and probably more correct) in getItemMatcher allowing the gadgets to change the way items are matched.

Option 2: More sustainable; done within OOUI -- allow overriding getItemMatcher method

Another option, which is specifically for OOUI and may actually be a good upstreaming idea in general, is to change the constructor of MenuSelectWidget so it can override the getItemMatcher that's coming in from OO.ui.SelectWidget. If a method is given in the constructor, it should be used, otherwise the default should be the getItemMatcher from the original SelectWidget.

That, too, can solve the problem for the gadgets; they can then do something like

var searchWidget = new mw.widget.AdjustableSearchWidgetThing( {
   itemMatcher: function ( str ) { return /* whatever they need */ }
} );
$('#searchText').replaceWith( searchWidget.$element );

This should resolve that problem and give OOUI users a better handle on the item matching.

I'm more for option 2 unless I'm missing something more inherent, but I think allowing to override the itemMatcher is a good idea in general for the widget (we've needed that almost anywhere the select widget is used, including RCFilters) and as a byproduct it will also resolve this specific issue and allow it to be more consistent across wikis that want this gadget, too.

TJones added a comment.Feb 6 2019, 4:47 PM

Thanks for looking into this, @Mooeypoo! It's too bad that there isn't a way to make it work now, but I'm glad this provides another use case for potential enhancements to OOUI. If the extra functionality ever gets implemented, ping me if you remember!

Supporting gadgets and other community-supported code is outside the Foundation's remit, sorry. If there's a specific request for a change to allow something I can look at that.

Ah, I see. This was a follow-up to a brief conversation at All Hands, and a Phab ticket was decided as the best way to continue the conversation and pull in people who might be interested or knowledgeable, to find out whether this was a simple lack of information on my part, or if a more complex effort would be needed to make something work—which is what turned out to be the case.

The general problem of wrong-keyboard correction is on the search team's list of goals for this quarter, and while we're not specifically using or supporting the gadget, I did suggest some improvements to the Russian one and if there were an easy way to make it behave properly, I would probably take on the task of finding other places it could be usefully deployed and suggesting it to those communities.

So, I'm happy with Moriel's answer, since that answers the question of whether it's easy or not. If someone wants to close or decline this ticket, that's fine with me.

Change 488745 had a related patch set uploaded (by Mooeypoo; owner: Mooeypoo):
[mediawiki/core@master] SearchInputWidget: Allow custom manipulation of the query value for API

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

Change 488746 had a related patch set uploaded (by Mooeypoo; owner: Mooeypoo):
[oojs/ui@master] SelectWidget: Allow 'getItemMatcher' to be replaced by custom functions

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

This comment was removed by Mooeypoo.

Okay, so I had more time to actually delve into the way the code works, and I'm slightly amending my analysis. The problem with the gadgets isn't really how to filter through the itemMatcher, which only works if the entire set of items already exists -- the problem with the specific SearchWidget is the API call. We need to make it possible for those gadgets to adjust the query that's sent to the search API.

Having itemMatcher be overridable is still a very good idea, as we saw when implementing rcfilters, but the gadgets will benefit from having a way to manipulate the search widget directly.

There are two commits in this ticket, then:

  1. SelectWidget: Allow 'getItemMatcher' to be replaced by custom functions: https://gerrit.wikimedia.org/r/c/oojs/ui/+/488746
  2. SearchInputWidget: Allow custom manipulation of the query value for API: https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/488745

The second one will allow gadgets to do what you're asking for, @TJones . I'll need to take a closer look at the gadgets themselves, but when the above is merged, it will allow the gadget to use OOUI instead of the jquery plugin they're using by only "infusing" the search widget and then replacing the query value.

Something like this should work in the gadget:

searchWidget = OO.ui.infuse( $( '#searchText' ) );
searchWidget.setQueryValueFunction( function () {
	var val = this.getValue();
	/* whatever manipulation you want of the original string */
	return 'foo_' + val;
}.bind( searchWidget ) );

(The above is an example that will takeover the search widget in Special:Search and send to the API a string that is what the user typed in the input, prefixed with 'foo_'.)

TJones added a comment.Feb 8 2019, 5:51 PM

Thanks, @Mooeypoo! That looks like it could work. I really appreciate your explanations and your patches!

For gadget support, there is not way to change the widget class, and so the correct way to do this (by inheritance) is off the table as you have identified.

However, the proposed patch which provides a method to override a protoype method just seems like hiding a hack behind a façade of respectability. It would probably just be easier for the user to reach in an override the method themselves, then no code changes are required upstream:

searchWidget = OO.ui.infuse( $( '#searchText' ) );
searchWidget.getQueryValue = function () {
   return myTransform( this.getValue() );
};

If you wand to be a bit more robust and preserve the original getQueryValue definition (in case it changes in the future) you could use a slightly longer hack:

searchWidget = OO.ui.infuse( $( '#searchText' ) );
parentGetQueryValue = searchWidget.getQueryValue;
searchWidget.getQueryValue = function () {
  // "Parent" method
  var val = parentGetQueryValue.call( this );
  return myTransform( val );
};
Mooeypoo added a comment.EditedFeb 21 2019, 9:26 PM

I guess that's true, but the benefit of doing my approach is that we're setting up an API for it, which exposes the ability to do what you're suggesting.

My main concern is that while it's true you can override the getQueryValue function, we don't really want to encourage overriding OOUI methods as a whole and as a matter of practice. In this specific case that's warranted, but I worry that this would encourage others to also override (mistakenly or without actually knowing what the implications are) other methods.

On top of that, this would assume the target audience of this -- gadget writers -- know how to do the "trick" of getting the "faux parent" method like you wrote in your code snippet. It feels like a hack -- and it is one.

Moreover, if we expose this as a built-in allowed override, we will be able to add/remove/adjust the override internally in OOUI without breaking gadgets hacky intentional behavior. It would be a lot harder to maintain consistency even with this hacky-stuff if we allow/encourage others to override that method directly.

Architecturally, I think it's not a bad idea to purposefully expose this as a setter just for making sure that we don't encourage bad behavior and not assume good practices (or even "just" ooui-practices) on the side of the implementer. I don't think this is crucial either way, but I'd still go for officially exposing through an API rather than not.

I guess that's true, but the benefit of doing my approach is that we're setting up an API for it, which exposes the ability to do what you're suggesting.

Providing overridable methods in an API, so methods like SearchWidget#getQueryValue and OptionWidget#getMatchText already provide this functionality.

What is being asked for here is supporting a use case where the user (a gadget author) doesn't have access to the original code, and so wants to hack it to change the functionality instead. I think it's perfectly reasonable to expect hacks to be implemented as hacks, and not provide additional redundant APIs for them.

On top of that, this would assume the target audience of this -- gadget writers -- know how to do the "trick" of getting the "faux parent" method

The first snippet would also be acceptable for a hack.

Moreover, if we expose this as a built-in allowed override, we will be able to add/remove/adjust the override internally in OOUI without breaking gadgets hacky intentional behavior. It would be a lot harder to maintain consistency even with this hacky-stuff if we allow/encourage others to override that method directly.

By this argument we should we provide a setter for every method we can reasonable expect users to want to override, just in case they aren't in a position to subclass. I'm not sure that would be a good idea.