TB_window spinner should probably disappear when loading is done

[niklasbr]

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).

2. 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.

[niklasbr]

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

[niklasbr]

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

[afercia]

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.

[niklasbr]

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.

[afercia]

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.

[afercia]

@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.

[niklasbr]

@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.

[niklasbr]

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?

[niklasbr]

First patch

[afercia]

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.

[afercia]

For the ones who really want to use Git :) ​https://make.wordpress.org/core/2014/01/15/git-mirrors-for-wordpress/

[niklasbr]

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.

[afercia]

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

[niklasbr]

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?

[afercia]

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]

Testing how to reduce CPU usage.

[afercia]

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

[afercia]

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

[afercia]

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.

[afercia]

Adds support for IE 8

[afercia]

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.

[wonderboymusic]

In 35418:

Thickbox: spinner should disappear when loading is done.

Props niklasbr, afercia.
Fixes #33311.

[ocean90]

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.