WordPress.org

Make WordPress Core

Opened 17 months ago

Last modified 7 months ago

#22839 new defect (bug)

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

Reported by: cdog Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.4.2
Component: Administration Keywords: 3.7-early needs-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 (5)

22839.diff (328 bytes) - added by cdog 17 months ago.
22839.on-page-load.jpg (149.0 KB) - added by cdog 17 months ago.
22839.after-show.jpg (155.1 KB) - added by cdog 17 months ago.
22839.2.diff (16.6 KB) - added by cdog 16 months ago.
22839.3.diff (16.8 KB) - added by cdog 8 months ago.

Download all attachments as: .zip

Change History (34)

cdog17 months ago

comment:1 cdog17 months 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 17 months ago by cdog (previous) (diff)

comment:2 follow-up: nacin17 months 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 cdog17 months 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: nacin17 months 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 cdog17 months 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.

Version 1, edited 17 months ago by cdog (previous) (next) (diff)

comment:6 follow-up: helenyhou17 months 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: cdog17 months 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 nacin17 months ago

  • Milestone changed from 3.5 to 3.5.1

Yes, helenyhou also uses Firefox.

comment:9 helenyhou17 months 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: helenyhou17 months 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 nacin17 months ago

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

cdog17 months ago

cdog17 months ago

comment:12 in reply to: ↑ 10 cdog17 months 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: helenyhou17 months 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 cdog17 months 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 17 months ago by cdog (previous) (diff)

comment:15 follow-up: helenyhou17 months 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 azaozz17 months 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 17 months ago by azaozz (previous) (diff)

comment:17 helenyhou17 months 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 azaozz17 months 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 17 months ago by azaozz (previous) (diff)

comment:19 azaozz17 months 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.

cdog16 months ago

comment:21 cdog16 months 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 16 months ago by cdog (previous) (diff)

comment:23 SergeyBiryukov16 months 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 SergeyBiryukov15 months ago

  • Milestone changed from Awaiting Review to 3.6

comment:25 helen15 months ago

  • Keywords ui-focus added

comment:26 ocean9011 months ago

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

comment:27 wonderboymusic9 months ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

cdog8 months ago

comment:28 cdog8 months ago

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

comment:29 helen7 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?)
Note: See TracTickets for help on using tickets.