WordPress.org

Make WordPress Core

Opened 9 months ago

Closed 8 days 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 9 months ago.
This is what it looks like when searching for media files
24859.diff (3.2 KB) - added by kovshenin 7 months ago.
24859.2.diff (3.1 KB) - added by kadamwhite 7 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 6 weeks ago.
24859.4.diff (2.4 KB) - added by kovshenin 6 weeks ago.
24859.5.diff (2.4 KB) - added by gcorne 6 weeks ago.
24859.6.diff (2.4 KB) - added by gcorne 6 weeks ago.
24859.7.diff (2.2 KB) - added by kadamwhite 6 weeks 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 6 weeks ago.
moved setting the initial spinner to false back to where gcorne had it
24859.9.diff (2.1 KB) - added by kadamwhite 6 weeks ago.
make gcorne's final css adjustment to avoid having to manually suppress the spinner
24859.10.diff (464 bytes) - added by kadamwhite 6 weeks ago.
Drop delay to 300ms
24859.11.diff (1.3 KB) - added by kadamwhite 6 weeks ago.
Move timeout logic inside Spinner view
24859.12.diff (1.9 KB) - added by kadamwhite 6 weeks ago.
Show spinner for scroll-triggered media lazy-load
24859.13.diff (2.1 KB) - added by gcorne 5 weeks ago.
24859.14.diff (2.1 KB) - added by gcorne 5 weeks ago.
24859.15.diff (370 bytes) - added by kovshenin 4 weeks ago.
24859.16.diff (1.1 KB) - added by kovshenin 2 weeks ago.
24859.17.diff (727 bytes) - added by kovshenin 8 days ago.

Download all attachments as: .zip

Change History (74)

jeffr09 months ago

This is what it looks like when searching for media files

comment:1 follow-up: helen9 months ago

So like a spinner?

comment:2 in reply to: ↑ 1 jeffr09 months ago

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

Last edited 9 months ago by jeffr0 (previous) (diff)

comment:3 DrewAPicture9 months ago

  • Cc xoodrew@… added

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

comment:4 helen8 months 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 DrewAPicture8 months ago

  • Milestone changed from Awaiting Review to 3.7

Maybe we can add a spinner in 3.7.

kovshenin7 months ago

comment:6 kovshenin7 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 helen7 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 kadamwhite7 months ago

  • Cc kadamwhite@… added

comment:9 kadamwhite7 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 7 months ago by kadamwhite (previous) (diff)

comment:10 follow-up: kadamwhite7 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 helen7 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 kadamwhite7 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 7 months ago by kadamwhite (previous) (diff)

kadamwhite7 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 kadamwhite6 months ago

Bumping this now that we're into 3.8

comment:14 jeffr03 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 nacin2 months ago

  • Milestone changed from Future Release to 3.9

kadamwhite, still happy with 24859.2.diff?

comment:16 helen2 months ago

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

comment:17 kadamwhite7 weeks ago

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

comment:18 helen6 weeks ago

Still refreshing, kadamwhite?

comment:19 kovshenin6 weeks 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 kadamwhite6 weeks 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.

gcorne6 weeks ago

comment:21 gcorne6 weeks 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.

kovshenin6 weeks ago

comment:22 kovshenin6 weeks 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.

gcorne6 weeks ago

comment:23 gcorne6 weeks 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.

gcorne6 weeks ago

comment:24 gcorne6 weeks ago

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

kadamwhite6 weeks ago

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

comment:25 kadamwhite6 weeks 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. :)

kadamwhite6 weeks ago

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

comment:26 kadamwhite6 weeks 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.

kadamwhite6 weeks ago

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

comment:27 gcorne6 weeks ago

  • Keywords commit added

I think this is ready to go.

comment:28 ircbot6 weeks ago

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

comment:29 wonderboymusic6 weeks 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 helen6 weeks 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 ircbot6 weeks ago

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

kadamwhite6 weeks ago

Drop delay to 300ms

comment:32 kadamwhite6 weeks 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).

kadamwhite6 weeks ago

Move timeout logic inside Spinner view

kadamwhite6 weeks ago

Show spinner for scroll-triggered media lazy-load

comment:33 kadamwhite6 weeks 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 nacin5 weeks 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.

gcorne5 weeks ago

comment:35 gcorne5 weeks 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: kadamwhite5 weeks 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 kadamwhite5 weeks 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.

gcorne5 weeks ago

comment:38 in reply to: ↑ 36 gcorne5 weeks 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 gcorne5 weeks 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 ircbot5 weeks ago

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

comment:41 helen5 weeks 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 helen5 weeks ago

In 27516:

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

kovshenin4 weeks ago

comment:43 kovshenin4 weeks 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 helen4 weeks ago

In 27621:

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

comment:45 helen4 weeks 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 helen3 weeks 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 jeffr03 weeks ago

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

comment:48 nacin3 weeks 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.

kovshenin2 weeks ago

comment:49 kovshenin2 weeks ago

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

comment:50 nacin10 days ago

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

comment:51 ircbot9 days ago

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

comment:52 helen9 days 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: nacin9 days 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.

kovshenin8 days ago

comment:54 in reply to: ↑ 53 ocean908 days 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 kovshenin8 days 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 ocean908 days 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.