Make WordPress Core

Opened 11 years ago

Closed 9 years ago

Last modified 9 years ago

#22839 closed defect (bug) (fixed)

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

Reported by: cdog's profile cdog Owned by: helen's profile helen
Milestone: 4.2 Priority: low
Severity: normal Version: 3.4.2
Component: Administration Keywords: has-patch
Focuses: ui Cc:

Description

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

Attachments (7)

22839.diff (328 bytes) - added by cdog 11 years ago.
22839.on-page-load.jpg (149.0 KB) - added by cdog 11 years ago.
22839.after-show.jpg (155.1 KB) - added by cdog 11 years ago.
22839.2.diff (16.6 KB) - added by cdog 11 years ago.
22839.3.diff (16.8 KB) - added by cdog 11 years ago.
22839.4.diff (10.1 KB) - added by MikeHansenMe 9 years ago.
Refreshed and uses is-active class as Helen suggested
22839.5.diff (12.9 KB) - added by valendesigns 9 years ago.

Download all attachments as: .zip

Change History (52)

@cdog
11 years ago

#1 @cdog
11 years ago

  • Cc catalin.dogaru@… added
  • Keywords has-patch needs-testing added; needs-patch removed

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

Version 0, edited 11 years ago by cdog (next)

#2 follow-up: @nacin
11 years ago

  • Milestone changed from Awaiting Review to 3.5

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?

#3 in reply to: ↑ 2 @cdog
11 years ago

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.

#4 follow-up: @nacin
11 years ago

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?

#5 in reply to: ↑ 4 @cdog
11 years ago

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.

Last edited 11 years ago by cdog (previous) (diff)

#6 follow-up: @helenyhou
11 years ago

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.

#7 in reply to: ↑ 6 ; follow-up: @cdog
11 years ago

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?

#8 @nacin
11 years ago

  • Milestone changed from 3.5 to 3.5.1

Yes, helenyhou also uses Firefox.

#9 @helenyhou
11 years ago

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

#10 in reply to: ↑ 7 ; follow-up: @helenyhou
11 years ago

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.

#11 @nacin
11 years ago

  • Keywords reporter-feedback added
  • Milestone changed from 3.5.1 to Awaiting Review

#12 in reply to: ↑ 10 @cdog
11 years ago

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.

#13 follow-up: @helenyhou
11 years ago

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.

#14 in reply to: ↑ 13 @cdog
11 years ago

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.

Last edited 11 years ago by cdog (previous) (diff)

#15 follow-up: @helenyhou
11 years ago

  • Component changed from UI to Administration
  • Keywords reporter-feedback removed
  • Version changed from trunk to 3.4.2

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.

#16 in reply to: ↑ 15 @azaozz
11 years ago

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.

Last edited 11 years ago by azaozz (previous) (diff)

#17 @helenyhou
11 years ago

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.

#18 @azaozz
11 years ago

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.

Last edited 11 years ago by azaozz (previous) (diff)

#19 @azaozz
11 years ago

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.

@cdog
11 years ago

#21 @cdog
11 years ago

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.

Last edited 11 years ago by cdog (previous) (diff)

#23 @SergeyBiryukov
11 years ago

  • Summary changed from Spinners don't show up in .sidebar-name to Toggle spinners by adding/removing a class instead of show()/hide()

#24 @SergeyBiryukov
11 years ago

  • Milestone changed from Awaiting Review to 3.6

#25 @helen
11 years ago

  • Keywords ui-focus added

#26 @ocean90
11 years ago

  • Keywords 3.7-early added
  • Milestone changed from 3.6 to Future Release

#27 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

@cdog
11 years ago

#28 @cdog
11 years ago

attachment:22839.3.diff updates the previous patch against current trunk (revision 25008).

#29 @helen
11 years ago

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

Punting - patch magically still applies but doesn't seem to have been tested. It breaks at least the following - I only tested about half of the spinner instances listed in 9.

  • Widgets, both individual when saving a change and sidebar when moving
  • Date format custom entry on general settings
  • Nav menu item adding (this shows up a LOT in that file, considering that it only appears to display for that one action - might we have some cruft or janky JS?)

#30 @helen
9 years ago

  • Milestone changed from Future Release to 4.2

Related: #30725

It's time we fix this. Let's go with a class name like .is-active - we've still got a lot to figure out when it comes to naming standards, but I think that will be a better starting point when it comes to JS-facing selectors.

@MikeHansenMe
9 years ago

Refreshed and uses is-active class as Helen suggested

#31 @MikeHansenMe
9 years ago

  • Keywords has-patch added; needs-patch removed

#32 @johnbillion
9 years ago

  • Keywords 3.7-early removed

This ticket was mentioned in Slack in #core by drew. View the logs.


9 years ago

#34 @DrewAPicture
9 years ago

  • Keywords needs-refresh added

Patch needs a refresh. Would be nice to get this in for 4.2 if we still can.

@valendesigns
9 years ago

#35 @valendesigns
9 years ago

  • Keywords needs-refresh removed

I've refreshed the patch. There are still a few places that could be converted, but this is a good start for now.

#36 @samuelsidler
9 years ago

  • Priority changed from normal to low

This ticket was mentioned in Slack in #core by helen. View the logs.


9 years ago

#38 @helen
9 years ago

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

In 31996:

Spinners: Toggle a class instead of show/hide.

Toggling spinners also now uses visibility instead of display, so that the space is always reserved and nothing moves around unexpectedly.

props cdog, MikeHansenMe, valendesigns.
fixes #22839. see #31875, #30725.

#39 @helen
9 years ago

I did a lot of testing here, but I know there are going to be some places that redefine spinner CSS that are now broken. Let's open new tickets for those instances. #30725 will address the media library spinner, which is definitely broken right now.

#40 @westonruter
9 years ago

Just opened #31884 and patched to fix the instance in Widget Customizer.

This ticket was mentioned in Slack in #core by jadpm. View the logs.


9 years ago

#42 @jadpm
9 years ago

Please note that changing the styles for the spinners from a display:none to a visibility:hidden might (and in fact does) have consecuences in third party plugins, as I just found out in one I work on.

In our particular case, we are adding the spinner span dinamically, and jQuery-show()-ing/hide()-ing it on demand, which does not have any effect over the visibility property. Not a big deal, we plan to implement out own spinner span soon, but this change might affect other too. Worth mentioning :-)

#43 @helen
9 years ago

I am aware that this is a little bit of a visual breaking change - I'm not sure there's really a way around it. It is, as far as I see, a matter of two things: spinners now taking up space all the time (which they should have been in the first place, hence this fix), and spinners not showing until plugins/themes have been updated. I don't consider either of those to be critical visual bugs, and would rather have a better base moving forward.

#44 @azaozz
9 years ago

Absolutely agree with @helen. There may be a few "missing" spinners or few small visual glitches. However, themes and plugins will be updated and we have a much better "base" going forward.

#45 @helen
9 years ago

In 32101:

Media: Bring back spinners, now without bouncing select elements.

props afercia for the initial patch.
see #22839, #30725.

Note: See TracTickets for help on using tickets.