Media Library does not have a loading indicator

Description

When you have a large amount of images in the media library, and use the search function when adding new media, there is a noticeable lag from entering the search term to when any results (if any) show up.

This was confirmed on WPTavern.com which contains about 900 images. We did not see the same results on a site that only contained 300 images. However, on all sites if there are no results a delay is seen between the time a term is entered and when results are displayed.

So I'm suggesting that a placeholder image or a status indicator that tells users that the search process is occurring would solve this issue.

Patch works in some quick testing - needs some RTL, though. It also shows while the library is initially loading, which might not be a bad thing. And would be good for somebody with more Backbone chops to review.

From a code perspective, it is strange to me that we're augmenting ajax with Backbone.Events, and not just extending the media object. media.on('ajax', ... ) feels more obvious and useful than media.ajax.on('ajax', ...) -- Less repetition of "ajax," more obvious path to leverage media for other communication purposes.

When we implemented this type of functionality in the app I'm working on currently, we handled the spinner in this fashion (pseudo-code ahead):

I recently read an article (dangerous words, I know) that noted a situation where people perceived an interaction as *slower* after the addition of a spinner, even if the load time actually stayed the same or decreased [citation needed, I can look for it if desired]. The solution proposed was to only display the spinner after about 200ms, to give the action time to complete "invisibly" without drawing attention to the fact a request was being made. Now, adding that type of timeout increases complexity without any demonstrated rewards, so I don't think it's worth going down that path—Mainly bringing it up here because it is tangentially relevant and psychologically interesting, and I apparently like hearing the sound of my fingers on the keyboard.

I recently read an article (dangerous words, I know) that noted a situation where people perceived an interaction as *slower* after the addition of a spinner, even if the load time actually stayed the same or decreased

I think that this may have been cited originally for the media modal, actually. However, I don't think the issue here is a perception of slow - it's a perception of broken, because there's no "no items found" message or anything like that.

Anyway, sounds like we need a different approach, so punting from 3.7. Please feel free to turn that pseudo-code into real code and a patch :)

"However, I don't think the issue here is a perception of slow - it's a perception of broken, because there's no "no items found" message or anything like that."

That sums up my reasoning for creating the ticket in the first place. Although nothing is technically broken, there is the appearance that something is broken when I perform a search and it takes a little while for search results or no results to pop up. It's that delay in between actions that throws me for a loop.

Great to see KadamWhite and others give my ticket some love! Thank you!

kovshenin, I came to the same conclusion last night--good catch. helen, I'll have what I have posted by the end of the evening here. I've got the patch updated and working, I'm just talking with gcorne for a sanity check.

Spurred by a chat with @kadamwhite, I think that I came up with a simpler solution as seen in 24859.3.diff​.

Instead of binding to a global event bus, we can leverage the existing {{{updateContent()}} method, which is already bound to the main collection events. This seems better because the spinner will only show for the duration of the initial ajax request that populates the the initial set of attachments. I also incorporated a delay of 600ms before showing the spinner. The thinking is that showing the spinner anytime there is a query would leave the user feeling that the browser is slower than it really is.

I like .3.diff, but running a second AJAX request before the first one is finished reverses the toggle :) To reproduce, try searching for different stuff real quick, it may also help to change the delay from 600 to 10, or use sleep() in admin-ajax.php.

24859.4.diff​ uses the same approach but when calling toggleSpinner explicitly states whether the spinner needs to be shown or hidden.

Something about the .6 patch wasn't working for me; the spinner was always hidden immediately due to the way it was being initialized. I made some final adjustments and this is now working for me on every browser that can run natively in OSX. (see 24859.7.diff​)

I did remove a couple lines from the patch, such as the one where we were setting toggleSpinner: false, as part of the initial AttachmentsBrowser view definition; also (as I cannot but be me) I made some small style adjustments to the patch for consistency with the style guide in the handbook. :)

Not sure what was wrong with my local that made .6.diff not work, but I've moved the initial disabling of the spinner back to the init method to reduce the number of times it gets called superfluously. gcorne believes (and I agree) that making CSS adjustments to avoid having to manually hide it in this way may be a good enhancement.

Thought I had left this comment last night, but never submitted it - the timeout feels too long. 300 felt pretty good to me, but chatting with nacin he wondered if perhaps we should leave the timeout at 0 and debounce the hiding instead, as if you type fast you get jittering of the spinner restarting.

Added one option for triggering the parent AttachmentBrowser's toolbar spinner from within the Attachments view, using .views.parent.toolbar. Reaching up the sub-view chain feels a little janky. These views share a collection so I'd have preferred to do this with collection events, but since we DIY'd all the AJAX (collection.more(), etc) I'm finding that none of the usual request/sync events fire on this.collection.

.11.diff I think is ready for prime-time; on this one (24859.12.diff​) I'd like some peer review.

I think I typed up a comment here and then never posted it. Anyway, there is a race condition here as of [27438]. When doing a search, I was able to pretty trivially get the spinner to show and stick on, rather than hide again. Not sure what was going on. Will need to make sure these patches fix those problems.

24859.13.diff​ builds on @kadamwhite's work in 24859.12.diff​ by fixing the race condition that @nacin reported and making the delay value a property.

300 felt pretty good to me, but chatting with nacin he wondered if perhaps we should leave the timeout at 0 and debounce the hiding instead, as if you type fast you get jittering of the spinner restarting.

I would rather not show the spinner in cases where the async request is fast because when a spinner is shown the user feels like the UI responsiveness is slower than it actually is. There is a ​YUI post about this that links to a now defunct jsfiddle, so trusting the "results" requires blind faith. :)

I haven't tested 24859.13.diff​ locally, but it doesn't look like this would work -- we're only setting a timeout when we check if ( ! this.spinnerTimeout ) { , but we're never setting that property back to undef or 0. Wouldn't that make this only run the first time for each spinner? I'd assume we'd need to do something like this.spinnerTimeout = clearTimeout( this.spinnerTimeout ); (which would set it back to undefined).

I'm also of the opinion that 300's a preferable value to 600, because 2-300ms is roughly the threshold after which we stop perceiving something as instantaneous; I raised the 'perceived slowness' spinner issue up above but after playing with it for a while 300ms is the point at which I think it begins to do more good than harm.

I haven't tested 24859.13.diff​ locally, but it doesn't look like this would work -- we're only setting a timeout when we check if ( ! this.spinnerTimeout ) { , but we're never setting that property back to undef or 0. Wouldn't that make this only run the first time for each spinner? I'd assume we'd need to do something like this.spinnerTimeout = clearTimeout( this.spinnerTimeout ); (which would set it back to undefined).

As ​reported in the Alpha/Beta forum, there is an issue with the spinner not being hidden when editing a gallery. The issue stems from Attachments.more() not resolving the promise when attachments.mirroring is no longer true. See #22656 for some of the history behind why more() works the way it does.

Looked into this again. There is indeed a simple case, where a promised deferred never gets resolved or rejected. It happens when a second .more() is fired which changes the collection being mirrored. It's usually not a problem, because the newer call to .more() also watches the new promise and hides the spinner on done().

However, if that newer call results in a cached query with a non-empty result, our code to hide the spinner in AttachmentsBrowser.updateContent() never kicks in. It's pretty easy to reproduce and easier if you add sleep(3) to admin-ajax.php: try searching for something and erasing your search before it finished loading. Quickly toggling between filters (All - Images - All) has the same effect.

In 24859.15.diff​: hide the spinner if updateContent was called with a non-empty collection.

This is feeling good to me, solely barring that the search is not debounced so if you type a huge long string of gibberish it can appear that the spinner is stuck. But I'm going to call that an edge case that only really happens when somebody (like me) is testing, and if we want to fix it, something to do on a new ticket.

When the modal first opens, it queries for 40 attachments. During this time, a spinner is shown. A second query is pretty much always triggered though. This results in a spinner, followed by 40 images (enough to fill the browser on most screens), followed by no spinner, then the spinner again, then 40 images (the addition of which you might not have noticed). Something here is not right. We could probably continuously show the spinner while the modal is being more or less initialized, though that's also a bit much spinner-wise.

Only show the media library loading spinner if we're scrolled toward the bottom. Prevents the spinner from flashing a second time when first loading the library due to a second query firing after initial load.

It looks like the undefined error pops up when scroll() is called for a view that has already been removed.

To reproduce you simply need to change the view while the library is still loading additional content, and when it's done it will call scroll() on a detached view. The reason @ocean90's steps to reproduce worked (for some people) is because "fo" matched some content but "foo" didn't, so there's a switch to the UploaderInline view.

Anyway, this is probably the reason we have that :visible check in place, so looks like we do need it after all. See 24859.17.diff​.