#38728 closed defect (bug) (fixed)
Prevent previewer becoming frozen when refreshing too fast
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 4.7 | Priority: | high |
| Severity: | normal | Version: | 4.7 |
| Component: | Customize | Keywords: | has-patch |
| Focuses: | Cc: |
Description
When the preview is refreshed too fast, it can happen that it becomes frozen.
This bug is easy to reproduce with the Twenty Seven theme.
1) In the customizer, navigate to menus
2) Create a menu
3) Enter the menu section and try to click several times in a row on a location => At some point, the previewer won't be refreshed anymore and seem to be frozen. ( css class wp-customizer-unloading added to the preview <body> )
The menu setting is not the root cause of the issue, it's just an example of setting using the "refresh" transport type.
When changing too fast the value of a setting with "refresh" transport property, at some point, the previewer get stuck. It's really becoming a problem with text type settings for which transport is not set to postMessage or not using the partial refresh.
My understanding of this problem is that a new api.PreviewFrame is instantiated before the previous one has received the 'synced' event.
The proposed fix introduces a api._previewer_loading $.Deferred() which state is checked before firing a new refresh() call.
Attachments (5)
Change History (22)
#6
@
7 years ago
A few details about the technical choice :
At first using a new api.state Value, like api.state( 'refreshing'), seemed to be appropriate since the purpose of this fix is in fact to listen to the refresh in progress state of the api. But it appeared that the synchronous approach did not work in this particular case.
That's why I've chosen the asynchronous $.Deferred() way.
Then I was thinking to include this deferred in the list of api._deferreds first. But I realized that api._deferreds was not necessarily designed for this. Right now it's used by the background_image control only.
Related possible improvement (might be better to open a new ticket) :
Another possible improvement I've discovered while working on the refresh process would be to make it return a promise() object.
Because right now, we can add callbacks to the 'synced' previewer event, but it will be fired on all api.previewer.refresh() calls, with no possibility to target a specific refresh situation that should be followed by a custom action.
This refresh promise would be resolve() in the onceSynced() function, with a sent_preview_data object as param.
This way, it would be possible to use the following kind of syntax to fire an action on demand after a specific refresh().
api.previewer.refresh().done( function( sent_preview_data ) {
// fire actions after a specific refresh, when the preview is ready and synced
// the sent_preview_data, could be an object sent when the api.preview.send( 'synced', preview_data() ),
// providing customizable server informations that we may need after a refresh.
} );
While this deferred would be implemented in the api.previewer.refresh() method of customize-control.js, another minor addition in customize-preview.js could allow user to easily pass custom server data on refresh to the panel this way.
The idea would be to replace the current code in [customize-preview.js]https://core.trac.wordpress.org/browser/trunk/src/wp-includes/js/customize-preview.js#L679 :
api.preview.send( 'synced');
by
api.server_data = api.server_data || new api.Value( {} );
api.trigger('before_synced', api.server_data() );//<= developers can alter the api.server_data() here, before it gets sent
api.preview.send( 'synced', api.server_data() );
This way, a developer could pass custom data to the panel when api.previewer.refresh().done(), using the synced event, with this type of code :
<?php
add_action('wp_footer', function() {
if ( ! is_customize_preview() )
return;
?>
<script type="text/javascript">
jQuery( function( $ ) {
wp.customize.bind( 'before_synced', function( value ) {
//the developer sets the server_data here, before it is sent to the preview
wp.customize.server_data( { custom_data : <?php echo get_custom_data(); ?> } );
} );
} );
</script>
<?php
});
If this sounds interesting for the customize-core I can push a patch about it.
#8
@
7 years ago
- Keywords needs-patch added; has-patch removed
@nikeo thanks for the patch and the detailed writeup. I agree that refresh returning a promise would be a good improvement, but that would fall in the realm of an enhancement so we'll have to target that for 4.8.
As for the defect itself, I was able to reproduce it as well. I'm testing this by editing the blogname setting in Twenty Sixteen to disable the postMessage transport so that every change to the site title causes a refresh.
Your idea to wait to do a refresh until the previous request has finished works but it doesn't take into account a key consideration. Consider WordPress running on a slow site where a refresh could take 10 seconds. With your patch, if I make a change, then wait a second, and then make a second change while the preview is being refreshed, the second change I made will fail to appear in the preview when it finishes loading. So it is important that whenever refresh is called that it destroy any existing request to load the preview and defer to a new PreviewFrame.
#9
@
7 years ago
@nikeo I think I see a fundamental problem: sometimes the preview iframes are not getting cleaned up. As you can see from customize-iframes-not-cleaned-up.png, there are additional preview iframes that are not getting removed. In that screenshot above, if I simply delete preview-7 and preview-16 from the DOM, then my changes start appearing again.
#10
@
7 years ago
I can see that there is a race condition where destroy() is failing to get called expectedly when a new iframe is being loaded before the previous has completed loading.
#11
@
7 years ago
@westonruter,
refresh as a promise, OK let's target 4.8 for this.
current problem : thanks for the analysis + screenshot. Yes the current fix is not enough, it should address the critical problems you've raised. I'm trying to come up with an improved solution.
#13
@
7 years ago
@westonruter I think I found the reason of the fundamental problem you've pointed out, and which is actually the real problem to solve.
The previousPreview must be stored once synced, in the onceSynced callback, and as an object which life is desynchronized from the preview loading process.
I've introduced an api._previousPreview property to do the job. There should be no impact on performances, since this property will store only one api.PreviewFrame instance at a time, and will be reset on each refresh call.
The fix is now much simpler then, because following your warning, I've removed the $.deferred() part that was forcing the refresh to be done in sequence, causing slow sites to potentially miss their last api changes.
This fixes the problem on my side (still on chrome). Let me know if it works on your side.
#15
@
7 years ago
- Owner set to westonruter
- Resolution set to fixed
- Status changed from new to closed
In 39199:
#16
follow-up:
↓ 17
@
7 years ago
@nikeo cheers. I was able to put the previous PreviewFrame object on the api.previewer instead of on api itself. This will keep the top-level namespace cleaner.
#17
in reply to:
↑ 16
@
7 years ago
Replying to westonruter:
@nikeo cheers. I was able to put the previous
PreviewFrameobject on theapi.previewerinstead of onapiitself. This will keep the top-level namespace cleaner.
@westonruter yes makes sense. thx for the quick feedbacks all this.
@nikeo is your patch reversed?