Prevent Media Library search making an AJAX request for each char typed.

[brandomeniconi]

I've noticed that while searching for a media in the "Media Library", WP makes an AJAX request immediately after a char is typed. So if I type in a long word, it will do a lot of sequential AJAX requests to the server.

In most small site environments that is fine, but in some large WP installations with a lot of media (and traffic), sometimes each request takes few seconds to complete, causing the latest requests failing due to queue timeout and the Library interface to hang.

I will be great if, while the user is typing, the UI only makes an AJAX request if the user has stopped writing for a determined amount of time. I've noticed that there is already a _.debounce handler in media-grid.js but it doesn't cancel the intermediate requests.

I've inspected this in few sites (with WP 4.6.1 ad very few plugins) and all have this behavior.

[adamsilverstein]

In 38911.diff​ :

• Debounce the media grid search. Also remove the extraneous change and search handlers.

[certainstrings]

Downloaded and tested. Patch resolved the issue for me.

[adamsilverstein]

@brandomeniconi Thanks for the report, I'm surprised we weren't debouncing this event already! In my patch I also removed what appear to be duplicate event handlers.

[brandomeniconi]

Great! Thanks for the patch.

[joemcgill]

38911.diff​ Looks pretty good, though the debounce time of 500ms feels a bit slow to me. I would target somewhere around 200-300ms for this interaction.

Also, it looks like the search and change handlers were added by @koop in [22533] to "properly capture clearing the search box", but I'm not seeing any side affects when these are removed. @adamsilverstein were you seeing other side affects by leaving the extra handlers in?

[adamsilverstein]

Replying to joemcgill:

38911.diff​ Looks pretty good, though the debounce time of 500ms feels a bit slow to me. I would target somewhere around 200-300ms for this interaction.

@joemcgill Thanks for the feedback! I agree - it feels a little slow to me as well.

I chose 500 after checking to see what we do elsewhere with ajax search boxes, see customize-nav-menus.js, themes.js , updates.js. Core is somewhat inconsistent - I also found some spots where we used 250 (widgets) and 1000 (media grid). I read somewhere recently (I can't seem to find now) that the average user pauses something like 400ms between keystrokes, I think that is the reason we chose to use 500.

Also, it looks like the search and change handlers were added by @koop in [22533] to "properly capture clearing the search box", but I'm not seeing any side affects when these are removed. @adamsilverstein were you seeing other side affects by leaving the extra handlers in?

Before the debounce I was seeing the the event fire multiple times per keystroke as the duplicate events were fired. Although the debounce fixes that, we can still remove thechange and search event handlers - keyup and input should cover all supported browsers and we use them in the three examples I linked above.

[joemcgill]

I also found some spots where we used 250 (widgets) and 1000 (media grid).

This does not surprise me. There's a bit of a balance to be had here. A shorter time (e.g, 250) would most likely lead to extra requests when people type slower, and a longer time leads to a perception of slowness between typing and experiencing the response.

This reminds me of when mobile browsers use to put a 300ms delay on all touch events to see if the user was going to double click. Browsers ​eventually removed the delay because people could detect the delay. Not saying this is exactly the same case as click response, but I would lean towards making the UX feel a bit snappier even at the cost of a few additional requests, since ​some research shows that people can sense any delay over 100ms.

[aidanlane]

+1 for the patch! I was ready to file the same issue.
This issue really hurts us with our massive media library when typing long keywords.

Question @adamsilverstein: what do you mean by 1000ms on media grid? This issue affects the media grid, right? (apologies for the lame question).

Thanks all!

[adamsilverstein]

@aidanlane thanks for testing. the 1000 ms delay is in the code that sets up the media grid (media-grid.js:L667 in bindSearchHandler), I will test to see where that is used.

[aidanlane]

Replying to adamsilverstein:

@aidanlane thanks for testing. the 1000 ms delay is in the code that sets up the media grid (media-grid.js:L667 in bindSearchHandler), I will test to see where that is used.

On closer inspection, it looks like bindSearchHandler is doing the right thing for us after all, with 1000ms debouncing. Looks like we just need to improve our search speed (which is currently slow due to a rubbish media categories plugin).

[adamsilverstein]

@aidanlane I see a search request on every keystroke without this patch.

I looked into the existing 1000ms debounce i mentioned in media-grid - that is actually used to reset the search url in the browser... if you type a search (without the patch), you will see the ajax request fire on every key request, then when you pause typing, the search is added to the url after 1 second of not typing.

Here is a screencast with the console open showing the excessive ajax requests:

After the patch, it looks like this:

[adamsilverstein]

300 ms debounce

[adamsilverstein]

@joemcgill I updated this with a 300 ms debounce, what do you think?

[joemcgill]

Replying to adamsilverstein:

@joemcgill I updated this with a 300 ms debounce, what do you think?

300ms seems like a reasonable number to me. A little delay, but not terrible.

[Kelderic]

Just tested the patch and don't see any issues.

[batmoo]

This looks good and works well.

Minor thing: do we need the separate debounceSearch function? Can we just wrap search in _.debounce() instead?

[adamsilverstein]

Minor thing: do we need the separate debounceSearch function? Can we just wrap search in _.debounce() instead?

Thanks for the feedback @batmoo.

We probably could just wrap search with _.debounce. I like adding the debouncedSearchmethod because its name makes it clear the function is debounced when you invoke it. Looking thru core for other debounce uses, I see we do it a bit both ways - sometimes creating a new named function and sometimes wrapping a function directly. I'll give it a try that way, will probably make for a smaller diff.

[dd32]

Untested, but the approach taken in 38911.2.diff​ looks good to me. Debouncing all search actions like that looks like the best way forward.

@adamsilverstein What needs to be done here to get a bit of movement?

[adamsilverstein]

38911.3.diff​: debounce the search without creating a separate debouncedSearch function

[adamsilverstein]

@adamsilverstein What needs to be done here to get a bit of movement?

Thanks for the reminder, I'll get this committed shortly.

[adamsilverstein]

In 40060:

Media: debounce the media grid search, avoiding duplicate requests.

Add a debounce wrapper to the media grid search handler. The search callback is now fired after a 300 ms typing pause.

Remove redundant handlers for 'search' and 'change', preventing multiple/duplicate search callbacks.

Props certainstrings, joemcgill, Kelderic, batmoo.
Fixes #38911.