Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#34893 closed enhancement (fixed)

Improve Customizer setting validation model

Reported by: westonruter's profile westonruter Owned by: westonruter's profile westonruter
Milestone: 4.6 Priority: high
Severity: normal Version: 3.4
Component: Customize Keywords: has-patch has-screenshots has-unit-tests commit
Focuses: javascript Cc:

Description (last modified by westonruter)

Customizer setting values need to be able to be validated and for any validation errors to block the Customizer from saving any setting until all are valid.

Settings in the Customizer rely on sanitization to ensure that only valid values get persisted to the database. The sanitization in the Customizer generally allows values to be passed through to be persisted and does not enforce validation that blocks the saving of the value. This is in large part because there is no standard facility for showing error messages relating to Customizer controls, and there is no standard method to raise validation errors. A Customizer setting can be blocked from being saved by returning null from the WP_Customize_Setting::sanitize() method (i.e. generally returned via customize_sanitize_{$setting_id}). When this is done, however, the modified value completely disappears from the preview with no indication for why the value seems to be reset to the default.

In JavaScript there is the wp.customize.Setting.prototype.validate() method that can be extended to return null in the case where the value should be rejected, but again this does not provide a way to display a validation message: the user can be entering data but they just stop seeing the value making changes in the preview without any feedback. Even worse, if the JS validate method actually manipulates the value to make it valid, there can be strange behavior in the UI as the user provides input, likely having to do with the two-way data binding of wp.customize.Element instances.

Furthermore, if one setting is blocked from being saved by means of validation in the sanitization method, the other settings in the Customizer state may very well be completely valid, and thus they would save successfully. The result is that some settings would get saved, whereas others would not, and the user wouldn't know which were successful and which failed (again, since there is no standard mechanism for showing validation error message). The Customizer state would only partially get persisted to the database. This isn't good.

We need to improve the Customizer by:

  • Validating settings on server before save.
  • Displaying validation error messages from server and from JS client.
  • Performing transactional/atomic setting saving, rejecting all settings if one is invalid.

Since the WP_Customize_Setting::sanitize() already partially supports validation by returning null, we can build on that to also allow it to return a WP_Error object. We'd want to retain the regular fuzzy sanitization during preview updates, but during the customize_save action we'd want to impose a strict pass/fail for setting validation.

This is closely related and complimentary to #30937: Add Customizer transactions

For the notification bar to display validation error messages, see #35210.

Feature plugin: https://github.com/xwp/wp-customize-setting-validation

Attachments (23)

34893.0.diff (1.9 KB) - added by westonruter 8 years ago.
POC
screenshot-1.png (40.2 KB) - added by westonruter 8 years ago.
Invalid site title
screenshot-2.png (60.6 KB) - added by westonruter 8 years ago.
Invalid widget title
screenshot-3.png (51.8 KB) - added by westonruter 8 years ago.
Invalid nav menu item title
34893.1.diff (5.0 KB) - added by westonruter 8 years ago.
Additional changes: https://github.com/xwp/wordpress-develop/compare/04bc5da...e6052b1 PR: https://github.com/xwp/wordpress-develop/pull/136
34893.2.diff (24.7 KB) - added by westonruter 8 years ago.
multiple-invalidity-notifications.png (28.1 KB) - added by westonruter 8 years ago.
Multiple notifications are displayed at once via bullet list
notifications-below-control.jpg (147.7 KB) - added by dlh 8 years ago.
Instance of an "invisible" notification
34893.3.diff (31.5 KB) - added by westonruter 8 years ago.
Add QUnit tests
34893.4.diff.png (17.2 KB) - added by celloexpressions 8 years ago.
34893.4.diff (31.2 KB) - added by celloexpressions 8 years ago.
CSS tweaks.
34893.5.diff (31.8 KB) - added by westonruter 8 years ago.
Prepend instead of append notifications container: https://github.com/xwp/wordpress-develop/compare/3f060ed...3af6b10
34893.6.diff (19.8 KB) - added by mrahmadawais 8 years ago.
Widget Notification CSS Border
34893.7.diff (32.0 KB) - added by westonruter 8 years ago.
https://github.com/xwp/wordpress-develop/pull/136/commits/0235decc780b1b155f1cfa8c1897f93ac70e18a6
data-attributes.png (165.3 KB) - added by westonruter 8 years ago.
34893.8.diff (32.1 KB) - added by westonruter 8 years ago.
https://github.com/xwp/wordpress-develop/pull/136/commits/2d7ea4bc23fe3b1ceaa7a69ca40857df80cd8f62
34893.9.diff (32.1 KB) - added by westonruter 8 years ago.
Use number_format_i18n(): https://github.com/xwp/wordpress-develop/pull/136/commits/81549ca484dcf649409d4ee3c1ae7b97497f8f81
34893.10.diff (32.4 KB) - added by westonruter 8 years ago.
Re-use existing notice styles and support notice-alt for widget controls https://github.com/xwp/wordpress-develop/pull/136/commits/77241302401ff5f74f5e07e060b18dfe1f15c423
34893.11.diff (32.4 KB) - added by celloexpressions 8 years ago.
Improve error styling for control containers and fix notice spacing for widgets, menus, and multiple vs. single notifications.
34893.11.diff.png (27.9 KB) - added by celloexpressions 8 years ago.
34893.12.diff (33.6 KB) - added by westonruter 8 years ago.
https://github.com/xwp/wordpress-develop/pull/136/commits/f86adb313cbcfa6817d62b0f270b4d0517f0af79
34893.13.diff (39.3 KB) - added by westonruter 8 years ago.
https://github.com/xwp/wordpress-develop/pull/136/commits/f75bada95f30f9fcf85c2540942351cb9c91123d
34893.14.diff (38.8 KB) - added by celloexpressions 8 years ago.
CSS adjustments per discussion in #design on Slack.

Download all attachments as: .zip

Change History (93)

#1 @westonruter
8 years ago

  • Keywords has-patch needs-testing added; needs-patch removed
  • Milestone changed from Future Release to 4.5
  • Owner set to westonruter
  • Status changed from new to accepted

I have implemented this in a feature plugin called “Customize Setting Validation”.

To do server-side validation of a setting, implement a setting sanitizer that returns null or a WP_Error object:

<?php
$wp_customize->add_setting( 'year_established', array(
        'type' => 'option',
        'sanitize_callback' => function ( $value ) {
                $value = intval( $value );

                // Apply strict validation when the sanitize callback is called during.customize_validate_settings.
                if ( doing_action( 'customize_validate_settings' ) && ( $value < 1900 || $value > 2100 ) ) {
                        return new WP_Error( 'invalid_value', __( 'Year must be between 1900 and 2100.' ) );
                }

                $value = min( 2100, max( $value, 1900 ) );
                return $value;
        }
) );

By returning null, the Customizer will display a generic error message. By returning WP_Error, the specific message can be provided.

The validation error message can also be set programmatically by JS by calling control.validationMessage.set(),
for example from an extended control.setting.validate() method. The validationMessage is inspired by HTML5.

For a demonstration of the functionality made possible with this Customizer setting validation API,
including how to do client-side validation, see the “Customize_Validate_Entitled_Settings” plugin. It will validate that the Site Name (blogname), nav menu item titles, and widget titles are all fully populated. The validation is done both on the client and on the server.

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


8 years ago

#3 @westonruter
8 years ago

  • Description modified (diff)

@westonruter
8 years ago

POC

#4 @westonruter
8 years ago

Instead of overloading the Customizer setting's sanitize_callback to do both sanitization and validation, for a Core patch the WP_Customizer_Setting should really also implement a validate_callback, just as has been done for the REST API (see WP_REST_Request::has_valid_params()). There can be a default validate_callback that checks if the sanitize_callback returns null and if so, then it can return false. Otherwise, it can return true. The REST API also supports the validate_callback returning a WP_Error, so our validate_callback in WP_Customize_Setting can do the same, something like 34893.0.diff.

Note the addition of a new $strict argument to the customize_sanitize filter which allows the filter to do strict/validation checking. I'm not 100% sold on this, but it would be nice if there could be some way to more easily integrate the sanitization/validation logic.

Maybe the REST API folks have some insights here :). The goal here is to keep the Customizer API as close to the REST API, so that Customizer settings can be just added to the REST API and it will just work.

/cc @rmccue @rachelbaker @danielbachhuber @joehoyle

#5 follow-up: @joehoyle
8 years ago

I agree on wanting to split the validate / sanitization. Validate can return true / false and optionally a WP_Error to provide extra validation feedback to the user. sanitize is used to mutate the value to the correct datatype, be it a safe string, bool, int etc.

I'd imagine you could just call the validate_callback in isolation also, for example, checking an inputs validity on keyup / blur.

@rmccue and myself have discussed for the REST API on Save, whether the validate callback should be called on the value pre or post sanitize. I think there's merit to both, depending on the use case, we never came to a hard decision on that, but core currently calls sanitize before validate. This means the validator function can deal with "clean" data, and is concerned with specific validity rather than type checks. For this way round, the example of posts_per_page is a good one: sanitize_callback calls absint on the data passed via POST, and the validate_callback would then > 0 && < 100.

@westonruter
8 years ago

Invalid site title

@westonruter
8 years ago

Invalid widget title

@westonruter
8 years ago

Invalid nav menu item title

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

Replying to joehoyle:

@rmccue and myself have discussed for the REST API on Save, whether the validate callback should be called on the value pre or post sanitize. I think there's merit to both, depending on the use case, we never came to a hard decision on that, but core currently calls sanitize before validate. This means the validator function can deal with "clean" data, and is concerned with specific validity rather than type checks. For this way round, the example of posts_per_page is a good one: sanitize_callback calls absint on the data passed via POST, and the validate_callback would then > 0 && < 100.

What about if the original raw unsanitized value is passed into the validator along with the sanitized value? This would allow the validator to have the flexibility to determine whether or not the sanitized value can be considered “good enough” for persisting to the DB, or else it can do a strict check to fail it simply if $sanitized_value !== $unsanitized_value.

In that regard, the patch for this ticket could be modified to pass in both the $sanitized_value and the $unsanitized_value to the customize_validate_{$setting_id} filter. I've amended my patch to implement that: 34893.1.diff. It also adds:

  • Adds a $strict second parameter to WP_Customize_Setting::save(), which is a signal that WP_Error can be returned
  • Validate all settings before the customize_save action, and return with a JSON error with an array of all the invalid settings if any are invalid.

For the REST API, however, it doesn't look like the original/raw/unsanitized value would still be available for the validate callback to look at?

Last edited 8 years ago by westonruter (previous) (diff)

#7 @sc0ttkclark
8 years ago

I'm keeping an eye on this for use in the Fields API, I could see this as super useful there as well, albeit the usage would be different outside of the Customizer -- I'd love to see what it could allow for.

#8 @westonruter
8 years ago

Another improvement is needed on the JS side of things. Namely, there is currently a wp.customize.Setting.prototype.validate() method allows you to sanitize a value and also to block the setting update if you pass back null to block the Setting (really, the Value). So really, the validate method in JS is serving in the same purpose as the sanitize method in PHP. They both conflate sanitization and validation.

I suggest that we extend wp.customize.Setting to introduce a sanitize method for the purpose of sanitization, and that the existing validate method be extended to allow for the returning of an Error object, which would have the same effect for blocking the value but it could also then emit an event on the Setting object, which could then be picked up by any controls to then display a validation error message.

Last edited 8 years ago by westonruter (previous) (diff)

#9 @westonruter
8 years ago

See demo video of the feature plugin: https://www.youtube.com/watch?v=ZNk6FhtS8TM

#10 @westonruter
8 years ago

  • Description modified (diff)

This ticket was mentioned in Slack in #core-fields by sc0ttkclark. View the logs.


8 years ago

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


8 years ago

#13 @westonruter
8 years ago

  • Description modified (diff)

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


8 years ago

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


8 years ago

#16 @westonruter
8 years ago

  • Milestone changed from 4.5 to Future Release

Punting this for next release since notification area isn't ready (#35210). This will make sense to go in as part of Customizer transactions (#30937).

#17 @westonruter
8 years ago

  • Keywords 4.6-early added

#18 @westonruter
8 years ago

  • Milestone changed from Future Release to 4.6

This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.


8 years ago

#20 @westonruter
8 years ago

  • Keywords 4.6-early removed

The error message that appears in the control should use the same interface as the global notifications. For example, they should both have instances of a NotificationCollection. See https://core.trac.wordpress.org/ticket/35210#comment:60

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


8 years ago

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


8 years ago

@westonruter
8 years ago

#24 @westonruter
8 years ago

  • Keywords needs-unit-tests added

Please see 34893.2.diff.

  • Introduces wp.customize.Notification which can represent WP_Error instances returned from the server.
  • Introduces wp.customize.Control.prototype.notifications.
  • Introduces wp.customize.Setting.prototype.notifications.
  • Injects a notification area into existing controls which is populated in response to the control's notifications collection changing, where this collection is synced with the notifications collections for each of its settings.
  • Attempting to save settings that are invalid will result in the save being blocked entirely, with the errors being sent in the customize_save_response for the controls to display notifications for, and the first such invalid control will be focused.
  • Introduces wp.customize.settingConstructor, allowing custom setting types to be used in the same way that custom controls, panels, and sections can be made.
  • Introduces WP_Customize_Setting::validate(), WP_Customize_Setting::$validate_callback, and the customize_validate_{$setting_id} filter.
  • Modifies WP_Customize_Manager::save() to check all settings for validity issues prior to calling their save methods.
  • Introduces WP_Customize_Setting::json() for parity with the other Customizer classes. This includes exporting of the type.

Needs more unit tests.

#25 @westonruter
8 years ago

I’ve updated the sample extension plugin (Customize Validate Entitled Settings) to make use of this patch: https://gist.github.com/westonruter/1016332b18ee7946dec3

#26 @dlh
8 years ago

@westonruter,

Why is the notification area placed at the bottom of the control container when there is no .customize-control-title, as opposed to the top?

#27 @westonruter
8 years ago

@dlh good question. I suppose the reason is that if there is no .customize-control-title element in a control, then perhaps the control is using a different element to serve as the title and it would make sense to prevent the notification from being injected before any such alternate title? Or if there is a control that just consists of a checkbox and label, without any title, then it wouldn't make sense for the notification appear above the title-less control?

@westonruter
8 years ago

Multiple notifications are displayed at once via bullet list

#28 @westonruter
8 years ago

  • Keywords has-screenshots added

#29 @dlh
8 years ago

@westonruter Those are fair considerations. I'm not sure whether my reasons in favor of displaying the notifications at the top of the container overcome them, but here they are:

  1. The notifications are not visible when the control is taller than the available space (see screenshot).
  2. Users might consider it inconsistent for notifications to appear above inputs sometimes and below inputs other times (but that is only speculation).

@dlh
8 years ago

Instance of an "invisible" notification

@westonruter
8 years ago

Add QUnit tests

#30 follow-up: @westonruter
8 years ago

@dlh thanks for demonstrating that. But it could be that the notification could be obscured at the top as well for such a long control, right? In such cases, perhaps it should just be left up to the custom control to just override the getNotificationsContainerElement to place the container at the appropriate spot?

#31 in reply to: ↑ 30 ; follow-up: @dlh
8 years ago

But it could be that the notification could be obscured at the top as well for such a long control, right?

True, except (I think) if you are in a different Customizer section when you save. In that case, it looks like you are brought back to top of the control with the notification at the bottom.

In such cases, perhaps it should just be left up to the custom control to just override the getNotificationsContainerElement to place the container at the appropriate spot?

Yeah, that sounds like it could also be a straightforward approach.

@celloexpressions
8 years ago

CSS tweaks.

#32 follow-up: @celloexpressions
8 years ago

  • Priority changed from normal to high

34893.4.diff cleans up the CSS a bit, see slightly-revised screenshot. One part that needs particular testing - the former width: 98% for all customizer inputs is revised to 100% so that they will align with other elements (such as media controls) and notifications.

Other thoughts:

  • I would be in favor of injecting at the top of the control when there isn't a title unless the custom control overrides the element.
  • Selective refresh seems to revert to the previous setting value when a setting is invalid. Perhaps we should do this but also keep it faded/in an updating state to indicate that there is a problem with the current setting (hence the previous value being used)?
  • We will want to move the notification out of the <label> element, but let's wait to do that until we do a general cleanup of the label structure in the customizer, since that will require a broader approach.
  • I think we could probably use the class .has-notifications on .customize-control instead of having both .customize-control and .customize-control-has-notifications.
  • The per-control/setting notifications look pretty good. In terms of an overall notifications area, I think it would be helpful in terms of usability to aggregate all instances of errors into a single place that included autofocusing links to each of the controls with errors. I'll make a mockup of that on #35210.

#33 @westonruter
8 years ago

@celloexpressions @valendesigns any feedback on the latest patch? I have the code in a PR for inline commenting as well: https://github.com/xwp/wordpress-develop/pull/136

I want to commit this today so we can move on to transactions.

#34 @westonruter
8 years ago

@celloexpressions thanks for the feedback! I missed that you commented just before me :-)

#35 @mrahmadawais
8 years ago

@westonruter I just tested the latest patch and here's what I think

  • In example plugin, the error being reported is wrong, it should read You must supply a Navigation Label instead of the title which begs the question of building a custom notification renderer which could get back the fieldname with WP_Error instance.
  • CSS Spacing issues are evident almost everywhere with the error notification. I can contribute on that part but probably it has more to do with how notifications are being rendered. Check the images attached below.
  • Would it be better if we can designate a certain area for notifications at the top so that folks building custom controls won't mess up the notifications

Check the red line to visually differentiate the odd spacing issues.
https://i.imgur.com/PNlFzzz.png
https://i.imgur.com/JECaqhZ.png
https://i.imgur.com/80fIU8b.png

#36 follow-up: @celloexpressions
8 years ago

@mrahmadawais the CSS spacing should be correct for the site title control, but I didn't check the others. The intent is for 8px above and below the notification, including any margins on adjacent elements.

#37 in reply to: ↑ 36 @mrahmadawais
8 years ago

Replying to celloexpressions:

@mrahmadawais the CSS spacing should be correct for the site title control, but I didn't check the others. The intent is for 8px above and below the notification, including any margins on adjacent elements.

I get that, but it doesn't play well with other conditions, as you can see in the comment above.

@westonruter Here's another suggestion.

  • Error notification inside the widgets area is not looking good. The white background of the notification is getting mixed up with the white background of widget accordion. This can be solved by moved the error message to the top of the chain i.e. before the div with class widget-top and inside the div with class widget. It would look something like this

Before
https://i.imgur.com/kDjFdos.png

After
https://i.imgur.com/1q93IGD.png

#38 @westonruter
8 years ago

@mrahmadawais:

Would it be better if we can designate a certain area for notifications at the top so that folks building custom controls won't mess up the notifications

For custom controls, developers can take two approaches to displaying notifications:

1) They can include a element with a customize-control-notifications-container class in the control's content.
2) Or they can override the controlgetNotificationsContainerElement() method to change how the notification gets injected.

Error notification inside the widgets area is not looking good. The white background of the notification is getting mixed up with the white background of widget accordion. This can be solved by moved the error message to the top of the chain i.e. before the div with class widget-top and inside the div with class widget. It would look something like this

I think the style should be modified rather than moving the notification outside of the control's collapsed contents. If it were outside, this would be confusing when users expect to see a list of controls not a mixture of a list of controls and validation messages.

@celloexpressions I tried re-using the default error styles in my default div.customize-control-notifications-container. But I think we should stop trying to do that. Instead of adding error to that root container element, I think the error should be added to the li itself, and so individual notifications could be errors where others could be updates or informational messages.

@westonruter
8 years ago

Prepend instead of append notifications container: https://github.com/xwp/wordpress-develop/compare/3f060ed...3af6b10

#39 in reply to: ↑ 31 @westonruter
8 years ago

Replying to dlh:

But it could be that the notification could be obscured at the top as well for such a long control, right?

True, except (I think) if you are in a different Customizer section when you save. In that case, it looks like you are brought back to top of the control with the notification at the bottom.

And replying to celloexpressions:

I would be in favor of injecting at the top of the control when there isn't a title unless the custom control overrides the element.

Done in 34893.5.diff

#40 @mrahmadawais
8 years ago

@westonruter You are right. Maybe we could just add #eee borders to the notification. I just submitted a patch and now the notification in the widgets area looks better.

BEFORE
https://i.imgur.com/kDjFdos.png

AFTER: 34893.6.diff
https://i.imgur.com/i4RNKzR.png

Last edited 8 years ago by mrahmadawais (previous) (diff)

@mrahmadawais
8 years ago

Widget Notification CSS Border

#41 @westonruter
8 years ago

@mrahmadawais that does look better, but I notice that the border on top/bottom is causing the border on the left to be cut diagonally. Perhaps a box shadow would have a better result?

Also sorry but I uploaded a new patch 34893.7.diff that didn't include your change. I moved the error class to be an data- attribute of the list item itself. This meant that I needed to change the list styles to be compact when multiple are displayed. See data-attributes.png.

@celloexpressions thoughts on this change? I leave the CSS up to you :)

Also, it would be easier to collaborate if you guys could push commits to the existing PR: https://github.com/xwp/wordpress-develop/pull/136 And this would avoid us having to manually reconcile and merge patches. You both have write access to that repo now.

#42 in reply to: ↑ 32 ; follow-up: @westonruter
8 years ago

Replying to celloexpressions:

  • I think we could probably use the class .has-notifications on .customize-control instead of having both .customize-control and .customize-control-has-notifications.

I've done this in the latest patches.

  • Selective refresh seems to revert to the previous setting value when a setting is invalid. Perhaps we should do this but also keep it faded/in an updating state to indicate that there is a problem with the current setting (hence the previous value being used)?

Yeah, the revert to the previous setting is a result of the invalid settings now being (properly) rejected, and so they are treated as if they weren't supplied at all. This would be true in both selective refresh and full-page refresh. At the moment, it would be up to the control to display the notification using JS to give feedback for why an update isn't appearing, as currently PHP-supplied error notifications are currently only being sent back upon attempted Save. However, with transactions (#30937) each change to a setting would get submitted to the server via a PATCH request, and so this is where I think we'd need to ultimately provide this feedback for why a setting change isn't appearing in the preview. I don't know if we should display any invalidity state inside the preview itself.

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


8 years ago

#44 @westonruter
8 years ago

  • Keywords ux-feedback added

Needs design feedback.

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


8 years ago

#46 @westonruter
8 years ago

@afercia thoughts on accessibility considerations? It is using aria-live="assertive" for the notification container.

#47 @westonruter
8 years ago

Addressed accessibility feedback from @afercia in 34893.8.diff. Now using wp.a11y.speak() for notifications added instead of an assertive aria live region for the entire notifications container.

#48 @ocean90
8 years ago

34893.8.diff:
sprintf( _n( 'There is %d invalid setting.', 'There are %d invalid settings.', $invalid_count ), $invalid_count );
should be
sprintf( _n( 'There is %s invalid setting.', 'There are %s invalid settings.', $invalid_count ), number_format_i18n( $invalid_count ) );

@westonruter
8 years ago

Re-use existing notice styles and support notice-alt for widget controls https://github.com/xwp/wordpress-develop/pull/136/commits/77241302401ff5f74f5e07e060b18dfe1f15c423

@celloexpressions
8 years ago

Improve error styling for control containers and fix notice spacing for widgets, menus, and multiple vs. single notifications.

#49 @celloexpressions
8 years ago

  • Keywords ux-feedback removed

34893.11.diff incorporates design refinements as discussed in #design in slack, and also fixes spacing of notifications for various cases. Indicators are only shown on menu item/widget headings when there is an error inside it (not for warnings/info).

#50 @westonruter
8 years ago

34893.12.diff fleshes out php/js-doc; only remove error notifications at save; fix qunit tests by adding a11y dependency. Also remove incremented param count for customize_sanitize_{$setting_id} filter, since $strict is no longer being used. (Includes changes from 34893.11.diff.)

#51 @westonruter
8 years ago

  • Keywords commit has-unit-tests added; needs-testing needs-unit-tests removed

34893.13.diff adds unit tests for changes to WP_Customize_Manager, with the exception of WP_Customize_Manager::save() which currently lacks unit tests in core already. (New Ajax tests need to be written for it at some point.)

Suggesting commit. Any objections?

#52 @westonruter
8 years ago

  • Keywords needs-dev-note added

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


8 years ago

#54 @westonruter
8 years ago

  • Keywords ux-feedback added; commit removed

Removing commit pending design meeting.

This ticket was mentioned in Slack in #design by hugobaeta. View the logs.


8 years ago

#56 @celloexpressions
8 years ago

Ongoing discussion in #design - haven't found a good way to outline nav menu item controls. Should probably commit 34893.13.diff for now and we'll continue iterating on that.

#57 @westonruter
8 years ago

@celloexpressions Besides the design of the collapsed controls for nav menu items and widgets (which is for multi-field controls), I recall there as also a desire from design to have the individual inputs for single-field controls to get a red border around the inputs when invalid: https://wordpress.slack.com/archives/design/p1463679781001325

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


8 years ago

@celloexpressions
8 years ago

CSS adjustments per discussion in #design on Slack.

#59 @celloexpressions
8 years ago

  • Keywords commit added; ux-feedback removed

34893.14.diff includes the outline on text inputs with errors and replaces the left border with an outline on the header element (as an inset box-shadow) for menus and widgets. Should be ready for a first-pass commit. Additional design updates can happen as other approaches are developed.

#60 @westonruter
8 years ago

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

In 37476:

Customize: Add setting validation model and control notifications to augment setting sanitization.

When a setting is invalid, not only will it be blocked from being saved but all other settings will be blocked as well. This ensures that Customizer saves aren't partial but are more transactional. User will be displayed the error in a notification so that they can fix and re-attempt saving.

PHP changes:

  • Introduces WP_Customize_Setting::validate(), WP_Customize_Setting::$validate_callback, and the customize_validate_{$setting_id} filter.
  • Introduces WP_Customize_Manager::validate_setting_values() to do validation (and sanitization) for the setting values supplied, returning a list of WP_Error instances for invalid settings.
  • Attempting to save settings that are invalid will result in the save being blocked entirely, with the errors being sent in the customize_save_response. Modifies WP_Customize_Manager::save() to check all settings for validity issues prior to calling their save methods.
  • Introduces WP_Customize_Setting::json() for parity with the other Customizer classes. This includes exporting of the type.
  • Modifies WP_Customize_Manager::post_value() to apply validate after sanitize, and if validation fails, to return the $default.
  • Introduces customize_save_validation_before action which fires right before the validation checks are made prior to saving.

JS changes:

  • Introduces wp.customize.Notification in JS which to represent WP_Error instances returned from the server when setting validation fails.
  • Introduces wp.customize.Setting.prototype.notifications.
  • Introduces wp.customize.Control.prototype.notifications, which are synced with a control's settings' notifications.
  • Introduces wp.customize.Control.prototype.renderNotifications() to re-render a control's notifications in its notification area. This is called automatically when the notifications collection changes.
  • Introduces wp.customize.settingConstructor, allowing custom setting types to be used in the same way that custom controls, panels, and sections can be made.
  • Injects a notification area into existing controls which is populated in response to the control's notifications collection changing. A custom control can customize the placement of the notification area by overriding the new getNotificationsContainerElement method.
  • When a save fails due to setting invalidity, the invalidity errors will be added to the settings to then populate in the controls' notification areas, and the first such invalid control will be focused.

Props westonruter, celloexpressions, mrahmadawais.
See #35210.
See #30937.
Fixes #34893.

#61 in reply to: ↑ 42 @westonruter
8 years ago

Replying to westonruter:

  • Selective refresh seems to revert to the previous setting value when a setting is invalid. Perhaps we should do this but also keep it faded/in an updating state to indicate that there is a problem with the current setting (hence the previous value being used)?

Yeah, the revert to the previous setting is a result of the invalid settings now being (properly) rejected, and so they are treated as if they weren't supplied at all. This would be true in both selective refresh and full-page refresh. At the moment, it would be up to the control to display the notification using JS to give feedback for why an update isn't appearing, as currently PHP-supplied error notifications are currently only being sent back upon attempted Save. However, with transactions (#30937) each change to a setting would get submitted to the server via a PATCH request, and so this is where I think we'd need to ultimately provide this feedback for why a setting change isn't appearing in the preview. I don't know if we should display any invalidity state inside the preview itself.

Follow-up on this: #36944

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


8 years ago

#63 @peterwilsoncc
8 years ago

In 37582:

Customize: Run autoprefixer following [37476]

Adds prefixes to box-shadow and transition properties.

See #34893

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


8 years ago

#65 @westonruter
8 years ago

In 37867:

Customize: Always define functions reflowPaneContents, findControlsForSettings, and _handleSettingValidities on wp.customize.

Moves definitions of functions outside of a jQuery.ready() callback, as these functions needn't be deferred to DOM ready. This change also ensures that the functions are available if customize-controls is enqueued outside of the Customizer context, as the ready callback short-circuits if _wpCustomizeSettings is not defined. The findControlsForSettings and _handleSettingValidities functions were misplaced in r37700.

See #34893.
See #36944.
See #29071.

#66 @westonruter
8 years ago

In 37942:

Customize: Reverse order of setting sanitization/validation, validating prior to sanitizing.

Reverses order where sanitization was being applied before validation originally in accordance with REST API logic.

Props westonruter, schlessera.
See #34893.
See #37192.
Fixes #37247.

#67 @ocean90
8 years ago

  • Keywords needs-dev-note removed

#68 @westonruter
7 years ago

In 39145:

Customize: For Header Image, ensure "Previously uploaded" and "Suggested" headings are hidden when lists are empty.

Fixes regression introduced with the addition of control notifications in [37476]. The container element for notifications is injected after the .customize-control-title element if the .customize-control-notifications-container element does not already exist in the control's template. Also adds missing margin between current image and uploaded images.

Props bradyvercher.
See #34893.
Fixes #38561.

#69 @westonruter
7 years ago

In 41374:

Customize: Add global notifications area.

  • Displays an error notification in the global area when a save attempt is rejected due to invalid settings. An error notification is also displayed when saving fails due to a network error or server error.
  • Introduces wp.customize.Notifications subclass of wp.customize.Values to contain instances of wp.customize.Notification and manage their rendering into a container.
  • Exposes the global notification area as wp.customize.notifications collection instance.
  • Updates the notifications object on Control to use Notifications rather than Values and to re-use the rendering logic from the former. The old Control#renderNotifications method is deprecated.
  • Allows notifications to be dismissed by instantiating them with a dismissible property.
  • Allows wp.customize.Notification to be extended with custom templates and render functions.
  • Triggers a removed event on wp.customize.Values instances _after_ a value has been removed from the collection.

Props delawski, westonruter, karmatosed, celloexpressions, Fab1en, melchoyce, Kelderic, afercia, adamsilverstein.
See #34893, #39896.
Fixes #35210, #31582, #37727, #37269.

#70 @westonruter
7 years ago

In 41390:

Customize: Add notifications API to sections and panels.

  • Adds a notifications property to instances of wp.customize.Panel and wp.customize.Section.
  • Adds a setupNotifications() method to Panel, Section, and Control.
  • Adds a getNotificationsContainerElement() method to the Panel and Section classes, like Control has.
  • Replace hard-coded notification in header media section with a notification.
  • Limit rendering notifications to panels and sections that are expanded, and to controls that have an expanded section.

See #34893, #35210, #38778.
Fixes #38794.

Note: See TracTickets for help on using tickets.