WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 16 months ago

#24859 closed task (blessed) (fixed)

Media Library does not have a loading indicator

Reported by: jeffr0 Owned by: nacin
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: has-patch
Focuses: javascript Cc:

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.

Attachments (18)

Screen Shot 2013-07-28 at 5.36.55 PM.png (49.4 KB) - added by jeffr0 2 years ago.
This is what it looks like when searching for media files
24859.diff (3.2 KB) - added by kovshenin 23 months ago.
24859.2.diff (3.1 KB) - added by kadamwhite 22 months ago.
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
24859.3.diff (2.0 KB) - added by gcorne 17 months ago.
24859.4.diff (2.4 KB) - added by kovshenin 17 months ago.
24859.5.diff (2.4 KB) - added by gcorne 17 months ago.
24859.6.diff (2.4 KB) - added by gcorne 17 months ago.
24859.7.diff (2.2 KB) - added by kadamwhite 17 months ago.
Made it work in my environments, removed code that didn't seem to be needed, and style tweaks
24859.8.diff (2.2 KB) - added by kadamwhite 17 months ago.
moved setting the initial spinner to false back to where gcorne had it
24859.9.diff (2.1 KB) - added by kadamwhite 17 months ago.
make gcorne's final css adjustment to avoid having to manually suppress the spinner
24859.10.diff (464 bytes) - added by kadamwhite 17 months ago.
Drop delay to 300ms
24859.11.diff (1.3 KB) - added by kadamwhite 17 months ago.
Move timeout logic inside Spinner view
24859.12.diff (1.9 KB) - added by kadamwhite 17 months ago.
Show spinner for scroll-triggered media lazy-load
24859.13.diff (2.1 KB) - added by gcorne 17 months ago.
24859.14.diff (2.1 KB) - added by gcorne 17 months ago.
24859.15.diff (370 bytes) - added by kovshenin 17 months ago.
24859.16.diff (1.1 KB) - added by kovshenin 16 months ago.
24859.17.diff (727 bytes) - added by kovshenin 16 months ago.

Download all attachments as: .zip

Change History (74)

@jeffr02 years ago

This is what it looks like when searching for media files

comment:1 follow-up: @helen2 years ago

So like a spinner?

comment:2 in reply to: ↑ 1 @jeffr02 years ago

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

Last edited 2 years ago by jeffr0 (previous) (diff)

comment:3 @DrewAPicture2 years ago

  • Cc xoodrew@… added

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

comment:4 @helen2 years ago

  • Summary changed from No Placeholder Image When Searching Media In Posts to Media Library does not have a loading indicator

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

comment:5 @DrewAPicture2 years ago

  • Milestone changed from Awaiting Review to 3.7

Maybe we can add a spinner in 3.7.

@kovshenin23 months ago

comment:6 @kovshenin23 months ago

  • Cc kovshenin added
  • Keywords has-patch added; needs-patch removed

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

comment:7 @helen23 months ago

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

comment:8 @kadamwhite23 months ago

  • Cc kadamwhite@… added

comment:9 @kadamwhite22 months ago

+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 ); 
Last edited 22 months ago by kadamwhite (previous) (diff)

comment:10 follow-up: @kadamwhite22 months ago

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.

comment:11 in reply to: ↑ 10 @helen22 months ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 3.7 to Future Release

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

comment:12 @kadamwhite22 months ago

Replying to helen:

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

I will endeavor to do so :)

Last edited 22 months ago by kadamwhite (previous) (diff)

@kadamwhite22 months ago

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

comment:13 @kadamwhite21 months ago

Bumping this now that we're into 3.8

comment:14 @jeffr019 months ago

  • Cc jeffc@… added

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

comment:15 @nacin18 months ago

  • Milestone changed from Future Release to 3.9

kadamwhite, still happy with 24859.2.diff?

comment:16 @helen18 months ago

  • Focuses javascript added
  • Keywords has-patch needs-refresh added; needs-patch removed

comment:17 @kadamwhite17 months ago

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

comment:18 @helen17 months ago

Still refreshing, kadamwhite?

comment:19 @kovshenin17 months ago

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.

comment:20 @kadamwhite17 months ago

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.

@gcorne17 months ago

comment:21 @gcorne17 months ago

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.

@kovshenin17 months ago

comment:22 @kovshenin17 months ago

  • Keywords needs-refresh removed

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.

@gcorne17 months ago

comment:23 @gcorne17 months ago

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.

@gcorne17 months ago

comment:24 @gcorne17 months ago

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

@kadamwhite17 months ago

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

comment:25 @kadamwhite17 months ago

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

@kadamwhite17 months ago

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

comment:26 @kadamwhite17 months ago

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.

@kadamwhite17 months ago

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

comment:27 @gcorne17 months ago

  • Keywords commit added

I think this is ready to go.

comment:28 @ircbot17 months ago

This ticket was mentioned in IRC in #wordpress-dev by wonderboymusic. View the logs.

comment:29 @wonderboymusic17 months ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 27438:

Add a loading indicator to the Media Library.

Props kadamwhite, gcorne, kovshenin.
Fixes #24859.

comment:30 @helen17 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:31 @ircbot17 months ago

This ticket was mentioned in IRC in #wordpress-dev by kadamwhite. View the logs.

@kadamwhite17 months ago

Drop delay to 300ms

comment:32 @kadamwhite17 months ago

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

@kadamwhite17 months ago

Move timeout logic inside Spinner view

@kadamwhite17 months ago

Show spinner for scroll-triggered media lazy-load

comment:33 @kadamwhite17 months ago

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.

comment:34 @nacin17 months ago

  • Type changed from enhancement to task (blessed)

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.

@gcorne17 months ago

comment:35 @gcorne17 months ago

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?

comment:36 follow-up: @kadamwhite17 months ago

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

comment:37 @kadamwhite17 months ago

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.

@gcorne17 months ago

comment:38 in reply to: ↑ 36 @gcorne17 months ago

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.

comment:39 @gcorne17 months ago

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.

comment:40 @ircbot17 months ago

This ticket was mentioned in IRC in #wordpress-ui by helen. View the logs.

comment:41 @helen17 months ago

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.

comment:42 @helen17 months ago

In 27516:

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

@kovshenin17 months ago

comment:43 @kovshenin17 months ago

  • Keywords commit removed

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.

comment:44 @helen17 months ago

In 27621:

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

comment:45 @helen17 months ago

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?

comment:46 @helen16 months ago

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

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

comment:47 @jeffr016 months ago

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

comment:48 @nacin16 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

@kovshenin16 months ago

comment:49 @kovshenin16 months ago

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

comment:50 @nacin16 months ago

  • Owner changed from wonderboymusic to nacin
  • Status changed from reopened to reviewing

comment:51 @ircbot16 months ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.

comment:52 @helen16 months ago

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

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.

comment:53 follow-up: @nacin16 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

@kovshenin16 months ago

comment:54 in reply to: ↑ 53 @ocean9016 months ago

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

comment:55 @kovshenin16 months ago

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.

comment:56 @ocean9016 months ago

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

In 28046:

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

props kovshenin.
fixes #24859.

Note: See TracTickets for help on using tickets.