r89950 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r89949‎ | r89950 | r89951 >
Date:08:41, 13 June 2011
Author:wikinaut
Status:resolved (Comments)
Tags:
Comment:
do not sent api requests for empty inputs, e.g. after a character input followed by a backspace in the search input. Make sure to clear the result div to wipe previous results at the client w/o sending a request. Lowers server load
Modified paths:
  • /trunk/extensions/Vector/modules/ext.vector.simpleSearch.js (modified) (history)

Diff [purge]

Index: trunk/extensions/Vector/modules/ext.vector.simpleSearch.js
@@ -40,26 +40,33 @@
4141
4242 // General suggestions functionality for all search boxes
4343 $( '#searchInput, #searchInput2, #powerSearchText, #searchText' )
 44+ .mouseover( function() { $(this).focus()
 45+ })
4446 .suggestions( {
4547 fetch: function( query ) {
4648 var $this = $(this);
47 - var request = $.ajax( {
48 - url: mw.config.get( 'wgScriptPath' ) + '/api.php',
49 - data: {
50 - action: 'opensearch',
51 - search: query,
52 - limit: 10,
53 - namespace: 0,
54 - suggest: ''
55 - },
56 - dataType: 'json',
57 - success: function( data ) {
58 - if ( $.isArray( data ) && 1 in data ) {
59 - $this.suggestions( 'suggestions', data[1] );
 49+ if ( query.length != 0 ) {
 50+ var request = $.ajax( {
 51+ url: mw.config.get( 'wgScriptPath' ) + '/api.php',
 52+ data: {
 53+ action: 'opensearch',
 54+ search: query,
 55+ namespace: 0,
 56+ suggest: ''
 57+ },
 58+ dataType: 'json',
 59+ success: function( data ) {
 60+ if ( $.isArray( data ) && 1 in data ) {
 61+ $this.suggestions( 'suggestions', data[1] );
 62+ }
6063 }
61 - }
62 - });
63 - $(this).data( 'request', request );
 64+ });
 65+ $this.data( 'request', request );
 66+ } else {
 67+ // no need to send ajax request for empty input but clear the result div
 68+ var context = $this.data( 'suggestions-context' );
 69+ context.data.$container.hide();
 70+ }
6471 },
6572 cancel: function () {
6673 var request = $(this).data( 'request' );

Follow-up revisions

RevisionCommit summaryAuthorDate
r89952corrected commentwikinaut09:41, 13 June 2011
r89953corrected commentwikinaut09:43, 13 June 2011
r89956follow up patch to r89950 r89952 r89953 r89955 : moved div clearing inside ca...wikinaut11:16, 13 June 2011
r90186Minor fixed in ext.vector.simpleSearch...krinkle23:21, 15 June 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r89900expressly set opensearch limit. Found a reproducible problem that on a large ...wikinaut23:13, 11 June 2011

Comments

#Comment by Wikinaut (talk | contribs)   08:45, 13 June 2011

I forgot to comment, that this also auto-focuses the cursor when user hovers over the search input. It must be checked, whether this annoys users or not.

#Comment by Krinkle (talk | contribs)   08:43, 15 June 2011

At college we run a wiki off trunk, I must say that the browser focus changing and the placeholder "Search" disspearing when I sweep the mouse over the right corner is annoying.

I would suggest reverting this and perhaps opening a bug ticket for the underlaying improvement you want to make with this.

#Comment by Wikinaut (talk | contribs)   12:36, 15 June 2011

Krinkle, you should not move the mouse (=> risk of R.S.I.) except when you really have to, e.g. for entering something into the input box. Now, when the cursor is caugth, you can start typing your query. This is very convenient. I admit, that the text body should regain focus in conformity to my patch for the search input, when you hover over the body. I don't want to introduce too much at once.

Please try the new behaviour (search input auto-focus when hovering) live on http://www.translatewiki.net . It saves _one_ click on the mouse.

#Comment by Krinkle (talk | contribs)   20:38, 15 June 2011

It saves once click when the intention is to focus it, it costs an extra click to get back to the editor and or other input fields on the page.

In the case described in the previous reply it costed a few extra key strokes as well. The student was editing a page while moving/pushing the mouse towards the right. As a result part of the text was written in the search field and http requests were made for search suggestions.

Although I agree it looks cool when showing off (saving a click), I think in general the majority of our readers will not expect this and may be confused by it. To be consistent, perhaps it should unfocus (blur()) on mouseleave ?

Don't get me wrong though, I'm not saying this is a bad idea, merely saying that in my opinion it's too big of a change to just push in SVN as the new default that was committed as "minor detail" commited together with something else. It should be discussed further, perhaps cross-linked with Trevor and/or someone else from the Usability Initiative. Hence better tracked in BugZilla as a feature request.

#Comment by Nikerabbit (talk | contribs)   20:44, 15 June 2011

Yeah it doesn't seem to be very useful.

RE: unfocus on mouseleave – definitely not, then it would not be possible to click to have focus and move mouse away to see what one is typing.

#Comment by Nikerabbit (talk | contribs)   09:31, 15 June 2011

I don't know how common that it is. It hasn't bothered me yet.

#Comment by Trevor Parscal (WMF) (talk | contribs)   20:44, 15 June 2011

Focus on hover breaks user expectations, and should not be used. Keyboard focus should always be deliberate and predictable.

#Comment by Wikinaut (talk | contribs)   21:53, 15 June 2011

I would like to give this "closing" statement:

I use this technique in all of my (about hundred) software gadgets - not necessarily wikis, because with jQuery it costs nothing. _My_ users are very happy with this technique.

Please allow me to suggest this:

0. I admit, the the UI changed, but as mentioned: less clicks, if all elements get the focus automatically, including the body. 1. I ask you to leave the focus (here) as I coded it here ( r89950 ) for a while. 2. I open a bugzilla in the following days with a broader scope, i.e. to make _all_ relevant page elements auto-fetch the focus on hover, (also on click of course), and I will supply you with a mockup wiki running trunk having this technique for testing. 3. after having tested this mockup for you all, I will either commit this as a master change to all modules, or revert my single patch here for the search field. 4. deadline for end of tests: 30.06.2011

Tom

#Comment by Krinkle (talk | contribs)   23:22, 15 June 2011

Making mockups, discussion on a bug ticket, test phase, sounds like a good way about this.

However right now it's inconsistent in trunk (which some wikis are running on), so untill this phase is over I've removed it for now in r90186.

#Comment by Wikinaut (talk | contribs)   23:26, 15 June 2011

that was too quickly removed. I am very disappointed, that this innovative change was removed so quickly without polling the developers. I accept your view, sleep over it. You dropped a chance of Wikipedia introducing a smart feature seldom seen elsewhere.

#Comment by 😂 (talk | contribs)   23:32, 15 June 2011

I see 3 (plus me now, so 4) developers already objecting to the change, that's enough consensus to revert for me.

Revert early, revert often.

If we poll this for 3 months and then decide to remove it then, it makes the revert that much more difficult.

#Comment by Jorm (WMF) (talk | contribs)   23:46, 15 June 2011

+1; this needs reversion.

#Comment by P858snake (talk | contribs)   23:46, 15 June 2011

late to the game.... already has been.

#Comment by P858snake (talk | contribs)   23:42, 15 June 2011

We don't restrict views about code to "developers", if someone comments about it we take it into account, as pointed out 3 people have already objected to this, And we have also long held have had objections to auto focusing to the search field compared to where it is "Expected" to be on the page (weather it is done on page load or say automated on hover over)

#Comment by Wikinaut (talk | contribs)   23:45, 15 June 2011

Why not being innovative ?

#Comment by P858snake (talk | contribs)   23:46, 15 June 2011

innovative isn't always best.

#Comment by Krinkle (talk | contribs)   23:28, 15 June 2011

You do know that Wikipedia does not run on /trunk, right ?

#Comment by Brion VIBBER (talk | contribs)   23:38, 15 June 2011

Mockups are great, but please don't check anything like that into MediaWiki trunk again without getting a firm consensus on it first.

#Comment by Brion VIBBER (talk | contribs)   23:29, 15 June 2011

De-'OK'ing; as noted above there seems to be a serious focus-stealing problem?

#Comment by Brion VIBBER (talk | contribs)   23:33, 15 June 2011

Per Krinkle, the objected part has been removed in later commits (which commit? it's not listed as a follow-up at least describing a revert), so this should be resolved as 'resolved'.

#Comment by Brion VIBBER (talk | contribs)   23:36, 15 June 2011

Ok apparently it was done in r90186 (label above lists only "minor fixes", but the removal of the focus change gets mentioned several lines in)

#Comment by Wikinaut (talk | contribs)   23:36, 15 June 2011

focus-onmouseover discussions please go here : https://bugzilla.wikimedia.org/show_bug.cgi?id=29425

Status & tagging log