Make WordPress Core

Opened 12 years ago

Closed 11 years ago

Last modified 7 years ago

#21456 closed task (blessed) (fixed)

Replace animated spinner gifs

Reported by: nacin's profile nacin Owned by: lessbloat's profile lessbloat
Milestone: 3.5 Priority: normal
Severity: normal Version:
Component: UI Keywords: has-patch deprecated-css
Focuses: Cc:

Description

As discussed in the IRC dev chat on Aug 1, we should replace our existing animated spinners with spin.js: http://fgnass.github.com/spin.js/

This has some advantages, including HiDPI support, a fresher design, color scheme support, and an all JS/CSS implementation.

Attachments (15)

21456.diff (22.6 KB) - added by lessbloat 12 years ago.
wpspin_light.gif (17.0 KB) - added by lessbloat 12 years ago.
wpspin_light.psd.zip (142.5 KB) - added by lessbloat 12 years ago.
IMG_0034.PNG (1.5 KB) - added by azaozz 12 years ago.
wpspin_light.png (789 bytes) - added by alternatekev 12 years ago.
png replacement for spinner gif, low-res
wpspin_light-2x.png (1.7 KB) - added by alternatekev 12 years ago.
png replacement for spinner gif, hi-res
hidpi-spinner.diff (19.2 KB) - added by alternatekev 12 years ago.
patch for gif spinner replacement
21456.2.diff (22.7 KB) - added by lessbloat 12 years ago.
wpspin_light.2.gif (8.9 KB) - added by lessbloat 12 years ago.
Goes with 21456.2.diff
21456.3.diff (35.4 KB) - added by lessbloat 12 years ago.
21456.4.diff (39.8 KB) - added by lessbloat 12 years ago.
21456.5.diff (42.1 KB) - added by lessbloat 12 years ago.
21456.6.diff (42.8 KB) - added by lessbloat 12 years ago.
wpspin_light-2x.gif (8.9 KB) - added by lessbloat 12 years ago.
21456.7.diff (2.5 KB) - added by lessbloat 11 years ago.

Download all attachments as: .zip

Change History (47)

#1 @johnbillion
12 years ago

Interesting to note that wp.com uses spin.js already (note sure where exactly). When spin.js came up on Hacker News there were a few posts noting that its CPU usage was pretty high.

#2 @azaozz
12 years ago

I'm having some second thoughts on it too. Quite CPU intensive, on slower PCs (tablets, smart phones) interferes with typing in TinyMCE (also a little bit with typing in any text field).

The fact that it uses css only is very attractive. Perhaps we can look into using some simpler shapes, 3 pulsating squares, or circles, or... Something that can be animated by using css transitions directly and doesn't need the trails.

The "old way" of doing something similar is to have separate transparent/semi-transparent png frames. It scales down well and looks really good, but needs 10-12 small images.

#3 @rfair404
12 years ago

  • Cc rfair404 added

#4 @lessbloat
12 years ago

Brought this up in the UI chat. We all agreed that we'd rather stick with a no-js solution for the time being. @joen already came up with a 2x version of the spinner:

http://core.trac.wordpress.org/attachment/ticket/21019/wpspin_light-2x.gif

Last edited 12 years ago by SergeyBiryukov (previous) (diff)

#5 follow-up: @lessbloat
12 years ago

I thought I'd check out the CPU usage myself (to see if perhaps it had been improved). Turns out, it has not.

http://neteye.github.com/activity-indicator.html appears to be somewhat less CPU intensive. I tested them both (on FF 14.0.1 - Mac 10.7.4):

Spin.js - Single stance consumes between 40-48% CPU, constantly while it's on screen

activity-indicator - 12-15% CPU constantly

#6 in reply to: ↑ 5 @Viper007Bond
12 years ago

Replying to lessbloat:

Spin.js - Single stance consumes between 40-48% CPU, constantly while it's on screen

activity-indicator - 12-15% CPU constantly

I don't get such drastic results (faster CPU probably) but I do see activity-indicator being better. 6-7% rather than 24-25%, using Firefox.

Last edited 12 years ago by Viper007Bond (previous) (diff)

@lessbloat
12 years ago

#7 @lessbloat
12 years ago

21456.diff adds one single retina-friendly animated gif spinner to rule them all (1x and 2x). Comes in at 17kb, so I didn't see any point in creating a low-res version. Props to @joen for the PSD.

Last edited 12 years ago by lessbloat (previous) (diff)

#8 @lessbloat
12 years ago

Something like this also works allowing us to use a png instead of a gif (to avoid the gif artifacts on different colored backgrounds):

<html>
	<head>
		<title>Spin me</title>
		<style type="text/css">
			.spin {
				background: url('spinner.png') no-repeat;
				width: 20px;
				height: 20px;
				-webkit-animation: spin 0.7s infinite linear;
				-moz-animation: spin 0.7s infinite linear;
				-o-animation: spin 0.7s infinite linear;
				-ms-animation: spin 0.7s infinite linear;
			}
			@media only screen and (-moz-min-device-pixel-ratio: 1.5), only screen and (-o-min-device-pixel-ratio: 3/2), only screen and (-webkit-min-device-pixel-ratio: 1.5), only screen and (min-device-pixel-ratio: 1.5) {
				.spin {
					background-image: url(spinner-2x.png);
					background-size: 20px 20px;
				}
			}
			@-webkit-keyframes spin {
				0% { -webkit-transform: rotate(0deg);}
				100% { -webkit-transform: rotate(360deg);}
			}
			@-moz-keyframes spin {
				0% { -moz-transform: rotate(0deg);}
				100% { -moz-transform: rotate(360deg);}
			}
			@-o-keyframes spin {
				0% { -o-transform: rotate(0deg);}
				100% { -o-transform: rotate(360deg);}
			}
			@-ms-keyframes spin {
				0% { -ms-transform: rotate(0deg);}
				100% { -ms-transform: rotate(360deg);}
			}
			@keyframes spin {
				0% { transform: rotate(0deg);}
				100% { transform: rotate(360deg);}
			}
		</style>
	</head>
	<body>
		<div class="spin"></div>
	</body>
</html>

However, when I compare the performance of this block of code against http://neteye.github.com/activity-indicator.html, they are about the same. At this point, I'm leaning towards switching the current gif to use http://neteye.github.com/activity-indicator.html.

Unless of course, we come up with anything better.

#9 @goto10
12 years ago

  • Cc dromsey@… added

At this point, I'm leaning towards switching the current gif to use http://neteye.github.com/activity-indicator.html.

Just wanted to mention that there is an issue with Net Eye Activity Indicator on webkit browsers when used with stylesheets that have @rules.

This is kind of a big deal, since child themes will inherently be using the @import rule to grab their parent theme's stylesheet.

Good news, there is a solution (see comment by becw): https://github.com/neteye/jquery-plugins/pull/47

It's kind of a bummer that this fix hasn't been integrated officially though.

#10 @lessbloat
12 years ago

Here's the trimmed down base code I was going to propose using (if we go with the neteye solution):

<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<script type="text/javascript" src="http://ajax.googleapis.com/ajax/libs/jquery/1.4.2/jquery.js"></script>
<script src="https://raw.github.com/gist/3061879/8e9d91998f796d452c7b3ef0c122cff8f4392124/jquery.activity-indicator-1.0.0.min.js" ></script>
</head>
<body>
<div id="busy3"></div>
<script>
	$(function() {
		$('#busy3').activity({segments: 7, width:2, space: 0, length: 3, speed: 1.5});
	});
</script>
</body>
</html>

Which runs at between 6% and 11% CPU on my laptop.

Last edited 12 years ago by lessbloat (previous) (diff)

@azaozz
12 years ago

#11 @azaozz
12 years ago

Tested this too. On the desktop PC it takes 2-3% CPU in both Chrome and Firefox. However on iPad 1 with iOS 5.1.1 it seems to be much higher. Also when zooming in noticed some irregularities with the bars.


#12 @koopersmith
12 years ago

As discussed in IRC, I think the best option for moving forward is using a PNG rotated using CSS3 animations, as outlined in lessbloat's earlier comment. We can override the background-image to use a GIF in browsers that don't support CSS3 animations.

Benefits of using a rotating PNG:

  • More granular control over transparency.
  • We don't have to matte the spinner based on the background, which makes the spinner far more flexible.
  • Low CPU usage.
  • Rendering bugs are less likely to occur.

#13 @lessbloat
12 years ago

  • Keywords needs-patch added

@alternatekev
12 years ago

png replacement for spinner gif, low-res

@alternatekev
12 years ago

png replacement for spinner gif, hi-res

@alternatekev
12 years ago

patch for gif spinner replacement

#14 @helenyhou
12 years ago

  • Keywords has-patch added; needs-patch removed

#15 @nacin
12 years ago

  • Owner set to lessbloat
  • Status changed from new to assigned
  • Summary changed from Replace animated spinner gifs with spin.js to Replace animated spinner gifs
  • Type changed from enhancement to task (blessed)

I believe this is being reviewed by lessbloat.

#16 @lessbloat
12 years ago

I tested hidpi-spinner.diff. Two things:

  • For whatever reason, when using CSS animation with the transparent PNG, the icon is somewhat lopsided, and it suffers from intermittent delays (tested in FF 14.0.1). I played with the PNG trying to get it to not look lopsided, without any luck.
  • Additionally, I tested CPU on the dashboard without the spinner showing (11%-12%) CPU, and then I dropped the slider code in via the console, and the CPU jumped to (38%-40%) which still seems really high to me for a small spinner.

For these reasons, my proposal would be to stick with 21456.diff, which isn't perfect, but overall seems to offer a better solution than using CSS animation with the PNG (at this point).

@lessbloat
12 years ago

@lessbloat
12 years ago

Goes with 21456.2.diff

#17 @lessbloat
12 years ago

Per UI chat, tried out spinner at 32px, and it appears to work:

http://f.cl.ly/items/1230093i0R1M0g0V0f1u/32px-animated.gif

Two Notes:

  • 21456.2.diff also fixes a conflict in 21456.diff
  • wpspin_light.2.gif should be named wpspin_light.gif, was renamed at upload because wpspin_light.gif was uploaded previously (at a 64px res).

#18 follow-up: @nacin
12 years ago

I suggested to lessbloat that it is probably time to move to a span with an image as a background, for both forwards and backwards compatibility reasons. It will be nice to have less insane markup to implement a spinner.

@lessbloat
12 years ago

#19 in reply to: ↑ 18 @lessbloat
12 years ago

Replying to nacin:

I suggested to lessbloat that it is probably time to move to a span with an image as a background, for both forwards and backwards compatibility reasons. It will be nice to have less insane markup to implement a spinner.

Replaced all <img> spinners with <span class="spinner"> in 21456.3.diff

#20 follow-up: @nacin
12 years ago

The reason why visibility: hidden is often used instead of display: none is because then the spinner takes up space. When toggling the display property, we've often seen other objects move around, which can be jarring and certainly not expected behavior. So it is definitely safer to use visibility.

I'd like to do either a 'visible' class or an 'active' class. Then show() and hide() can be add/toggle/removeClass. Better then css('visibility', value), that's for sure.

Looking at 21456.3.diff, I'm seeing a lot of brokenness. Not your fault — we basically rolled our own spinner code for quite a long time. This is unfortunately way more than just a find-and-replace operation. Also, the spinners on options-general.php aren't unused.

I suggest we introduce .spinner, then start to use it wherever we can. Each instance will need to be confirmed (which is not fun or easy) as being not broken style-wise. A decent amount of CSS classes (.waiting, .ajax-loading) can probably vanish, and some JS could probably be cleaned up.

Also, the nice thing about .spinner is it also allows us to direct a media query at wpspin_light.gif for the 2x spinner.

@lessbloat: Perhaps we can get someone else to pick this up so we don't get sucked into this much further.

@lessbloat
12 years ago

#21 in reply to: ↑ 20 ; follow-up: @lessbloat
12 years ago

Couple improvements in 21456.4.diff:

Replying to nacin:

The reason why visibility: hidden is often used instead of display: none is because then the spinner takes up space. When toggling the display property, we've often seen other objects move around, which can be jarring and certainly not expected behavior. So it is definitely safer to use visibility.

With the new spinners using float, this should no longer be necessary. They should all work with display: none. Please let me know if you see some place where there is any movement, and I'll fix it.

I'd like to do either a 'visible' class or an 'active' class. Then show() and hide() can be add/toggle/removeClass. Better then css('visibility', value), that's for sure.

This still needs to be done.

Looking at 21456.3.diff, I'm seeing a lot of brokenness. Not your fault — we basically rolled our own spinner code for quite a long time. This is unfortunately way more than just a find-and-replace operation. Also, the spinners on options-general.php aren't unused.

I went through each spinner one by one and tested them in several browsers. I made a spreadsheet with the status of each: https://docs.google.com/spreadsheet/ccc?key=0Av7nMZXUOf52dExvVXBKZE8tbHFJWVRBck9kQkJxd3c#gid=0 There are still a couple that I need some help figuring out. Again, please also report any additional bugs that you spot, and I'll fix them.

I suggest we introduce .spinner, then start to use it wherever we can. Each instance will need to be confirmed (which is not fun or easy) as being not broken style-wise. A decent amount of CSS classes (.waiting, .ajax-loading) can probably vanish, and some JS could probably be cleaned up.

Done. Still need to do this for the spinners in the spreadsheet that I've been unable to test.

Last edited 12 years ago by lessbloat (previous) (diff)

#22 in reply to: ↑ 21 ; follow-up: @SergeyBiryukov
12 years ago

Replying to lessbloat:

There are still a couple that I need some help figuring out.

  • class-wp-themes-list-table.php:
    On Install Themes screen, select any feature in the filter, click "Find Themes". Scroll to the bottom. The spinner will appear in the bottom right corner while the themes are loading.
  • class-wp-plugin-install-list-table.php and class-wp-theme-install-list-table.php:
    These look like remnants of AJAX in list tables (#14579, #16262).
  • class-wp-editor.php (.river-waiting):
    Open "Insert/edit link" dialog, search for something that would return many posts. Scroll to the bottom. The spinner will appear at the bottom while the posts are loading.

@lessbloat
12 years ago

#23 in reply to: ↑ 22 @lessbloat
12 years ago

Replying to SergeyBiryukov: Thanks Sergey! That was super helpful.

21456.5.diff should cover the rest of the remaining spinners.

@lessbloat
12 years ago

#24 @lessbloat
12 years ago

21456.6.diff adds media query for wpspin_light-2x.gif for backwards compatibility.

#25 @nacin
12 years ago

In [22019]:

New HiDPI spinner. Uses clean <span class="spinner"></span> markup.

Be on the lookout for weirdness.
props lessbloat. see #21456.

#26 @mitchoyoshitaka
12 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

#27 @ocean90
11 years ago

Shouldn't we provide some backwards compatibility for plugins which are using the old image with the .ajax-loading CSS class to hide the spinner by default?

#28 @nacin
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Yes.

#29 @nacin
11 years ago

.ajax-feedback should get visibility: hidden back, and any remaining instances of it in HTML should be removed.

#wpcontent .ajax-loading should also return as visibility: hidden.

In both cases let's mark them with some kind of basic /* deprecated */ comment. Maybe one day (not now) we spin off a deprecated.css that we can use so when people inspect CSS, it's clear their styling comes from a deprecated file.

@lessbloat
11 years ago

#30 @lessbloat
11 years ago

Changes in 21456.7.diff:

  • Set #ajax-loading, .ajax-loading, .ajax-feedback, .imgedit-wait-spin, .list-ajax-loading to visibility: hidden;
  • Removed all instances of .ajax-loading from HTML (all instances of .ajax-feedback were already removed)
  • Fixed margin-top for theme list page spinner

#31 @nacin
11 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In [22167]:

Retina/span spinner cleanup. Restores CSS classes no longer used by core. props lessbloat. fixes #21456.

#32 @afercia
7 years ago

  • Keywords deprecated-css added
Note: See TracTickets for help on using tickets.