Media Library does not have a loading indicator

[jeffr0]

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.

[jeffr0]

This is what it looks like when searching for media files

[helen]

So like a spinner?

[jeffr0]

Replying to helen:

So like a spinner?

Yep. A simple spinner is what I was thinking. That way, the user knows that something is happening.

[DrewAPicture]

+1 for a spinner or some indication there's something happening.

[helen]

Seems like there should also be a spinner when anything is loading, including first open.

[DrewAPicture]

Maybe we can add a spinner in 3.7.

[kovshenin]

24859.diff​ is an attempt to add a spinner for some active ajax actions in the attachments browser view.

[helen]

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.

Related: #22839

[kadamwhite]

+1 on showing while the library is initially loading.

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

// media-views.js
media.view.AttachmentsBrowser = media.View.extend({
    // ...
    spinnerCount: 0,
    initialize: function() {
        // ...
        media.on('ajax:start', function() {
            this.spinnerCount += 1;
            this.updateSpinner();
        });
        media.on('ajax:end', function() {
            this.spinnerCount -= 1;
            this.updateSpinner();
        });
        // ...
    },
    updateSpinner: function( result, options ) {
        var spinnerActions = ['query-attachments'];

        if ( _.indexOf(spinnerActions, options.data.action) < 0 )
                return;

        if ( 0 < this.spinnerCount ) {
            this.toolbar.get('spinner').show();
        } else {
            this.toolbar.get('spinner').hide();
        }
    }
    // ...
});

// media-models.js
    ajax: function( action, options ) {
        var media = this,
            result;

        if ( _.isObject( action ) ) {
            options = action;
        }

        result = wp.ajax.send.apply( this, arguments );

        media.trigger('ajax:start');

        result.done(function() {
            // View doesn't have to care about interacting with the request
            media.trigger('ajax:end');
        });

        return result;
    }
//...
_.extend( media, Backbone.Events ); 

[kadamwhite]

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.

[helen]

Replying to kadamwhite:

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

[kadamwhite]

Replying to helen:

Please feel free to turn that pseudo-code into real code and a patch

I will endeavor to do so :)

[kadamwhite]

Alter patch to fire events on media object itself, clean up events when view is removed, and remove view's awareness of the ajax promise

[kadamwhite]

Bumping this now that we're into 3.8

[jeffr0]

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

[nacin]

kadamwhite, still happy with 24859.2.diff​?

[kadamwhite]

I'm in the process of refreshing this patch, and validating the add-Events-to-media approach with my colleagues

[helen]

Still refreshing, kadamwhite?

[kovshenin]

Looking at .2.diff. I like the general idea of ajax:start and ajax:end, however, the spinner count incr/decr has been moved away from updateSpinner and the check for spinnerActions. So in theory:

• ajax:start could increment the spinner count for query-attachments, spinner shown, count 1
• ajax:start increments the spinner count for an action called foo-bar, spinner shown, count 2
• ajax:end for query-attachments, spinner still shown, count back to 1
• ajax:end for foo-bar, count is 0, but the spinner is still shown because updateSpinner will bail for foo-bar

In reality though, I haven't found a neat way to reproduce this, but I think either way we should keep the action check closer to the spinner count.

[kadamwhite]

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.

[gcorne]

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.

[kovshenin]

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.

[gcorne]

Thanks @kovshenin for catching my mistake.

24859.5.diff​ improves upon your tweak by making the parameter a string so it is a little easier to read and switches to using the timeoutID to clear the timeout rather that using a flag.

[gcorne]

jQuery.toggle() uses true/false so we should, too. See 24859.6.diff​.

[kadamwhite]

Made it work in my environments, removed code that didn't seem to be needed, and style tweaks

[kadamwhite]

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

[kadamwhite]

moved setting the initial spinner to false back to where gcorne had it

[kadamwhite]

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.

[kadamwhite]

make gcorne's final css adjustment to avoid having to manually suppress the spinner

[gcorne]

I think this is ready to go.

[wonderboymusic]

In 27438:

Add a loading indicator to the Media Library.

Props kadamwhite, gcorne, kovshenin.
Fixes #24859.

[helen]

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.

[kadamwhite]

Drop delay to 300ms

[kadamwhite]

helen -- dropped the delay to 300ms, just as a start. (See 24859.10.diff​)

There's two additional things I'm working on here, currently:

1. Move the delay logic inside the spinner view, so that if we used it elsewhere we wouldn't have to duplicate this code; and
2. enable the lazy-loading scrolling to also trigger the spinner (for consistency with the search box, and to make it more obvious when all results have loaded).

Thoughts on 2 welcome; 1 is represented in 24859.11.diff​ (which also drops the timeout to 300ms).

[kadamwhite]

Move timeout logic inside Spinner view

[kadamwhite]

Show spinner for scroll-triggered media lazy-load

[kadamwhite]

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.

[nacin]

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.

[gcorne]

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

Google Chrome also is doing some interesting ​stuff in this area.

With the race condition fixed, I am not sure if there is anything we need to do in terms of controlling when the spinner is hidden. @helen, do you agree?

[kadamwhite]

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

[kadamwhite]

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.

[gcorne]

Replying to kadamwhite:

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

Yikes, at least I was halfway there. ;) Fixed in 24859.14.diff​.

[gcorne]

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.

[helen]

400ms ended up feeling better than 300 in practice, so it doesn't show up too soon when you first open the modal, but latest patch seems like a good improvement otherwise.

[helen]

In 27516:

Smooth out some display and race condition issues with the media modal loading spinner. props kadamwhite, gcorne. see #24859.

[kovshenin]

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.

[helen]

In 27621:

Ensure the media library spinner hides when displaying a cached result set. props kovshenin. see #24859.

[helen]

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.

Anything else?

[helen]

Guess there's nothing else for this ticket. New ones for new issues. Thanks for your first report, jeffr0 :)

[jeffr0]

Thank you Helen and to everyone else who worked hard to fix this problem.

[nacin]

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.

[kovshenin]

An idea in 24859.16.diff​: Only show the spinner if we're close to the bottom when scrolling.

[helen]

In 28019:

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.

props kovshenin. fixes #24859.

[nacin]

With [28019] I was getting undefined toolbar for this.views.parent.toolbar on line 4891 (trunk is 4906). This occurred when inserting an audio playlist I *think* but I don't have steps to reproduce.

[ocean90]

Replying to nacin:

I don't have steps to reproduce.

• Open Modal
• Wait if first page is loaded
• Enter in search foo
• Wait for result
• Remove one o from foo
• Wait
• Enter the o again

[kovshenin]

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

[ocean90]

In 28046:

Move toolbar declaration after the :visible check, like it was before [28019].

props kovshenin.
fixes #24859.