Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Legacy widgets: Some widgets don't load all the scripts they need #22765

Closed
jorgefilipecosta opened this issue May 29, 2020 · 7 comments
Closed
Assignees
Labels
[Block] Legacy Widget Affects the Legacy Widget Block - used for displaying Classic Widgets [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets [Type] Bug An existing feature does not function as intended

Comments

@jorgefilipecosta
Copy link
Member

Describe the bug
Some widgets don't work as expected on legacy widgets block because the scripts they need are not loaded.

That happens because instead of using the widget specific functions to enqueue assents and widget may be using different methods:

  • Use a general admin enqueue method and then a condition that version if the screen matches certain criteria that the new widget screen may not match.
  • Use setup actions e.g:sidebar_admin_setup to enqueue things and not only setup.

I guess for some cases we may not be able to make things work and probably third party widgets will need a small change in their conditions/enqueue mechanism.
But we may try to emulate the old screen better and trick widgets into enqueuing their scrips.

I guess a method to improve here is install something like 15 very used widgets. Verify which ones don't work because their scripts are not enqueued.
Check the conditions on this widgets and see if we can make this contains be true with some "emulation".

To reproduce
Install https://wordpress.org/plugins/simple-image-widget/.
Verify the widget does not works as expected on legacy widgets because its scripts are not loaded.

@jorgefilipecosta jorgefilipecosta added [Type] Bug An existing feature does not function as intended Customization Issues related to Phase 2: Customization efforts [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Block] Legacy Widget Affects the Legacy Widget Block - used for displaying Classic Widgets [Package] Edit Widgets /packages/edit-widgets labels May 29, 2020
@jorgefilipecosta
Copy link
Member Author

Documenting here why Site Origin widget bundle https://wordpress.org/plugins/so-widgets-bundle/ does not load correctly on legacy widgets.
The scripts required by this widgets are correctly loaded. But they have some conditions on their initialization that are not met on the new widgets screen.
These problems may be common to other widgets so documenting them may be useful.

The widget initializes when a click to open the widget is made:

$('.widgets-holder-wrap').on('click', '.widget:has(.siteorigin-widget-form-main) .widget-top', function () {

On the legacy widgets, the click does not happen so the widget is never initialized.

The plugin has a different initialization for the customizer blocks

	var $body = $( 'body' );
	if ( $body.hasClass('wp-customizer') ) {
		// Setup new widgets when they're added in the customizer interface
		$(document).on('widget-added', function (e, widget) {

This initialization relies on the widget-added, on the customizer blocks the event happens so the widget works as expected.
If the widget was updated to remove the condition of having a wp-customizer classname on the body it would load correctly on the new widgets screen.

Fortunately, Site Origin widget bundle already implements a block similar to legacy widgets that allows to use their widgets in block areas and works well. The mechanism the block use can not replicate in the core because they are specific for the widgets the plugin contains.

I will continue to do more tests with third party widgets and document why things did not worked or improve legacy widgets and make things work.

@draganescu
Copy link
Contributor

This is blocked because it's not clear how much work should go into supporting non standard ways of "hooking" into WP admin.

@draganescu draganescu removed the Customization Issues related to Phase 2: Customization efforts label Feb 18, 2021
@noisysocks
Copy link
Member

Let's do this:

I guess a method to improve here is install something like 15 very used widgets. Verify which ones don't work because their scripts are not enqueued.
Check the conditions on this widgets and see if we can make this contains be true with some "emulation".

@draganescu
Copy link
Contributor

In current Gutenberg trunk it looks like the block editor embedded in the Customizer is more compatible than the standalone one. For instance:

  1. On trunk install the Site Origin widgets bundle and setup a theme with sidebars
  2. Go to Appearance > Widgets
  3. Add a legacy widget from site origin - e.g. the Button widget to any sidebar
  4. Notice a lot of the controls in the Site Orign Button Widget's form don't work
  5. Save
  6. Go to Appearance > Customize > Widgets > [your sidebar]
  7. Notice the controls of the Site Orign Button Widget's form work

How come that one is more compatible? Looks like this is automatically solved in the customizer screen, and we should / could copy however that works to the stand alone editor as well.

@noisysocks
Copy link
Member

noisysocks commented Apr 27, 2021

So it looks like what's happening is that SiteOrigin Widgets only listens for the widget-added event to do initialisation when the <body> has a wp-customizer class. The Legacy Widget block will correctly fire widget-added but <body> will not have wp-customizer unless we're in the Customizer.

This is not correct. Widgets should set up the widget when the widget-added event fires regardless of what admin page the user is on. This is what the core widgets do.

https://plugins.trac.wordpress.org/browser/so-widgets-bundle/trunk/base/js/admin.js#L1405

Still, I think we might be able to make Legacy Widget compatible with this code by faking the click event on .widget-inside. I'll try that and then spin up a PR if it works.

edit: Oops, just realised this is almost literally what @jorgefilipecosta wrote above 😅

@noisysocks
Copy link
Member

Still, I think we might be able to make Legacy Widget compatible with this code by faking the click event on .widget-inside. I'll try that and then spin up a PR if it works.

Hm, nope, this doesn't work because the plugin binds the click event on jQuery.ready and the React app has not rendered at that point.

I think that in this case it's not unreasonable to say the the plugin needs to be updated to remove the if ( $body.hasClass('wp-customizer') ) check so that it works on the new screen.

I'm warming to the argument that the Widgets screen should launch as an opt-in feature so that plugins have an opportunity to test and make these sorts of minor changes.

Please keep bringing up any compatibility issues you see and I will investigate!

@noisysocks noisysocks self-assigned this Apr 28, 2021
@noisysocks
Copy link
Member

I think let's close this overview/catch-all issue. The Legacy Widget block has been through a lot of iteration recently and backwards compatibility is definitely in a better state. See #31126 (comment). There are also now some docs that explain the caveats surrounding the widget-added event. See #31577.

If and when we find new backwards compatibility issues, let's open one new issue per problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Legacy Widget Affects the Legacy Widget Block - used for displaying Classic Widgets [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

3 participants