Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#38911 closed enhancement (fixed)

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

Reported by: brandomeniconi's profile brandomeniconi Owned by: adamsilverstein's profile adamsilverstein
Milestone: 4.8 Priority: normal
Severity: normal Version: 4.6.1
Component: Media Keywords: has-patch commit
Focuses: javascript, administration, performance Cc:

Description

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.

Attachments (3)

38911.diff (1.4 KB) - added by adamsilverstein 7 years ago.
38911.2.diff (1.4 KB) - added by adamsilverstein 7 years ago.
300 ms debounce
38911.3.diff (1.3 KB) - added by adamsilverstein 7 years ago.

Download all attachments as: .zip

Change History (25)

#1 @adamsilverstein
7 years ago

In 38911.diff :

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

#2 @certainstrings
7 years ago

Downloaded and tested. Patch resolved the issue for me.

#3 @adamsilverstein
7 years ago

@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.

#4 @brandomeniconi
7 years ago

Great! Thanks for the patch.

#5 @adamsilverstein
7 years ago

  • Keywords has-patch 2nd-opinion added
  • Owner set to adamsilverstein
  • Status changed from new to assigned

#6 follow-up: @joemcgill
7 years ago

  • Keywords 2nd-opinion removed

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?

#7 in reply to: ↑ 6 @adamsilverstein
7 years ago

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.

#8 @joemcgill
7 years ago

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.

#9 @aidanlane
7 years ago

+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!

#10 follow-up: @adamsilverstein
7 years ago

@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.

Last edited 7 years ago by adamsilverstein (previous) (diff)

#11 in reply to: ↑ 10 @aidanlane
7 years ago

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).

#12 @adamsilverstein
7 years ago

@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:

https://cl.ly/1r1U2z212M0n/Screen%20Recording%202017-01-07%20at%2006.28%20PM.gif

After the patch, it looks like this:

https://cl.ly/44220f091P2e/Screen%20Recording%202017-01-07%20at%2006.36%20PM.gif

Last edited 7 years ago by adamsilverstein (previous) (diff)

@adamsilverstein
7 years ago

300 ms debounce

#13 @adamsilverstein
7 years ago

  • Milestone changed from Awaiting Review to 4.8

#14 follow-up: @adamsilverstein
7 years ago

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

#15 in reply to: ↑ 14 @joemcgill
7 years ago

  • Keywords commit added

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.

#16 @Kelderic
7 years ago

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

#17 follow-up: @batmoo
7 years ago

This looks good and works well.

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

#18 in reply to: ↑ 17 @adamsilverstein
7 years ago

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.

#19 follow-up: @dd32
7 years ago

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?

#20 @adamsilverstein
7 years ago

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

#21 in reply to: ↑ 19 @adamsilverstein
7 years ago

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

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

#22 @adamsilverstein
7 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

Note: See TracTickets for help on using tickets.