WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 4 months ago

Last modified 4 months ago

#22839 closed defect (bug) (fixed)

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

Reported by: cdog Owned by: 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 3 years ago.
22839.on-page-load.jpg (149.0 KB) - added by cdog 3 years ago.
22839.after-show.jpg (155.1 KB) - added by cdog 3 years ago.
22839.2.diff (16.6 KB) - added by cdog 3 years ago.
22839.3.diff (16.8 KB) - added by cdog 2 years ago.
22839.4.diff (10.1 KB) - added by MikeHansenMe 6 months ago.
Refreshed and uses is-active class as Helen suggested
22839.5.diff (12.9 KB) - added by valendesigns 4 months ago.

Download all attachments as: .zip

Change History (52)

@cdog3 years ago

comment:1 @cdog3 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-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.

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

comment:2 follow-up: @nacin3 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?

comment:3 in reply to: ↑ 2 @cdog3 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.

comment:4 follow-up: @nacin3 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?

comment:5 in reply to: ↑ 4 @cdog3 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 3 years ago by cdog (previous) (diff)

comment:6 follow-up: @helenyhou3 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.

comment:7 in reply to: ↑ 6 ; follow-up: @cdog3 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?

comment:8 @nacin3 years ago

  • Milestone changed from 3.5 to 3.5.1

Yes, helenyhou also uses Firefox.

comment:9 @helenyhou3 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

comment:10 in reply to: ↑ 7 ; follow-up: @helenyhou3 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.

comment:11 @nacin3 years ago

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

@cdog3 years ago

@cdog3 years ago

comment:12 in reply to: ↑ 10 @cdog3 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.

comment:13 follow-up: @helenyhou3 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.

comment:14 in reply to: ↑ 13 @cdog3 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 3 years ago by cdog (previous) (diff)

comment:15 follow-up: @helenyhou3 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.

comment:16 in reply to: ↑ 15 @azaozz3 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 3 years ago by azaozz (previous) (diff)

comment:17 @helenyhou3 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.

comment:18 @azaozz3 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 3 years ago by azaozz (previous) (diff)

comment:19 @azaozz3 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.

@cdog3 years ago

comment:21 @cdog3 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 3 years ago by cdog (previous) (diff)

comment:23 @SergeyBiryukov3 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()

comment:24 @SergeyBiryukov3 years ago

  • Milestone changed from Awaiting Review to 3.6

comment:25 @helen3 years ago

  • Keywords ui-focus added

comment:26 @ocean902 years ago

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

comment:27 @wonderboymusic2 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

@cdog2 years ago

comment:28 @cdog2 years ago

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

comment:29 @helen22 months 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?)

comment:30 @helen6 months 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.

@MikeHansenMe6 months ago

Refreshed and uses is-active class as Helen suggested

comment:31 @MikeHansenMe6 months ago

  • Keywords has-patch added; needs-patch removed

comment:32 @johnbillion5 months ago

  • Keywords 3.7-early removed

comment:33 @slackbot4 months ago

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

comment:34 @DrewAPicture4 months ago

  • Keywords needs-refresh added

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

@valendesigns4 months ago

comment:35 @valendesigns4 months 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.

comment:36 @samuelsidler4 months ago

  • Priority changed from normal to low

comment:37 @slackbot4 months ago

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

comment:38 @helen4 months 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.

comment:39 @helen4 months 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.

comment:40 @westonruter4 months ago

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

comment:41 @slackbot4 months ago

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

comment:42 @jadpm4 months 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 :-)

comment:43 @helen4 months 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.

comment:44 @azaozz4 months 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.

comment:45 @helen4 months 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.