WordPress.org

Make WordPress Core

Opened 14 months ago

Closed 14 months ago

Last modified 13 months ago

#27767 closed defect (bug) (fixed)

Customizer: Widget settings are lost when activating a previewed theme

Reported by: ocean90 Owned by: nacin
Milestone: 3.9 Priority: highest omg bbq
Severity: blocker Version: 3.9
Component: Customize Keywords: has-patch commit
Focuses: Cc:

Description

Steps to reproduce:

  • Go to wp-admin/themes.php and click "Live Preview" for a theme
  • Change the header image
  • Now add a widget to a sidebar
  • Click "Save & Activate"
  • Go to front end and you will notice that the header is changed, but the widgets are lost

Attachments (11)

27767-01.patch (4.1 KB) - added by gcorne 14 months ago.
27767-02.patch (4.3 KB) - added by gcorne 14 months ago.
27767-03.patch (5.1 KB) - added by westonruter 14 months ago.
Prevent getWidgetFormControls() from returning any undefined values when no form controls are found. https://github.com/x-team/wordpress-develop/compare/trac-27767
27767-04.patch (5.5 KB) - added by gcorne 14 months ago.
27767-05.patch (5.8 KB) - added by gcorne 14 months ago.
27767-06.patch (460 bytes) - added by gcorne 14 months ago.
27767-07.patch (8.6 KB) - added by westonruter 14 months ago.
Changes from previous patches: https://github.com/x-team/wordpress-develop/commit/44e75cc9e0cd35ec5dcebcc62f420c5e5b53f095 https://github.com/x-team/wordpress-develop/pull/10 Cumulative changes: https://github.com/x-team/wordpress-develop/pull/10/files
27767-08.patch (9.3 KB) - added by gcorne 14 months ago.
27767-09.patch (9.9 KB) - added by westonruter 14 months ago.
Apply some formatting fixes requested ocean90 in https://github.com/x-team/wordpress-develop/pull/10
27767-10.patch (10.0 KB) - added by westonruter 14 months ago.
https://github.com/x-team/wordpress-develop/commit/af03194179956fdaa054b01db555baa4caf34d98
27767-11.patch (10.8 KB) - added by westonruter 14 months ago.
https://github.com/x-team/wordpress-develop/commit/58d5a11f1807ea723af7ffa3434908972754afb1?w=1

Download all attachments as: .zip

Change History (40)

comment:1 @ircbot14 months ago

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

comment:2 @ocean9014 months ago

  • Summary changed from Customizer: Widgets settings are lost when activating a previewed theme to Customizer: Widget settings are lost when activating a previewed theme

comment:3 @westonruter14 months ago

Likely due to Widget Customizer not incorporating logic from #17979 (Avoid losing widgets when switching themes)

comment:4 @nacin14 months ago

  • Severity changed from critical to blocker

comment:5 follow-up: @nacin14 months ago

Customizing widgets before activating a theme sounds like one of two primary use cases of widgets in the customizer. What am I missing here? Do customizing widgets just not work if you're doing "Save & Activate"?

comment:6 in reply to: ↑ 5 @westonruter14 months ago

Replying to nacin:

Customizing widgets before activating a theme sounds like one of two primary use cases of widgets in the customizer. What am I missing here? Do customizing widgets just not work if you're doing "Save & Activate"?

When switching themes, changes made to widgets in the customizer will be lost when you click "Save & Update". But actually, the widget instances themselves get saved properly. The problem is specifically with the widget-sidebar associations. Those are what get lost.

If you go in to the _wp_sidebars_changed() function and comment out retrieve_widgets(true) then the newly-added widgets and new widget sort orders get saved when switching themes. So there is something here when switching themes that is not accounting for changes made to widget areas in the customizer.

comment:7 @ircbot14 months ago

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

@gcorne14 months ago

@gcorne14 months ago

comment:8 @gcorne14 months ago

Since @DH-Shredder asked me to guess at how much effort was involved, I started digging in a bit and came up with an idea for how to fix the issues. 27767-02.patch seems to do it, although it definitely needs testing, review, and docs. The basic idea is:

  1. When loading the customizer with a theme that is not active, we need to retrieve_widgets(), but without any writes occurring, so I added another argument that is used to control whether writes occur. This gets tricky because we also need to do the same when loading the iframe the first time, so the patch relies on the fact that customize_messenger_channel will be preview-0 the first time.
  1. When saving the customizer and activating a theme, we need to capture that the action happened via the customizer so that we don't apply retrieve_widgets() again. I think the ideal is for this data to be incorporated into the theme_switched option to avoid races.

@westonruter14 months ago

Prevent getWidgetFormControls() from returning any undefined values when no form controls are found. https://github.com/x-team/wordpress-develop/compare/trac-27767

comment:9 @westonruter14 months ago

I found that when I tried to click to add a new widget to a sidebar, I got a JS error due to sidebar.getWidgetFormControls() not being able to find the widget form controls, and returning undefined when it can't. I updated the patch to not ever return undefined, but this doesn't address the underlying problem. As soon as I added a new widget to the sidebar, all of a sudden a bunch of other widgets all of a sudden appeared in the sidebar, but they appeared faded-out indicating that they aren't shown in the preview.

comment:10 @ocean9014 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

Tested 27767-03.patch and it works so far. I agree to gcorne , that we have to add some docs here. Will do also some more tests.

wp_get_sidebars_widgets () has a filter sidebars_widgets, I think for consistency we should have it in get_sidebars_widgets() too.

@gcorne14 months ago

comment:11 @gcorne14 months ago

So, I am still learning my way around some of this code, but I think that there are three possible causes for sidebar.getWidgetFormControls() not being able to find the widget form controls:

  1. There is a case where sidebars_widgets needs to be overridden based on retrieve_widgets( true, false ) and it isn't. This could be either because it needs to be overridden earlier in the execution or that the conditions that control when the overriding occurs is missing a situation.
  1. There is a scenario where sidebars_widgets is being overridden based on (retrieve_widgets( true, false ) when the global sidebar_widgets has already been passed through retrieve_widgets.
  1. There is a scenario where sidebars_widgets is being overridden, but not in the context of the new theme causing a mismatch.

Because of the layers of filtering involved, getting this right is a bit tricky. 27767-04.patch tries to solve #2 and #3, which I think are the most likely causes. I also included a fix to customize-widgets.js to fix JQMIGRATE: $(html) HTML strings must start with '<' character. I left out the fix that @westonruter came up with for the javascript error so that the symptom of the problem is not suppressed.

More testing/review is definitely needed.

@gcorne14 months ago

comment:12 @gcorne14 months ago

27767-05.patch is a little more robust than 27767-04.patch.

comment:13 follow-up: @gcorne14 months ago

While testing, I discovered an issue with previewing twentyeleven and the widget "Twenty Eleven Ephemera." I can add the widget just fine, but making changes to the widget doesn't seem to work. All I get is a spinner. This is true both with and without 27767-05.patch applied, so when previewing a theme that registers widgets and interacting with those widgets, the widget customizer doesn't seem to work quite work right.

comment:14 @DH-Shredder14 months ago

Notice (with traceback included) when previewing Twenty Fourteen and adding the Twenty Fourteen Ephemera Widget:
http://screen.getsource.net/04-12-2014-13-15-25.png

comment:15 @nacin14 months ago

Another angle: How bad does this break when the current theme doesn't support widgets?

comment:16 @nacin14 months ago

If we can't get this fixed we'll either need to pull out the widget customizer or delay 3.9. I don't want to do either. DH-Shredder is going to try to coordinate this for the rest of the day. This is the highest priority ticket for 3.9 at the moment. Let's push through it.

comment:17 in reply to: ↑ 13 @gcorne14 months ago

Replying to gcorne:

While testing, I discovered an issue with previewing twentyeleven and the widget "Twenty Eleven Ephemera." I can add the widget just fine, but making changes to the widget doesn't seem to work

This is caused by the ajax calls with action == 'update-widget' not including the theme parameter, so the customizer doesn't load in preview mode. Unfortunately, making that change exposes issues with the other work that I had done to override the sidebars_widgets to apply retrieve_widgets() under certain circumstances. Figuring out how to best handle that requires more thought.

@gcorne14 months ago

comment:18 follow-up: @westonruter14 months ago

I believe I've nailed down the problem. In the latest patch 27767-07.patch:

  • Run retrieve_widgets() earlier, and only when initially opening customizer in a theme switch. (The results of this function get passed along subsequently by Customizer POSTs.)
  • Likewise, initially filter sidebars_widgets option to be the retrieved widgets. This restores any widget area configuration previously used on the previewed theme.
  • Store the old theme's sidebars_widgets in a new customizer setting which gets supplied into the sidebars_widgets theme mod after theme activation.

comment:19 in reply to: ↑ 18 @gcorne14 months ago

Replying to westonruter:

I believe I've nailed down the problem. In the latest patch 27767-07.patch

Everything is working well for me. Much better than my attempt to get sidebars_widgets initialized correctly. :)

One minor quibble...I am not sure that old_sidebars_widgets quite communicates what is being stored there. Some other ideas are preexisting_sidebars_widgets, current_sidebars_widgets or maybe active_theme_sidebars_widgets.

comment:20 follow-up: @ocean9014 months ago

Found an issue with 27767-07.patch: After saving & activating the theme the widgets are in the right place, but after visiting wp-admin/widgets.php (where retrieve_widgets() is called too) the widget is moved to another sidebar, see screencast https://cloudup.com/cCQwwH0gVPQ.

@gcorne14 months ago

comment:21 in reply to: ↑ 20 ; follow-up: @gcorne14 months ago

Replying to ocean90:

Found an issue with 27767-07.patch: After saving & activating the theme the widgets are in the right place, but after visiting wp-admin/widgets.php (where retrieve_widgets() is called too) the widget is moved to another sidebar, see screencast https://cloudup.com/cCQwwH0gVPQ.

27767-08.patch fixes this.

comment:22 in reply to: ↑ 21 @westonruter14 months ago

Replying to gcorne:

Replying to ocean90:

Found an issue with 27767-07.patch: After saving & activating the theme the widgets are in the right place, but after visiting wp-admin/widgets.php (where retrieve_widgets() is called too) the widget is moved to another sidebar, see screencast https://cloudup.com/cCQwwH0gVPQ.

27767-08.patch fixes this.

Good one! Your most recent change to remove_theme_mod( 'sidebars_widgets' ) was what I was also working on. This also fixes the problem for me.

@westonruter14 months ago

Apply some formatting fixes requested ocean90 in https://github.com/x-team/wordpress-develop/pull/10

comment:23 @ocean9014 months ago

  • Keywords commit added; needs-testing removed

27767-09.patch passes all my previous failed tests. Good job.

comment:24 @ircbot14 months ago

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

comment:25 @westonruter14 months ago

In 27767-10.patch use wp_loaded instead of widgets_init action.

comment:26 @westonruter14 months ago

In 27767-11.patch use WP_Customize_Manager::doing_ajax() instead of other less-elegant checks.

comment:27 @nacin14 months ago

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

In 28124:

Customizer: Properly handle widget settings when activating a previewed theme.

props westonruter, ocean90, gcorne.
fixes #27767.

comment:28 @ircbot13 months ago

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

comment:29 @westonruter13 months ago

The resulting commit r28124 did not fix the issue, due to a typo where the logic in retrieve_widgets('customize') should have been inverted. See #27897.

Note: See TracTickets for help on using tickets.