Toggle spinners by adding/removing a class instead of show()/hide()

[cdog]

I've noticed a bug with the spinner while updating a previous submission (ticket:19159). It worked in 3.4 when .ajax-feedback was used instead of .spinner.

Elements with class .spinner don't show up in any .sidebar-name (e.g. Main Sidebar, Inactive Widgets etc.) when an action is in progress (e.g. clearing inactive widgets).

​.show() restores the display property to whatever it was initially. Because the spinner is a span element, that value will be inline which causes the width and height properties to be ignored conform to the ​CSS Level 1 specifications.

The spinner is displayed in .widget-title because the float property is used which establishes a new ​block formatting context, while in .sidebar-name float is set to none.

One solution would be to use ​.css() instead of ​.show() to set the display property to inline-block. Or to make the element to establish a new BFC which requires less changes in code.

Seems like this will affect any spinners that are set to float: none. Looking through wp-admin.css, I see a few others.

I believe this is styled so <div class="spinner"></div> could work just the same, yes?

Replying to nacin:

Seems like this will affect any spinners that are set to float: none. Looking through wp-admin.css, I see a few others.

I believe this is styled so <div class="spinner"></div> could work just the same, yes?

Changing it to div will cause ​.show() to restore the initial value to block, not to inline-block. I think this would pull the spinner below the sidebar title.

Other affected ones include: .find-box-search .spinner, .options-general-php .spinner, .press-this #publishing-actions .spinner, .sidebar-name .spinner, and .update-php .spinner, in wp-admin.css alone. But I doubt all of those are broken.

.show() and .hide() are clever but we should probably pivot to adding/removing a class in the future, as that will also be helpful.

Finally, what browser?

Replying to nacin:

Finally, what browser?

I'm testing in Firefox 16.0.2 on Ubuntu.

In 3.4, instead of .show() and .hide(), was used .css() to set the visibility not the display property.

The spinner does appear for me when rearranging within a sidebar (note that those are also .sidebar-name), including Inactive Widgets. As azaozz noted in #20957:comment:4, is the only time that it appeared, at least in 3.4. I just gave it a run in 3.4.2 and can't get that particular spinner to appear except in the rearrangement scenario.

Replying to helenyhou:

The spinner does appear for me when rearranging within a sidebar (note that those are also .sidebar-name), including Inactive Widgets.

I'm on the Widgets page and I can't get it work.

Opened up the console and tried:

jQuery( '.spinner', jQuery( '.inactive-sidebar' ) ).show();

jQuery( '.spinner', jQuery( '.sidebar-name ) ).show();

Seems to have no effect but inspecting it with Firebug shows that display inline was set. But the element doesn't show up until inline-block is used (or position: absolute).

It may happen only in Firefox?

Yes, helenyhou also uses Firefox.

Tested the spinners in trunk:

From nacin's list:

• .find-box-search .spinner - works
• .options-general-php .spinner - works (changing date format)
• .press-this #publishing-actions .spinner - works
• .sidebar-name .spinner - works (rearranging)
• .update-php .spinner - works

Others that I found:

• Customizer save - works
• List table quick edit - works (posts, pages, taxonomies)
• Themes list table infinite scroll - works
• QuickPress - works
• Publish metabox - works (save draft)
• Comments metabox on edit screen - works
• Nav menus: theme locations, add to menu, search items - works
• Link dialog search - works
• Fullscreen editor save - works
• New media modal attachment details save - works
• Image editor - works

Replying to cdog:

I'm on the Widgets page and I can't get it work.

As nacin noted, I am one of the notorious Firefox holdouts :) They are working just fine for me. Testing in the console is nice and all, but watching it fly by in practice shows me that inline-block is what gets set when rearranging widgets.

Replying to helenyhou:

As nacin noted, I am one of the notorious Firefox holdouts :) They are working just fine for me. Testing in the console is nice and all, but watching it fly by in practice shows me that inline-block is what gets set when rearranging widgets.

attachment:22839.on-page-load.jpg​ is what I see right after the page loads and attachment:22839.after-show.jpg​ after .show() gets called.

What version of Firefox are you using?

I'll investigate this further and report back.

Oh, I get the same result if you run .show() yourself in the console. But I'm not seeing it as broken in core. I'm on 17.0.1 on OSX.

Replying to helenyhou:

Oh, I get the same result if you run .show() yourself in the console. But I'm not seeing it as broken in core. I'm on 17.0.1 on OSX.

You are right. It's my fault. Should be more carefully next time.

saveOrder does use:

$('#' + sb).closest('div.widgets-holder-wrap').find('.spinner').css('display', 'inline-block');

and save:

$('.spinner', widget).show();

and works just well.

Got in error by myself while switching from .ajax-feedback to .spinner. It's confusing because of the two different methods used to show the spinner (depending on float or not). Who knows where my mind was.

Still, applying the patch what I've suggested in my first comment would make possible to use:

$('#' + sb).closest('div.widgets-holder-wrap').find('.spinner').show();

instead of:

$('#' + sb).closest('div.widgets-holder-wrap').find('.spinner').css('display', 'inline-block');

making the code more clearer.

Thanks for taking your time.

Will have to test to see what side effects the patch might have. Doesn't seem right to just suddenly make it positioned absolutely, and anything we do in core in the future really should be about the spinner in general, not just in one place. Reusable, and all that, which I think is what you're looking for here.

Replying to helenyhou:

+1, position: absolute will have side effects in many other places, in plugins, and in the future when adding spinners. Seems better to standardize on using visibility: visible/hidden as that won't mess with positioning, blocks, floats, etc.

That, or we could add/remove a class that works the way .screen-reader-text does. I think we had some problems with using visibility because it reserves the space.

Yeah, adding/removing a class would be best. Yes, setting visibility to hidden reserves the place but that is usually good as it ensures the elements/text around the spinner won't "jump" when it's displayed.

Thinking more about this: there is an accessibility aspect here too. Ideally screen readers should announce that something is happening on the page, so we may need to toggle some aria attributes there. Lets look at spinners again in 3.6.

Related: [22019]

Currently, spinners are toggled via .show()/.hide() jQuery methods. But this doesn't seem to work in all cases creating some inconsistencies (mainly when floats are used).

attachment:22839.2.diff​ introduces the .active class which toggles a spinner when added/removed. The main advantage is that it adds a standard way of toggling a spinner. Another advatage of adding/romoving a class is that it allows more than one property to get changed at once (not only display) if it's needed.

In Quick Edit mode, the spinner seems to have an unnecessary offset of 10px (removed in patch).

Also found that the #major-publishing-actions .spinner never shows up when editing an attachment (with or without the patch applied) like it does when editing a post.

Steps to reproduce:

1. In Dashboard, go to Media > Libary.
2. Select an image.
3. Press Update.

This particular spinner (from #submitpost) is toggled only in wp-includes/js/autosave.js which is not loaded when editing an attachment.

Related: #23120