WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#29572 closed enhancement (fixed)

Customizer: add a framework for rendering controls from JS templates

Reported by: celloexpressions Owned by: ocean90
Milestone: 4.1 Priority: high
Severity: normal Version: 3.4
Component: Customize Keywords: has-patch commit
Focuses: javascript Cc:
PR Number:

Description

In order to facilitate future performance improvements such as lazy-loading Customizer objects and to make it easier to dynamically add controls without Ajax calls, we should introduce a framework for rendering controls from JS templates instead of PHP.

We would maintain the current PHP API and continue support it fully. But we should be able to leverage the framework in all of the core controls in a back-compatible way.

Essentially, WP_Customize_Control->render_content() would output nothing. We would add a function to WP_Customize_Control for defining a JS template, and this would always be output in the Customizer controls footer (if possible, even for controls that weren't added with PHP, since any available control type would be able to be added dynamically later). That part may require registering available control types (and custom controls) with the Customizer, since we would want to only output the template once, and not every available control type would have controls added to it initially. All of the arguments passed in PHP when adding the control would be sent through to JS (which might eliminate the need for manually adding things to pass to_json()). Finally, all added controls of types that have templates would be rendered from their template using the parameters passed when adding the control.

In the future if controls are dynamically created or loaded, any controls with JS templates could have new instances created directly, or by retrieving information in the form of json from an Ajax call (rather than having to get the markup for the control UI from Ajax). This would be a significantly more efficient approach for things like menu items in the Customizer.

Most important thing here: make this completely optional and opt-in. The existing PHP API for controls and Custom controls would be completely untouched; this is something that would improve things internally, and that could be leveraged in custom controls.

Scope for this ticket is to create this framework and to leverage it for the main WP_Customize_Control. After that, we can explore implementing it in core's custom controls.

Attachments (6)

29572.diff (7.0 KB) - added by celloexpressions 5 years ago.
First-pass, POC, implements the framework in WP_Customize_Color_Control (as it's mostly JS anyway).
29572.2.diff (7.1 KB) - added by celloexpressions 5 years ago.
Make print_template() final (for now), several docs updates.
29572.3.diff (623 bytes) - added by johnbillion 5 years ago.
29572.4.diff (679 bytes) - added by adamsilverstein 5 years ago.
29572.5.diff (3.2 KB) - added by westonruter 5 years ago.
https://github.com/xwpco/wordpress-develop/pull/48
29572.6.diff (886 bytes) - added by ocean90 5 years ago.

Download all attachments as: .zip

Change History (35)

#1 @Aniruddh
5 years ago

For 4.0, I was expecting something like this instead of the PHP API expansion.

@celloexpressions
5 years ago

First-pass, POC, implements the framework in WP_Customize_Color_Control (as it's mostly JS anyway).

#2 follow-up: @celloexpressions
5 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 4.1

Took a first stab at this, and it's easier than I expected.

29572.diff implements a framework for rendering controls from JS/Underscore templates instead of from PHP. With multiple controls of the same type, this allows us to only send down the html once, in template form, and the data for each control, most of which was already passed as json. It also utilizes the framework in the core WP_Customize_Color_Control. As we re-think the upload and image controls in 4.1, those would also be good candidates for leveraging this.

In terms of developers leveraging this API, writing JS templates like this is extremely similar to writing PHP for displaying controls. Devs don't need to do any JS beyond the template, which is added in PHP anyway. Here's what it looks like to implement this in an existing custom control class:

  1. Make your render_content() function empty (but it does need to exist to override the default one).
  2. Create a new function, content_template(), and put the old contents of render_contents() there.
  3. Add any custom class variables that are needed for the control to the data passed to json by modifying the to_json() function (see WP_Customize_Color_Control for an example).
  4. Convert the PHP from render_content() into a JS template, using <# #> around JS statements to evaluate and {{ }} around variables to print. PHP class variables are available in the data object; for example, the label can be printed with {{ data.label }}.
  5. Register the custom control class/type. This critical step tells the Customizer to print the template for this control. This is distinct from just printing templates for all controls that were added because the ideas are that many instances of this control type could be rendered from one template, and that any registered control types would be available for dynamic control-creation in the future. just do something like $wp_customize->register_control_type( 'WP_Customize_Color_Control' );.

Ideally, with the control improvements in 4.0, custom controls should only be necessary when they need to do something with JS. Therefore, I'm okay with effectively removing the core examples of overriding the render_content() function by moving that logic to JS templates.

@westonruter: any thoughts on how/whether widgets could leverage this? I'm not sure that it would be as useful there given the need to get the widget form from PHP. Menus, on the other hand, would benefit hugely from this. I'll look at leveraging that in the plugin once this lands in core.

Note: framework for dynamically-creating controls via JS is not in the scope of this ticket (as it also requires information about associated sections/panels, priority, etc., which depends on the creation of those models). However, the code should be structured in a way that lends itself to the future build-out of a formalized framework for that. Notably this means naming things so that we can add render_container-type actions, which would probably be a function in the JS model that custom controls would only override in JS.

This should give us potentially huge performance improvements when dealing with things like menus, since we only need to send the html structure down from the server once, and can reduce individual controls to their data (in json format). That will also facilitate lazy-loading controls based on user navigation.

Related: #28709, #28580.

#3 in reply to: ↑ 2 @westonruter
5 years ago

Replying to celloexpressions:

any thoughts on how/whether widgets could leverage this? I'm not sure that it would be as useful there given the need to get the widget form from PHP.

No, widgets can't really leverage JS templates as they are now. Widget control form templates are a bit of a mess, and they need to be largely emulating what is on the widgets admin page in order to maintain compatibility with existing plugins.

This ticket was mentioned in IRC in #wordpress-dev by celloexpressions. View the logs.


5 years ago

#5 @celloexpressions
5 years ago

  • Priority changed from normal to high

#6 follow-ups: @celloexpressions
5 years ago

  • Keywords dev-feedback added

Specific dev-feedback-y questions:

  • How do we feel about the concept of "registered control types"?
  • Should we go ahead and implement render-container logic, but leave it unused in core, or is it okay to indicate that things will change a bit here in the future to accommodate that? (again, that would be necessary once we formalize the ability to dynamically render controls, and eventually do that for all controls, via ajax for unsupported types).
  • Can/should we always export all class variable to json, or should it remain a whitelist, with the requirement of adding custom class variables in subclasses (that's what #29572.diff does)?
  • Should we check if the string passed is a WP_Customize_Control before registering it, or okay to assume that it's fine?

I'd like to wait on further work on #21483 so that we can leverage this there, hence high-priority. Would be ideal to get this in sooner rather than later to facilitate additional feedback and further integrations in various core and third-party controls. But not really reasonable to demonstrate this outside of core via a plugin, much like how the Panels API was introduced.

This ticket was mentioned in IRC in #wordpress-dev by celloexpressions. View the logs.


5 years ago

#8 in reply to: ↑ 6 @celloexpressions
5 years ago

Replying to celloexpressions:

  • Should we go ahead and implement render-container logic, but leave it unused in core, or is it okay to indicate that things will change a bit here in the future to accommodate that? (again, that would be necessary once we formalize the ability to dynamically render controls, and eventually do that for all controls, via ajax for unsupported types).

Re-reading the patch, I think we can just make WP_Customize_Control::print_template() final for now.

#9 in reply to: ↑ 6 @johnbillion
5 years ago

Replying to celloexpressions:

  • How do we feel about the concept of "registered control types"?

I like this, and it makes sense from the point of view of outputting one template per control.

  • Can/should we always export all class variable to json, or should it remain a whitelist, with the requirement of adding custom class variables in subclasses (that's what #29572.diff does)?

We should stick to a whitelist. If a particular control needs access to other member variables, it can override the to_json() method.

  • Should we check if the string passed is a WP_Customize_Control before registering it, or okay to assume that it's fine?

In PHP 5.2, I don't think it's possible to check that a string is of a particular class (using is_a() or instanceof). You can only check the instantiated object. Someone correct me if I'm wrong.

@celloexpressions
5 years ago

Make print_template() final (for now), several docs updates.

#10 @celloexpressions
5 years ago

29572.2.diff makes several docs changes and marks WP_Customize_Control::print_template() as final, since that function could change significantly when we eventually introduce a framework for dynamically creating controls.

I'm hoping that we can get a first run of this in in the next week or so, so that #21483 has a few weeks to be worked on before beta, although I'll be offline most of next week so we'll see if any further iterations are needed.

This ticket was mentioned in IRC in #wordpress-dev by celloexpressions. View the logs.


5 years ago

#12 @johnbillion
5 years ago

The one remaining issue with 29572.2.diff is that esc_html() doesn't double-encode a string, but double braces in an Underscore template will. This comes to light with Twenty Fifteen which escapes its control label in PHP:

'label' => esc_html__( 'Header & Sidebar Background Color', 'twentyfifteen' )

The control label also gets escaped using double braces in the JS template, so the ampersand gets double-encoded and ends up looking like Header &amp; Sidebar Background Color. In the PHP controls, esc_html() is used and doesn't result in double-escaping.

We can fix this in Twenty Fifteen, but many other plugins and themes also escape their control labels.

I think the solution is to explicitly decode data.label and data.description before they're passed to the control template. This will prevent double encoding.

@celloexpressions: thoughts?

#13 @celloexpressions
5 years ago

I think that would work, although I wouldn't know exactly how to implement that, so someone else can tweak the patch.

This ticket was mentioned in IRC in #wordpress-dev by celloexpressions. View the logs.


5 years ago

#15 @johnbillion
5 years ago

In 30014:

Add the ability for a customizer control to render its controls via a JavaScript template. Switches the default color picker control to a JavaScript template. See #29572. Props celloexpressions

@johnbillion
5 years ago

#16 @johnbillion
5 years ago

  • Type changed from feature request to enhancement

29572.3.diff addresses the remaining double-escaping issue by decoding entities in the label and description before they're passed to the control's JSON attributes.

#17 @celloexpressions
5 years ago

  • Keywords dev-feedback removed

29572.3.diff looks good to me.

#18 @adamsilverstein
5 years ago

29572.4.diff:

  • Corrects some jshint errors introduced in r30014

#19 @SergeyBiryukov
5 years ago

In 30024:

Fix JSHint errors introduced in [30014].

props adamsilverstein.
see #29572.

#20 @ocean90
5 years ago

It seems like this brokes some stuff, see #30124, #30125, #29980.

#21 @celloexpressions
5 years ago

#30124 was marked as a duplicate.

#22 @celloexpressions
5 years ago

#30124 was a known issue, which 29572.3.diff should fix. I think the break of the patch on #29980 will probably be addressed by #30125.

The handling of the default color in the color control needs some tweaking, we'll use #30125 for that.

This ticket was mentioned in Slack in #core by westonruter. View the logs.


5 years ago

#24 @westonruter
5 years ago

  • Keywords commit added

In 29572.5.diff:

  • Supply ENT_QUOTES and blog charset as encoding param to html_entity_decode().
  • Factor out duplicated html_entity_decode() calls to new wp_decode_entities() function.
  • Include ENT_HTML5 in $quote_style if defined (PHP>=5.4).
  • Add unit tests for new wp_decode_entities function.

celloexpressions has given an LGTM for these changes.

This ticket was mentioned in Slack in #core by ocean90. View the logs.


5 years ago

@ocean90
5 years ago

#26 follow-up: @ocean90
5 years ago

Is there a reason why we just use the double braces? I don't see a reason for not supporting HTML in the control description, like to add a link for example. It would be consistent with the PHP templates.

#27 in reply to: ↑ 26 @westonruter
5 years ago

Replying to ocean90:

Is there a reason why we just use the double braces? I don't see a reason for not supporting HTML in the control description, like to add a link for example. It would be consistent with the PHP templates.

+1. Makes sense.

#28 @ocean90
5 years ago

  • Owner set to ocean90
  • Resolution set to fixed
  • Status changed from new to closed

In 30326:

Customizer: Prevent double-encoding in WP_Customize_Control.

Control's label and description may include HTML.

fixes #29572.

#29 @westonruter
4 years ago

In 36776:

Customize: Fix PHP notice when calling WP_Customize_Control::json() inside content_template() method.

A temp control is instantiated when WP_Customize_Manager:: render_control_templates() is called. This control needs to explicitly specify an empty settings array to avoid trying to use a temp setting which won't exist.

See #35926.
See #29572.

Note: See TracTickets for help on using tickets.