WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#33322 closed defect (bug) (fixed)

Theme Installer: spinner animated GIF and high CPU usage

Reported by: afercia Owned by: wonderboymusic
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.2
Component: Themes Keywords: has-patch
Focuses: ui, javascript Cc:

Description

In the Theme Installer (Themes > Add New > Preview a Theme), the spinner loading indicator causes the same performance issues noticed for the Customizer spinner a while ago, see https://core.trac.wordpress.org/ticket/31196#comment:14

On modern hardware, with a dedicated GPU and hardware acceleration support, you will barely notice this but on old hardware the CPU usage can easily be over 50% and in the worst case always stuck at 100% since the spinner is always "spinning" in the background even when overlaid by the iframe.

Looks like modern browser still have issues with animated GIFs especially when some CSS properties (e.g. opacity, box-shadow) are set on the GIF container. I can reproduce high CPU usage using both Chrome and Firefox, they behave differently but the result is the same.

Regardless, when the preview iframe loads, the background spinner image should be removed as already done for the Customizer.

See also related #33311 for the Thickbox spinner.

https://cldup.com/NUKm7LaRGM.png

Attachments (3)

33322.patch (1.7 KB) - added by afercia 6 years ago.
33322.2.patch (1.7 KB) - added by adamsilverstein 6 years ago.
33322.3.patch (2.5 KB) - added by afercia 6 years ago.

Download all attachments as: .zip

Change History (16)

@afercia
6 years ago

#1 @afercia
6 years ago

  • Keywords has-patch added

First pass: the proposed patch uses a callback on the iframe load event to toggle a CSS class and remove the background spinner.

#2 @atomicjack
6 years ago

-removed-

Last edited 6 years ago by atomicjack (previous) (diff)

#3 @helen
6 years ago

@atomicjack afercia has been around quite a bit, we do check on various older systems. Calling them "unsupported" is not quite accurate, though our support may amount to all of "make sure we don't crash the browser", which is what is happening here. Philosophically, we would like to enable as many people to publish as possible, and there are times where they are not in control of their computing environment. These are often the people who are in need of a voice on the web.

I'd kindly suggest that you spend a little more time observing before making such sweeping statements to experienced contributors about best practices for this specific project.

Last edited 6 years ago by helen (previous) (diff)

#4 @afercia
6 years ago

No worries :) WordPress is still officially supporting IE 8 and as I see it it should then support also the OS and the hardware where IE 8 typically runs. When speaking about performance, it's a bit pointless testing IE 8 on a Virtual Machine running on a powerful Mac :) Would recommend also to consider that outside the US Windows XP is still largely used. See also #33311 where similar issues happens also on a MacBook Pro and the video posted there that shows the continuous repaints.

#5 @jdgrimes
6 years ago

Just to confirm, on my MacBook Pro running OS X 10.4 I experience high CPU usage from these spinners if they are displayed for long periods of time (more than a few seconds).

#6 follow-up: @adamsilverstein
6 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 4.3.2
  • Owner set to adamsilverstein
  • Status changed from new to accepted

33322.2.patch:

  • refresh against trunk

@afercia Thanks, this looks good! tested and see the class being added and removed and spinner hidden as a result when iframe loading completes. I trust this solves the issue, although I didn't test for or observe the high cpu use on my local machine.

@jdgrimes can you test the patch to see if it resolves the issue on your end?

#7 in reply to: ↑ 6 @jdgrimes
6 years ago

Replying to adamsilverstein:

@jdgrimes can you test the patch to see if it resolves the issue on your end?

Yep, that fixes it for me.

#8 @helen
6 years ago

  • Milestone changed from 4.3.2 to 4.4

This wasn't introduced in 4.3, so I'm not inclined to put it into a point release. Let's get it in, though.

#9 @helen
6 years ago

  • Keywords commit removed
  • Owner changed from adamsilverstein to wonderboymusic
  • Status changed from accepted to assigned

@wonderboymusic noted an unnecessary usage of this.$el on a quick review request from me, he's going to do a final review and commit.

#10 @afercia
6 years ago

There's one more thing to consider: the CPU usage while the spinner is being displayed. As per the testing run on #33311 applying an animated GIF as background on a so large area forces browsers to continuously repaint the whole area. For example, here's what Chrome does. The console "Enable paint flashing" option will allow you to actually see the re-painted area. There's a similar option in Firefox's console.
To minimize the high CPU usage, the spinner should be used as background of the smallest possible area (20px x 20px) as experimented on #33311 using CSS generated content.

https://cldup.com/0G29noiXD6.png

@afercia
6 years ago

#11 @afercia
6 years ago

Refreshed patch. Using the same CSS technique experimented in #33311 the re-painted area is now just 20px x 20px, see the screenshot below. The CPU usage is now noticeably lower in Chrome and a bit lower in Firefox even while the spinner is being displayed. I'd recommend to consider using this CSS technique wherever a background spinner is used in the admin.
Tried also to cache a bit this.$el.

https://cldup.com/AWjm1-iVse.png

#12 @wonderboymusic
6 years ago

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

In 35281:

Theme Installer: toggle iframe-ready on the overlay to control the spinner animated GIF and high CPU usage.

Props afercia, adamsilverstein.
Fixes #33322.

#13 @SergeyBiryukov
6 years ago

In 35310:

Autoprefixer for [35281].

See #33322.

Note: See TracTickets for help on using tickets.