#27355 closed task (blessed) (fixed)
Customizer: Add framework for selective refresh (partial preview refreshes)
Reported by: | westonruter | Owned by: | westonruter |
---|---|---|---|
Milestone: | 4.5 | Priority: | normal |
Severity: | normal | Version: | 3.4 |
Component: | Customize | Keywords: | has-patch has-unit-tests commit has-screenshots |
Focuses: | javascript | Cc: |
Description (last modified by )
In regards to the Widget Customizer merge in #27112, we agreed to remove the partial preview refreshing of widgets (live previews via postMessage transport and re-rendering widgets via Ajax then doing DOM replacements). This live preview functionality for widgets was removed in favor of generalizing the functionality so that any customize control can add support for doing so. This partial preview refresh is key to keeping things DRY by eliminating the current requirement that all setting updates via postMessage transport re-implement all rendering logic in JavaScript for components that are normally written out in PHP—but even when the render logic is attempted to be written in JS, it will never be able to account for any filters added in PHP.
For instance, with such partial preview refreshes, the Navigation Menu control needn't do a full refresh of the preview: the menu's container could be targeted for a partial refresh, where the new menu gets rendered via an Ajax request and the response replaces the old menu.
Originally mentioned at https://core.trac.wordpress.org/ticket/27112#comment:10
Feature plugin available
WordPress.org: https://wordpress.org/plugins/customize-partial-refresh/
GitHub project: https://github.com/xwp/wp-customize-partial-refresh
For summaries of the feature as merged into 4.5, see Make Core posts:
Attachments (12)
Change History (116)
This ticket was mentioned in IRC in #wordpress-dev by ocean90. View the logs.
11 years ago
This ticket was mentioned in IRC in #wordpress-dev by westonruter. View the logs.
11 years ago
This ticket was mentioned in IRC in #wordpress-dev by westonruter. View the logs.
11 years ago
This ticket was mentioned in IRC in #wordpress-dev by westonruter. View the logs.
10 years ago
This ticket was mentioned in IRC in #wordpress-dev by celloexpressions. View the logs.
10 years ago
#9
follow-up:
↓ 11
@
10 years ago
We might need to squeeze this into 4.1, as Twenty Fifteen implements colorschemes in a way that desperately begs for this. We'll see if it's feasible to get it patched in time, given our two other big high-priority Customizer API tickets that are still in progress/waiting for commit: #28709 and #29572. See #29988.
This ticket was mentioned in IRC in #wordpress-dev by westonruter. View the logs.
10 years ago
#11
in reply to:
↑ 9
@
10 years ago
Replying to celloexpressions:
We might need to squeeze this into 4.1, as Twenty Fifteen implements colorschemes in a way that desperately begs for this. We'll see if it's feasible to get it patched in time, given our two other big high-priority Customizer API tickets that are still in progress/waiting for commit: #28709 and #29572. See #29988.
Added a twentyfifteen-specific patch to do partial preview refreshes of inline styles: #29988 (comment)
This ticket was mentioned in IRC in #wordpress-dev by celloexpressions. View the logs.
10 years ago
#13
@
10 years ago
- Milestone changed from Future Release to 4.1
- Owner set to westonruter
- Status changed from new to assigned
Per IRC discussion, this could actually be fairly straightforward as a core API, probably simpler than implementing in a theme anyway. @westonruter will take a look for 4.1 next week if there's time after #28709 is done, and if we make it in we'll have Twenty Fifteen leverage it.
This ticket was mentioned in IRC in #wordpress-dev by celloexpressions. View the logs.
10 years ago
This ticket was mentioned in Slack in #core-themes by iandstewart. View the logs.
10 years ago
#16
@
10 years ago
Just wanted to jot down this idea I had regarding this: when a partial is in the process of being refreshing, we could apply a 0.5 opacity with a CSS transition (by default) to indicate that it is in the process of refreshing. A theme could override this with different behavior if they wanted.
Also wanted to share the idea whereby partial refresh will nicely make possible inline editing. If each partial registered has an associated CSS selector for the element inside the preview that should be refreshed when the associated setting is changed from the Customizer pane, we can use this in the opposite direction as well: when clicking on an element in the preview, any controls that are associated with that element by the partials' selectors can be identified, and the entire list of controls relevant to that context can be filtered down. What's more is that these controls could be loaded up on the frontend outside the context of the traditional Customizer at all. You could be on a page and shift-click an element (for example), to trigger the Customizer control related to that element to be dynamically created and presented as a floating box next to the element. Or the control could slide up from the bottom of the window if on mobile.
For settings registered with transport=postMessage
, we should also let these be eligible for inclusion in the frontend in the same way, even though there is no specific partial that they are associated with. Perhaps the CSS selector shouldn't be a property of a new “Partial” object at all, but rather a property on the Customizer Setting object. This would allow any setting to be associated with one (or more) CSS selectors. The “postMessage” transport would then be synonymous with just a “js” transport. And actually, “transport” becomes somewhat of a poor name since there is no cross-frame communication at all if these interactions are happening on the front-end.
Anyway, I wanted to dump out these ideas here, even though they should get a separate ticket, so that the ideas can help guide the implementation of partial preview refreshes as a first step.
This ticket was mentioned in Slack in #core by westonruter. View the logs.
10 years ago
This ticket was mentioned in Slack in #core by westonruter. View the logs.
10 years ago
#19
@
10 years ago
In 27355.wip.diff (HEAD: 6847dde
):
- Clean up customize-setting classes for coding standards, phpDoc.
- Introduce Setting.selector for frontend partial association.
- Eliminate duplicated values exported in preview.
- Let Pane sync settings into customizer via postMessage.
- Sync settings objects (w/ selector) instead of just the values.
- Add
WP_Customize_Setting::json()
. - Update TwentyFourteen to re-use selector defined in PHP.
See GitHub PR: https://github.com/xwpco/wordpress-develop/pull/51
With the selector now in place, the next step is to add support for the partialRefresh
transport type.
#20
follow-up:
↓ 21
@
10 years ago
- Focuses ui removed
A thought:
- If a setting has a
selector
andtransport: postMessage
, do$( selector ).html( to );
for the setting in core, eliminating the need to implement it in JS. If no selector is specified, require implementing it manually as you must currently do. In other words, by specifying a selector you say that the value of that setting is the content of that selector, so no other handling is needed. - That wouldn't let you specify a selector for ones that you have to actually use it yourself in JS, but on the other hand such JS is typically very dom-oriented and front-end, so specifying a selector in PHP then getting it from the API in JS would be fairly roundabout anyway.
- That's not directly related to partial refreshes, but makes it even easier to implement partial refreshes where the entire contents of the selector is the value of the setting.
Also,
- Unfortunately, we're not going to be able to make any changes to Twenty Fourteen here, since it needs to be 100% back-compat to 3.8. But we can do whatever we'd like for Twenty Fifteen.
WP_Customize_Manager::update_settings()
could be really useful. It should probably wrap a singular version, which would be a bit nicer than modifying properties directly, as the currentget_
methods require. Of course, we would need to introduce this for all of the Customizer objects - panels, sections, settings, and controls. Probably needs a new ticket, maybe not 4.1 material.- Other than that, the current patch looks like a good start.
#21
in reply to:
↑ 20
@
10 years ago
Replying to celloexpressions:
- If a setting has a
selector
andtransport: postMessage
, do$( selector ).html( to );
for the setting in core, eliminating the need to implement it in JS. If no selector is specified, require implementing it manually as you must currently do. In other words, by specifying a selector you say that the value of that setting is the content of that selector, so no other handling is needed.
Yeah, good thought. However, only sometimes is the JS behavior just sticking the value into a certain element as the HTML or (better) text content. Changing a color, for instance, wouldn't make sense at all to use $( setting.selector ).text( value )
. So then that would necessitate having some declarative way in PHP to indicate how JS should handle the updating of a setting on the element(s) found by the selector: do you update the text content? Do you replace the HTML for the entire element? Do you change an attribute's value? Do you change some style property? So you can see that this quickly can get out of hand, this automatic generating JS logic to apply setting changes. So I think for now it should continue to be a manual update if postMessage
transport is supplied.
- That's not directly related to partial refreshes, but makes it even easier to implement partial refreshes where the entire contents of the selector is the value of the setting.
Yeah, if the transport is partialRefresh
, then we can automatically apply the change because by definition we're updating a specific element, and not having to do any special logic. We would, however, be firing events when a partial gets updated so that Customizer preview JS can re-attach any event handlers or construct dynamic elements.
Even in this case, however, I think the default handler for refreshing a partial's element should be allowed to be explicit. When attaching these default behaviors, we could check to see if the theme already defines custom behaviors for a partial refresh and defer to it.
- Unfortunately, we're not going to be able to make any changes to Twenty Fourteen here, since it needs to be 100% back-compat to 3.8. But we can do whatever we'd like for Twenty Fifteen.
Thanks for pointing that out. I'll revert.
WP_Customize_Manager::update_settings()
could be really useful. It should probably wrap a singular version, which would be a bit nicer than modifying properties directly, as the currentget_
methods require. Of course, we would need to introduce this for all of the Customizer objects - panels, sections, settings, and controls. Probably needs a new ticket, maybe not 4.1 material.
OK, so $wp_customize->update_setting( $id, $array )
and $wp_customize->update_settings( array( $id1 => array(…), $id2 => array(…), … ) )
? And then implement update_panel(s)
, update_section(s)
, update_control(s)
as well? Good stuff. Yeah, I agree that this should go in another ticket.
This ticket was mentioned in Slack in #core by westonruter. View the logs.
10 years ago
#23
@
10 years ago
I suggest punting this to 4.2 and instead rely on JS in the Customizer for compiling the CSS for TwentyFifteen. For TwentyFifteen and #29988, using JS to manage the CSS makes for a much better experience in that the changes happen instantly without any HTTP requests, and it is DRY in that the JS-rendered CSS gets stored in the theme_mod to then be output via PHP for normal visitors. The caveat is that the settings that comprise the CSS must only be edited in the Customizer (including the background color), so any other admin pages that touch those Appearance settings have to be disabled (#25569 and #25571).
In 29988#comment:20 I also get into the use of logic-less Mustache templates for applying settings to templates. This is a piece missing from Core I think. Underscore templates often have JS code embedded within them, and this makes them unusable for rendering settings onto templates via PHP. Likewise, PHP “templates” are even more logic-filled and so are unsuitable for rendering settings onto templates with JS.
Taylor Buley presents a great idea in his talk Taylor Buley: How Parade.com Uses the WordPress Theme Customizer API for how they used Mustache templates for rendering template partials as opposed to straight PHP, and this enabled them to use postMessage
transport much more extensively.
I know it is unrealistic to expect developers to start writing Mustache templates exclusively, so the partial refresh proposed in this ticket is sort of a stopgap.
#25
@
10 years ago
For a client project I'm assigned to, we're going to be heavily using widgets and the Customizer. (Big surprise.) I've gone back to the old Widget Customizer feature-as-plugin code and resurrected the partial refresh logic we had in there and put it into a new standalone plugin: Customize Partial Refresh.
This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.
10 years ago
#27
@
10 years ago
- Milestone changed from Future Release to 4.2
Adding this to 4.2 since it is closely related to #30937 (Customize Transactions), and since transactions currently re-implement the preview refresh functionality to do a regular location.reload()
, resulting in a momentary page redraw which would be eliminated by partial refreshing (and make refreshing faster across the board, anyway).
This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.
10 years ago
This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.
10 years ago
#34
@
9 years ago
- Milestone changed from Future Release to 4.4
@westonruter has expressed interest in shepherding this in 4.4
This ticket was mentioned in Slack in #core by wonderboymusic. View the logs.
9 years ago
#37
@
9 years ago
See also #33738 which highlights a problem with postMessage
for previewing changes: it fails to take into account any PHP filters that may have been applied, such as applying wptexturize
to the blogname
. This specific issue would be solved by the partialRefresh
transport, at the expense of a slower preview update (though still much faster still than a full page refresh).
#38
@
9 years ago
- Summary changed from Customizer: Add framework for partial preview refreshes to Customizer: Add framework for selective refresh (partial preview refreshes)
This ticket was mentioned in Slack in #core by westonruter. View the logs.
9 years ago
This ticket was mentioned in Slack in #core by westonruter. View the logs.
9 years ago
#42
@
9 years ago
Yep. I have a work-in-progress patch, but making a general “selective refresh” framework that can be used for nav menus, widgets, and any other thing that needs to be re-rendered via PHP is going to take time to nail, and it will require community testing to make sure the use cases are accounted for. So yeah, this needs feature plugin.
#43
@
9 years ago
@valendesigns is now picking up work on Selective Refresh in the context of the “Customize Partial Refresh” plugin. We had a chat about a possible architecture for the framework, and I summarized it at https://github.com/xwp/wp-customize-partial-refresh/issues/11#issuecomment-156346576
Feedback welcome!
#44
@
9 years ago
I've created a PR https://github.com/xwp/wp-customize-partial-refresh/pull/17 for this feature in the wp-customize-partial-refresh
plugin. I would appreciate all feedback and discussion around this approach with anyone who is interested.
This ticket was mentioned in Slack in #core by westonruter. View the logs.
9 years ago
This ticket was mentioned in Slack in #core by westonruter. View the logs.
9 years ago
This ticket was mentioned in Slack in #core by mike. View the logs.
9 years ago
#49
@
9 years ago
I just found from Make post that this ticket is expecting feedback from Theme and Plugin Developer. Being a Theme developer I would also like to help in this feature. What specific thing are we expecting from Theme developer? Ease in using feature? Testing several widgets?
#50
@
9 years ago
@rabmalin Thank you. It's specifically the framework proposed here in this PR: https://github.com/xwp/wp-customize-partial-refresh/pull/17
What the framework should make easy is the ability to use PHP to render the changes to a given setting. Let's say you have an option that contains a welcome message on your site. You want this message be texturized and to allow shortcodes to be embedded in it, and anything else that the_content
filters apply. You should then be able to register a new Customizer control for this option, say it a textarea
, and with selective refresh you can make changes to the text in the control and watch only the element containing the welcome message refresh, not the entire page, making the changes _much_ faster to appear. The framework introduced to Core should make it easy to do this, and the patch in the PR is a proposed API for this.
Note that this does not include widgets in its scope. It is intended as the underlying selective-refresh framework which can be used by widgets and nav menus and anything else to be previewed. The existing selective refresh for nav menus should be re-implemented to use this framework.
#51
follow-up:
↓ 52
@
9 years ago
Great. Nice work done. Following things came to my mind after testing feature/framework
branch of the repo.
add_theme_support
declaration could be omitted. I would like to go with Nick in this. Iftransport
ispartialRefresh
then framework should automatically apply that method.- Separate line for
selector
assignment in separate line could be simplified by passing it as arguments inadd_setting
. - When I tested using
partialRefresh
, I noticed two request on every change. One was AJAX request and another was request for whole site. I guess that was for full page refresh. May be this is a bug. - It would be great to some kind of notice if
transport
is setpartialRefresh
andselector
is not set.
#52
in reply to:
↑ 51
@
9 years ago
Replying to rabmalin:
Great. Nice work done. Following things came to my mind after testing
feature/framework
branch of the repo.
add_theme_support
declaration could be omitted. I would like to go with Nick in this. Iftransport
ispartialRefresh
then framework should automatically apply that method.- Separate line for
selector
assignment in separate line could be simplified by passing it as arguments inadd_setting
.
Note that the selector
won't have to be defined outside of the args passed into WP_Customize_Setting
once a core merge happens because at that time it will be a core property defined on the class, and so it will recognized automatically.
- When I tested using
partialRefresh
, I noticed two request on every change. One was AJAX request and another was request for whole site. I guess that was for full page refresh. May be this is a bug.- It would be great to some kind of notice if
transport
is setpartialRefresh
andselector
is not set.
I've just updated the patch with a round of improvements, including most of the feedback so far: https://github.com/xwp/wp-customize-partial-refresh/pull/17#issuecomment-172401750
OK, pushed 93765c4 which introduced several changes:
- Rename to follow selective-refresh naming.
- Re-use postMessage transport so that JS updates can be used for live approx. updates followed by selective refresh PHP-rendered data.
- Send selective refresh requests in batch, with Ajax response returning multiple partials
- Use
postMessage
to send selective refresh partials into preview- Send message to preview when selective refresh starts so element can be styled.
- Remove spinner since selective refresh can be indicated in preview elements.
- Refresh if no elements in preview match selector.
- Inject selector property into Setting instances, to anticipate extension to
WP_Customize_Setting::json()
- Let
WP_Customize_Setting::value()
be default rendered partial, usingWP_Customize_Setting::render()
if it exists. Supportingrender_callback
arg would need to be part of the Core merge.- Add todos for future improvements.
(Yes, the unit tests are now failing.)
I also created a demo plugin (Site Title Smiles) that uses the plugin at the current state: https://gist.github.com/westonruter/a15b99bdd07e6f4aae7a
Here's a demo video (note how text changes apply immediately via
postMessage
and then get upgraded to PHP-filtered text to includewptexturize
andconvert_smilies
once the selective refresh gets applied when the Ajax request completes):
I realized a few things when working on this patch.
What we're doing here is basically implementing the logic to calculate and preview what the REST API identifies as the
rendered
properties, where the REST API'sraw
properties are what the Customizer actually has stored in the settings. There is a difference here, however, because a setting could be rendered in _multiple_ contexts in the preview, displayed in different ways in different template partials. One partial may consist entirely of the PHP-sanitized setting, whereas another partial may incorporate the setting's value as _part_ of the content. So I think our one-to-one mapping of settings to rendered partials is too simple and we need to devise a one-to-many relationship of a setting to one or more partials. This is somewhat what would be needed by nav menus since changing anav_menu_item
wouldn't correspond to updating a single nav menu itemli
in the preview: instead, it actually causes the element representing the nav menu to be refreshed. So allnav_menu_item
settings would need to be linked to thenav_menu
, and the API should support that.
I'll also note that what this plugin now does is provide a way to easily see what the PHP-sanitized setting value will be. This is what the Customize Setting Validation plugin provides _upon save_, where the actual saved values get pushed back into the JS models for the settings. With transactions, each setting change would push the setting value into the transaction post via a REST API
PATCH
request, and the response there can include the PHP-sanitized value to make sure that the JS setting gets updated to reflect any PHP sanitization at the time of input.
#53
follow-up:
↓ 54
@
9 years ago
Option 1
- Use
postMessage
intransport
- If
selector
is set apply Partial Refresh, otherwise applypostMessage
.
Option 2
- Use
refresh
intransport
- If
selector
is set apply Partial Refresh, otherwise applyrefresh
.
Which would be good option?
#54
in reply to:
↑ 53
@
9 years ago
Replying to rabmalin:
Which would be good option?
I suggest option 1: augmenting postMessage
transport with selective refresh. From the demo video, you can see this has the added benefit of allowing a JS implementation to do an approximate instant preview of a setting change while waiting for the Ajax response to return with the actual rendered contents. The JS preview logic is not required here.
#55
follow-up:
↓ 56
@
9 years ago
@westonruter Will the plugin repo be updated? Testing is much easier when the WP update mechanism can be used, especially now that Calypso and Shiny Updates allow auto update of plugins. https://wordpress.org/plugins/customize-partial-refresh/
#56
in reply to:
↑ 55
@
9 years ago
Replying to ryan:
@westonruter Will the plugin repo be updated? Testing is much easier when the WP update mechanism can be used, especially now that Calypso and Shiny Updates allow auto update of plugins. https://wordpress.org/plugins/customize-partial-refresh/
Once the PR gets merged. I wanted to try out an API incorporating the WP_Customize_Partial
(or WP_Customize_Template_Part
) which allows multiple settings to be associated with a rendering. In many ways, the class is looking similar to WP_Customize_Control
, except the settings are associated for rendering instead of editing.
This ticket was mentioned in Slack in #core by mike. View the logs.
9 years ago
#60
@
9 years ago
Here's the latest work done to introduce the “Partial” construct to the Customizer APIs: https://github.com/xwp/wp-customize-partial-refresh/pull/18
Introduced are WP_Customize_Partial
PHP class and a corresponding wp.customize.Partial
in JS. Partials are registered in PHP via (eventually) $wp_customize->add_partial()
and in JS via wp.customize.partial.create()
.
One interesting bit is how to standardize handling the JS-initialization of any dynamic content, such as Emojis or MediaElement.js players. It will presently copy some of the logic developed for Jetpack's Infinite Scroll, but it would be great of there was a standard event that JS could listen for to make the necessary DOM changes (adding events, replacing elements, etc), similar to the widget-added
and widget-updated
events that are triggered on document
.
#61
@
9 years ago
Latest changes merged into feature branch PR: https://github.com/xwp/wp-customize-partial-refresh/pull/17
For another example for how to make use of this API, see this “Welcome Message” plugin which adds a TinyMCE rich text editor control to the Customizer with changes that get selectively-refreshed in the preview: https://gist.github.com/westonruter/ed4ae3f8b6f6d653e0c6
Quick demo video: https://www.youtube.com/watch?v=hb3uezGTzFc
Next steps include rounding off some rough edges, and reworking nav menus partial refreshing in Core to make use of this new API to make sure that it handles the (complicated) use case, and then to implement the even more complicated use case of widgets.
This ticket was mentioned in Slack in #core by westonruter. View the logs.
9 years ago
#63
@
9 years ago
Finally, a new version of the Customize Partial Refresh plugin has been released.
Please review the Core dev chat logs today: https://wordpress.slack.com/archives/core/p1455139493002289
I'm looking for feedback on compatibility between the feature plugin and your themes and especially any widgets that are available. I'm also looking for feedback on the API it provides. I will be writing up a post on Make Core that describes the API, but in the mean time you can look at the existing plugins referenced on the plugin readme or look at how partial refresh is implemented for nav menus and widgets in the plugin itself.
Note that the plugin requires 4.5-alpha, so please use the latest nightly to test.
#64
@
9 years ago
Tested On 8 Themes
All Default Widgets And Menus Work As Expected
@westonruter,
This feature plugin worked exactly as expected for the widgets and menus in the following themes in the WordPress.org theme directory:
Graphene: https://wordpress.org/themes/graphene/
Hueman: https://wordpress.org/themes/hueman/
Responsive: https://wordpress.org/themes/responsive/
Spacious: https://wordpress.org/themes/spacious/
Sparkling: https://wordpress.org/themes/sparkling/
Sydney: https://wordpress.org/themes/sydney/
Twenty Sixteen: https://wordpress.org/themes/twentysixteen/
Zerif Lite: https://wordpress.org/themes/zerif-lite/
These all have over 60,000 active downloads. This isn't to speak for the theme authors, though.
All of the default widgets worked as expected: "Archives," "Calendar," "Categories," "Custom Menu," "Meta," "Pages," "Recent Comments," "Recent Posts," "RSS," "Search," "Tag Cloud," and "Text." Also, the menus update instantly in the Customizer. The "Menu Location" control works well, as do the individual menu controls.
Using a control for a widget quickly produced a change in only the given widget in the preview iframe. This included the "select" element for the "Archives" and "Categories" widgets. I used the theme test data that wordpress.org recommends to test new themes: https://wpcom-themes.svn.automattic.com/demo/theme-unit-test-data.xml
As @westonruter showed in the video on this ticket, only the widget with the change was refreshed. It very briefly became lighter, and refreshed with the new content.
Plugin version: 0.5.4
WordPress version: 4.5-alpha-35776-src
#65
@
9 years ago
Just published a technical writeup in a Make Core post: https://make.wordpress.org/core/2016/02/16/selective-refresh-in-the-customizer/
This ticket was mentioned in Slack in #core by westonruter. View the logs.
9 years ago
#67
@
9 years ago
- Keywords has-patch needs-testing added; needs-patch removed
27355.0.diff ports the feature plugin into a core patch. I didn't include the unit tests in this patch, aside from changes to the existing unit tests for the feature.
Automated tests pass with patch applied: https://travis-ci.org/xwp/wordpress-develop/builds/110013004
The Selective Refresh functionality can be disabled entirely by filtering selective_refresh
out of the array passed into the customize_loaded_components
filter.
#68
@
9 years ago
Added 27355.diff which covers the inline docs audit requested by @mikeschroder as well as some coding standards fixes.
So as not to hold up merge, I've opened #35855 to decide whether we should wrap isset( $this->manager->selective_refresh )
in a class method to DRY up some of the conditionals introduced here.
#69
@
9 years ago
- Keywords needs-testing removed
OK, I've finished off the PHP unit tests as well as make a few additional tweaks as seen in the patches and GitHub compare views above. Note that the patch does not include QUnit tests since there are no tests at all for the Customizer preview currently. I've opened #35857 specifically to address that outside the scope of this ticket.
@ocean90 I'll commit the patch once you've had a look.
#71
@
9 years ago
- Owner changed from westonruter to ocean90
- Status changed from assigned to reviewing
#72
follow-ups:
↓ 73
↓ 76
↓ 78
↓ 79
↓ 80
@
9 years ago
- Owner changed from ocean90 to westonruter
- Widgets: When adding a widget to an empty sidebar I get a JS error:
Uncaught TypeError: this.targetWindow is not a function @ customize-base.js?ver=4.5-alpha-35776-src-1455792213:707 api.Messenger.api.Class.extend.receive @ customize-base.js?ver=4.5-alpha-35776-src-1455792213:707 n.isFunction.d @ jquery.js?ver=1.12.0:2 n.event.dispatch @ jquery.js?ver=1.12.0:3 r.handle @ jquery.js?ver=1.12.0:3
- Nav Menus: When I enter "Foo<" into the input field for a menu item title I get a PHP warning:
Object {partial: "nav_menu_instance[c306e5285ebf74fc00b87f1d9b839d2a]", error_number: 8, error_string: "Uninitialized string offset: 1", error_file: "/srv/www/wp-develop/svn/src/wp-includes/formatting.php", error_line: 377} @ customize-selective-refresh.js?ver=4.5-alpha-35776-src-1455794958:604 Object {partial: "nav_menu_instance[c306e5285ebf74fc00b87f1d9b839d2a]", error_number: 8, error_string: "Uninitialized string offset: 1", error_file: "/srv/www/wp-develop/svn/src/wp-includes/formatting.php", error_line: 377} @ customize-selective-refresh.js?ver=4.5-alpha-35776-src-1455794958:604
(Note: It seems like something is called twice on menu updates.)
- In
WP_Customize_Nav_Menus::filter_wp_nav_menu_args()
some docs for$can_partial_refresh
might be helpful. WP_Customize_Widgets::filter_wp_kses_allowed_data_attributes()
should have thepublic
keyword and not@access private
.- In
WP_Customize_Widgets::end_dynamic_sidebar()
let's remove theassert()
line. - Running all unit tests dies at ~17%:
Starting test 'Tests_DB::test_bail'. Database is dead.
. It seems likeTest_WP_Customize_Selective_Refresh_Ajax
is the culprit because it runs on the default group and definesDOING_AJAX
.
Other notes:
- Nav Menus: Probably unrelated: Assign an author the
edit_theme_options
cap and enter "foo'foo" into the input field for a menu item title: For some reasons the preview will have slashes. (https://cloudup.com/ccKXry0gjDC) - Nav Menus: Users with the
unfiltered_html
cap can break the customizer, the same happens without the patch too, see https://gist.github.com/ocean90/db41b33785679b85c73a
#73
in reply to:
↑ 72
;
follow-up:
↓ 75
@
9 years ago
Replying to ocean90:
- Nav Menus: When I enter "Foo<" into the input field for a menu item title I get a PHP warning:
Object {partial: "nav_menu_instance[c306e5285ebf74fc00b87f1d9b839d2a]", error_number: 8, error_string: "Uninitialized string offset: 1", error_file: "/srv/www/wp-develop/svn/src/wp-includes/formatting.php", error_line: 377} @ customize-selective-refresh.js?ver=4.5-alpha-35776-src-1455794958:604 Object {partial: "nav_menu_instance[c306e5285ebf74fc00b87f1d9b839d2a]", error_number: 8, error_string: "Uninitialized string offset: 1", error_file: "/srv/www/wp-develop/svn/src/wp-includes/formatting.php", error_line: 377} @ customize-selective-refresh.js?ver=4.5-alpha-35776-src-1455794958:604
I grabbed a stack trace for when this error occurs and I found:
#0 /srv/www/wordpress-develop/src/wp-includes/formatting.php(377): WP_Customize_Selective_Refresh->handle_error(8, 'Uninitialized s...', '/srv/www/wordpr...', 377, Array) #1 /srv/www/wordpress-develop/src/wp-includes/formatting.php(240): _wptexturize_pushpop_element('<', Array, Array) #2 [internal function]: wptexturize('Fosssos<') #3 /srv/www/wordpress-develop/src/wp-includes/plugin.php(235): call_user_func_array('wptexturize', Array) #4 /srv/www/wordpress-develop/src/wp-includes/nav-menu-template.php(164): apply_filters('the_title', 'Fosssos<', 4946) #5 [internal function]: Walker_Nav_Menu->start_el('<li id="menu-it...', Object(WP_Post), 0, Object(stdClass)) #6 /srv/www/wordpress-develop/src/wp-includes/class-wp-walker.php(146): call_user_func_array(Array, Array) #7 /srv/www/wordpress-develop/src/wp-includes/class-wp-wa in <b>/srv/www/wordpress-develop/src/wp-includes/customize/class-wp-customize-selective-refresh.php</b> on line <b>283</b><br />
As such, it looks like it is specifically an issue with wptexturize
and not with selective refresh or nav menus in the customizer. If you save the menu item and then exit the Customizer, you'll see the same PHP notice if WP_DEBUG_DISPLAY
is enabled. This patch fixes the issue:
-
src/wp-includes/formatting.php
function wptexturize_primes( $haystack, $needle, $prime, $open_quote, $close_quo 374 374 */ 375 375 function _wptexturize_pushpop_element( $text, &$stack, $disabled_elements ) { 376 376 // Is it an opening tag or closing tag? 377 if ( '/' !== $text[1] ) {377 if ( isset( $text[1] ) && '/' !== $text[1] ) { 378 378 $opening_tag = true; 379 379 $name_offset = 1; 380 380 } elseif ( 0 == count( $stack ) ) {
(Note: It seems like something is called twice on menu updates.)
@ocean90 Do you have the same menu appearing in multiple places, such as assigned to a nav menu location and to a Custom Menu widget? If so, then it is expected that the call to render would happen multiple times since there would be multiple placements.
#75
in reply to:
↑ 73
@
9 years ago
Replying to westonruter:
@ocean90 Do you have the same menu appearing in multiple places, such as assigned to a nav menu location and to a Custom Menu widget?
Yeah, you're right. Twenty Sixteen includes the primary menu in the footer too.
#76
in reply to:
↑ 72
@
9 years ago
Replying to ocean90:
- Widgets: When adding a widget to an empty sidebar I get a JS error:
I've seen this before as well in certain situations but didn't patch it yet. The issue can be easily reproduced even without selective refresh by adding a button in the preview which does:
wp.customize.preview.send( 'refresh' ); wp.customize.preview.send( 'refresh' );
This will also trigger the error.
I've filed a ticket for this and patched it in #35866.
The alternative fix, to address the symptom instead of the underlying cause, is to debounce the requestFullRefresh
function:
-
src/wp-includes/js/customize-selective-refresh.js
wp.customize.selectiveRefresh = ( function( $, api ) { 507 507 * 508 508 * @since 4.5.0 509 509 */ 510 self.requestFullRefresh = function() {510 self.requestFullRefresh = _.debounce( function() { 511 511 api.preview.send( 'refresh' ); 512 } ;512 } ); 513 513 514 514 /** 515 515 * Request a re-rendering of a partial.
But better to fix the underlying issue instead.
#78
in reply to:
↑ 72
@
9 years ago
Replying to ocean90:
- In
WP_Customize_Nav_Menus::filter_wp_nav_menu_args()
some docs for$can_partial_refresh
might be helpful.WP_Customize_Widgets::filter_wp_kses_allowed_data_attributes()
should have thepublic
keyword and not@access private
.- In
WP_Customize_Widgets::end_dynamic_sidebar()
let's remove theassert()
line.
Done in 27355.4.diff, including the moving of the refresh
message receiving from customize-nav-menus.js
to the main customize-controls.js
file.
- Running all unit tests dies at ~17%:
Starting test 'Tests_DB::test_bail'. Database is dead.
. It seems likeTest_WP_Customize_Selective_Refresh_Ajax
is the culprit because it runs on the default group and definesDOING_AJAX
.
I'm trying to figure out why this is happening.
- Nav Menus: Probably unrelated: Assign an author the
edit_theme_options
cap and enter "foo'foo" into the input field for a menu item title: For some reasons the preview will have slashes. (https://cloudup.com/ccKXry0gjDC)- Nav Menus: Users with the
unfiltered_html
cap can break the customizer, the same happens without the patch too, see https://gist.github.com/ocean90/db41b33785679b85c73a
I'll see if I can fix these as well.
#79
in reply to:
↑ 72
@
9 years ago
Replying to ocean90:
- Running all unit tests dies at ~17%:
Starting test 'Tests_DB::test_bail'. Database is dead.
. It seems likeTest_WP_Customize_Selective_Refresh_Ajax
is the culprit because it runs on the default group and definesDOING_AJAX
.- Nav Menus: Users with the
unfiltered_html
cap can break the customizer, the same happens without the patch too, see https://gist.github.com/ocean90/db41b33785679b85c73a
These are addressed in 27355.6.diff. As in the screenshots above, when a partial's content is rendered the parsing of the HTML is wrapped in a try/catch block, and in the case of an error being thrown the error will be displayed at the end of the partial's container. In addition to a JS script error, it also captures document.write()
calls which likewise blow away the preview.
- Nav Menus: Probably unrelated: Assign an author the
edit_theme_options
cap and enter "foo'foo" into the input field for a menu item title: For some reasons the preview will have slashes. (https://cloudup.com/ccKXry0gjDC)
Looking at this now.
#80
in reply to:
↑ 72
;
follow-up:
↓ 81
@
9 years ago
- Keywords commit has-screenshots added
- Status changed from reviewing to accepted
Replying to ocean90:
- Nav Menus: Probably unrelated: Assign an author the
edit_theme_options
cap and enter "foo'foo" into the input field for a menu item title: For some reasons the preview will have slashes. (https://cloudup.com/ccKXry0gjDC)
I narrowed this down to the following line in WP_Customize_Nav_Menu_Item_Setting::sanitize()
:
<?php $menu_item_value['title'] = apply_filters( 'title_save_pre', $menu_item_value['title'] );
The the wp_filter_kses
function is adding the slash which applies on this title_save_pre
filter. For some reason, the function does:
<?php return addslashes( wp_kses( stripslashes( $data ), current_filter() ) );
Which is the reason for the slash being injected, because addslashes()
adds slashes before apostrophes, even though there wasn't a slash that got stripped originally by stripslashes
. So, to me this function and (wp_filter_post_kses
like it) looks like it is doing the wrong thing. The easiest way I see to fix the issue is to bypass those KSES functions altogether with something like nav-menu-item-kses-filter-fix.diff.
All of this to say, no, it is not related to the new selective refresh functionality being introduced here.
Adding commit
keyword, and I'll commit in ~12 hours, unless anything else is identified needing to be fixed prior to commit.
#81
in reply to:
↑ 80
@
9 years ago
Replying to westonruter:
All of this to say, no, it is not related to the new selective refresh functionality being introduced here.
Moved into #35869.
Adding
commit
keyword, and I'll commit in ~12 hours, unless anything else is identified needing to be fixed prior to commit.
No objections. Only a minor thing which can happen on commit.
There is too much work to do here to make it for 3.9. Punting this to future release.
For reference, the partial preview refresh code removed from Widget Customizer can be seen here: https://github.com/x-team/wp-widget-customizer/compare/without-partial-previews