WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#33311 closed enhancement (fixed)

TB_window spinner should probably disappear when loading is done

Reported by: niklasbr Owned by: afercia
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.2
Component: Plugins Keywords: has-patch commit
Focuses: ui, javascript, administration, performance Cc:

Description

Background

Some third-party WordPress services such as Gravity Forms or WPMU Dev supply their own source for the "View version X.Y details" ThickBox content. Unlike WordPress.org this supplied content does not always specify a background colour or background image for the loaded content.

This results in a visible spinner which continues to spin after all content has been properly loaded. This is most visible on very small screens where the spinned can interfere with the content above it (a from a z layering perspective) or very large screens where the spinner can again be visible below (a y axis perspective) the content.

Two ideas for possible solutions

  1. Update the documentation for the ThickBox to specify that the spinner need to be covered or removed. However, this is not optimal because something that can be fixed with code will fix the issue for everyone, not everyone will read updated documentation (unless there is a deprecation message).
  1. Use a callback to add or remove a class which controls the spinner to #TB_window which removes the animated gif. I would prefer this solution because this would decrease energy usage (by a small amount, sure, but still) because the spinner is always animating, even when it is obscured by other elements, constantly redrawing/recompositing the content.

I will be attaching two files, one is a screen shot showing the effect with Gravity Form's update dialogue (third-party iframe source). The other file is a screen recording of Yoast SEO (WordPress.org iframe source) using Firefox's Paint Flashing Tool demonstrating the effect of removing the animated image on resource usage.

Attachments (6)

TB window constant redraws.mp4 (9.2 MB) - added by niklasbr 6 years ago.
Firefox dev tools showing the constant redraws/compositions despite the hidden spinner
GF iframe spinner background.png (78.9 KB) - added by niklasbr 6 years ago.
Screen grab showing the spinner interfering with content when the loaded content has no background
33311-Fix-excessive-resource-usage-due-to-animating-GIF-1.patch (4.1 KB) - added by niklasbr 6 years ago.
First patch
33311.test.patch (1.5 KB) - added by afercia 6 years ago.
Testing how to reduce CPU usage.
33311.patch (4.4 KB) - added by afercia 6 years ago.
33311.2.patch (6.8 KB) - added by afercia 6 years ago.
Adds support for IE 8

Change History (28)

@niklasbr
6 years ago

Firefox dev tools showing the constant redraws/compositions despite the hidden spinner

@niklasbr
6 years ago

Screen grab showing the spinner interfering with content when the loaded content has no background

#1 @afercia
6 years ago

I can confirm that on old hardware the spinner causes very high CPU usage, until the thickbox modal is closed, especially when using Firefox but also with other browsers. On very old hardware the CPU usage can reach 100%. Noticed while working on #33305 where I'm experimenting a callback on iframe load (Plugin detail modals use an iframe). I'd agree with @niklasbr that should probably be a built-in thickbox feature. Thickbox is not the only place where "persistent" spinners are used though.

#2 @niklasbr
6 years ago

Thanks @afercia. Nice input.

Thickbox is not the only place where "persistent" spinners are used though.

I browsed around in the admin interface and Customizer but did not find anywhere else than plugin install/update where it happened. But I am sure I missed some.

#3 @afercia
6 years ago

I think the theme installer (Themes > Add New > Preview) .wp-full-overlay-main has one spinner which is always "spinning" in the background also when overlaid by the Preview iframe and never gets removed. Should probably be handled in a separate ticket though.

#4 @afercia
6 years ago

@niklasbr could you check if removing box-shadow set on the #TB_window helps a bit Firefox? It does for me, reducing the repainting to just the spinner 20px x 20px area. It doesn't completely solve the problem and won't help with other browsers though.

#5 @atomicjack
6 years ago

  • Keywords needs-patch added

#6 @niklasbr
6 years ago

@afercia, as you say it does cut the CPU usage in half when using Firefox 39 on my MacBook Pro. But in my quick testing Chrome does not seem to care whether or not box-shadow is active and repaints the entire TB_window area, and Safari seems to only repaint the 20x20 area.

I don't think removing the box-shadow is a reasonable solution in any case since .media-modal-content has a shadow and I think consistency is important.

I'll dive deeper into this by the end of the week, see what I can do about a patch.

#7 @niklasbr
6 years ago

I did a cursory look at the ThickBox code and pretty much found a simple solution. TB is showing its age, but still works pretty well and is straight-forward enough. Thoughts?

#8 @niklasbr
6 years ago

  • Keywords has-patch added; needs-patch removed

#9 @niklasbr
6 years ago

  • Keywords dev-feedback added

#10 @afercia
6 years ago

Patching tb_showIframe() seems the most logical thing, since it's already the "callback" we need :) About the patch format, there are some things to fix. Please have a look at the Handbook: https://make.wordpress.org/core/handbook/tutorials/working-with-patches/
Patches should preferably be made against a SVN install. Also, no need to patch .rtl (or minified) files, they will be generated during the build process.

#12 @niklasbr
6 years ago

Thanks for your feedback @afercia! I will remove the RTL part of the patch. Though I am not sure what specifically you are referring to when you say "there are some things to fix" because from my novice point of view I used that guide when creating the patch and it seems to me (again, a novice) that I followed it as good as I could.

Upon reading it again the only thing that strikes me as wrong is the file name of the patch, it should be 33311.patch and not 33311-Fix-excessive-resource-usage-due-to-animating-GIF-1.patch, right? Anything else I am missing? Probably.

#13 @afercia
6 years ago

Yes the file name format should be ticket#.patch, there are exceptions for example when creating alternate versions etc. Generally, patches should be built from the SVN install root, so that the paths in the patch would start with src/.... This way, people who want to manually download and apply a patch could easily do that on all platforms and using different tools.
By the way, I've asked on Slack and when applying patches the Grunt patch task should handle the Git diffs too. See https://make.wordpress.org/core/handbook/testing/patch/

About the patch itself:

  • not sure we can simply use the existing ".spinner" rule since it uses some properties that should be reset for the modal (e.g. float, opacity...)
  • also, not sure the CSS class name "spinner" would be appropriate, because that’s not what we really use it for so maybe it would be more appropriate to be loading or something like that

#14 @niklasbr
6 years ago

Thanks. I'll give it another try to submit a better patch, but before that maybe we can try to agree whether or not we should use the .spinner class on the modal or some new CSS rule?

My own reason for choosing it is that it looked to me to be the solution with the least code changes needed. As you can see the patch have more deletions than insertions. The .spinner class is already using the same graphics as previously, which to me indicates that the .spinner class serves a mostly identical semantic purpose as the spinner.gif background in the modal.

A related question which occurred to me is whether or not this issue could be expanded to increase the accessibility of the modal by adding ARIA attributes and making sure it has proper focus?

By the way, I can't access the Slack channel you link to, maybe because I am not a contributor?

#15 @afercia
6 years ago

Was thinking to use a specific rule for the plugin modal spinner because I'd like to try to significantly reduce CPU usage also while the spinner is showing. Experimented a bit in the last days, would appreciate your opinion and any thoughts. See attached test patch which reduces the re-painted area to a 20px x 20px square. Should then be combined with your solution to remove the spinner after the load event.

About joining Slack, I think all you need is a wordpress.org account and start from here: https://make.wordpress.org/chat/

About the accessibility issues, see #33305 (still working on a patch for that)

@afercia
6 years ago

Testing how to reduce CPU usage.

#16 @afercia
6 years ago

Screenshot to illustrate the current situation, see how using Chrome the entire TB_window area is continuously repainted even when the spinner is not visible because is behind the iframe. Same happens in Firefox, see the video attached above. We should try to avoid to use an animated GIF as background image in a so large area. 33311.test.patch creates a small 20px x 20px area and sets the background GIF there.
Thinking we should combine the two patches and use the best of the two approaches:

  • when using an animated GIF as background image, always set it on the smallest possible area
  • always remove the background image when it's not needed anymore

https://cldup.com/CUWrn89qCF.png

#17 @afercia
6 years ago

Related (for the high CPU usage part): #33322 and #31196.

#18 @afercia
6 years ago

  • Owner set to afercia
  • Status changed from new to assigned
  • Version changed from 4.2.4 to 4.2

@afercia
6 years ago

#19 @afercia
6 years ago

  • Keywords commit added; dev-feedback removed
  • Milestone changed from Awaiting Review to 4.4

Refreshed patch. Combines the two approaches. The CSS part reduces CPU usage while the spinner is being shown. Additionally, the spinner gets removed when the iframe loading has completed. See screenshot below, the re-painted area is now a small 20px x 20px square as already done for #33322.

Note: uses a custom event which will be useful for #33305.

https://cldup.com/eKtSfVIZ-y.png

@afercia
6 years ago

Adds support for IE 8

#20 @afercia
6 years ago

Patch .2 is an alternate version if we want to support Internet Explorer 8. Otherwise 33311.patch should be good enough for modern browsers.

  • IE 8 needs a allowTransparency='true' attribute on the iframe, I guess the spinner never worked on IE 8 for this reason
  • IE 8 doesn't get dynamic changes for CSS generated elements unless the content of the pseudo element gets changed as well

Please notice the spinner animated GIF will spin noticeably slower in IE 8 and 9, that's because
any delay between animated GIFs frames less than 6/100 is changed to 10/100. The only solution would be to serve a different GIF to old IEs and I guess it's not worth it.

#21 @wonderboymusic
6 years ago

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

In 35418:

Thickbox: spinner should disappear when loading is done.

Props niklasbr, afercia.
Fixes #33311.

#22 @ocean90
6 years ago

In 35648:

Thickbox: Change only the background-image property for HiDPI screens.

background resets background-size and makes the spinner fuzzy. Introduced in [35418].

See #33311.

Note: See TracTickets for help on using tickets.