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

Widget-id value used to build form field ids isn't consistent with hidden widget-id field #28668

Closed
manooweb opened this issue Feb 2, 2021 · 13 comments · Fixed by #31485
Closed
Assignees
Labels
[Block] Legacy Widget Affects the Legacy Widget Block - used for displaying Classic Widgets Needs Dev Ready for, and needs developer efforts [Status] In Progress Tracking issues with work in progress

Comments

@manooweb
Copy link
Contributor

manooweb commented Feb 2, 2021

Description

When I add our legacy widget as a new legacy widget in any widget area its form field ids and names are based on the WP_Widget::$id_base and WP_Widget::$number used by calling WP_Wigdet::get_field_id() or WP_Widget::get_field_name() method or directly using the WP_Widget::$id property.
See https://github.com/polylang/polylang/blob/master/include/widget-languages.php#L102-L137

In the new widget screen, for our legacy widget fields I obtained something like that:

<input type="checkbox" class="checkbox pll-dropdown" id="widget-polylang--1-dropdown" name="widget-polylang[-1][dropdown]">
<label for="widget-polylang--1-dropdown">Displays as a dropdown</label>

Notice the --1 for the id and widget-polylang[-1][dropdown] for the name of the field in the code above.

The new widget screen also adds some hidden fields for each widget. The rendering is the responsability of the LegacyWidgetEditDomManager component
See https://github.com/WordPress/gutenberg/blob/v9.8.3/packages/edit-widgets/src/blocks/legacy-widget/edit/dom-manager.js#L100-L131

At the end when the widget is added into the widget area these fields are;

<input type="hidden" name="widget-id" class="widget-id" value="polylang-4">
<input type="hidden" name="id_base" class="id_base" value="polylang">
<input type="hidden" name="widget_number" class="widget_number" value="-4">
<input type="hidden" name="multi_number" class="multi_number" value="">
<input type="hidden" name="add_new" class="add_new" value="">

Notice that the widget-id and widget_number fields aren't consistent with ones are used in the widget form fields.

This issue creates another one on our side because we have some js code which manipulate the fields to prevent the user to make wrong choice.

Step-by-step reproduction instructions

  1. Go to 'Appearance > Widgets '
  2. Click on '+' in the
  3. Right click on the title widget field and then Inspect
  4. See the HTML code espacially id and name attribute of the input tag
  5. See the HTML code of hidden fields some lines below the previous one.

Expected behaviour

Exactly the same as the legacy widget screen wp-admin/widgets.php

Each field need to be consistent with the widget id_base and number properties.
For example in the old screen:

<input type="checkbox" class="checkbox pll-dropdown" id="widget-polylang-6-dropdown" name="widget-polylang[6][dropdown]">
<label for="widget-polylang-6-dropdown">Displays as a dropdown</label>

and

<input type="hidden" name="widget-id" class="widget-id" value="polylang-6">
<input type="hidden" name="id_base" class="id_base" value="polylang">
<input type="hidden" name="widget-width" class="widget-width" value="250">
<input type="hidden" name="widget-height" class="widget-height" value="200">
<input type="hidden" name="widget_number" class="widget_number" value="5">
<input type="hidden" name="multi_number" class="multi_number" value="6">
<input type="hidden" name="add_new" class="add_new" value="">

Going further

One part of the issue is due to the REST API controller which retrieves the legacy widget to add it in the widget area.
See https://github.com/WordPress/gutenberg/blob/v9.8.3/lib/class-wp-rest-widget-types-controller.php#L408

Notice I've not debugged the React part of the issue yet especially on how the widget id_base and number/instance props is retrieved.
However I can surely say that it is what creates an issue in our legacy widget. Indeed I modified manually the HTML code in the browser and clicked on its options. All worked as expected including the correct refreshing of the React component. Save the widgets screen by clicking the Udpate button also works fine. I can verify by using the legacy widget screen.
It seems there is no problem to manipulate the widget fields with javascript code due to the LegacyWidgetEditHandler component implementation which listen any changes in the widget form, if I understood well.

WordPress information

  • WordPress version: 5.6.2
  • Gutenberg version: 9.8.3
  • Are all plugins except Gutenberg deactivated? No our plugin is also activated
  • Are you using a default theme (e.g. Twenty Twenty-One)? Yes: 2020

Device information

  • Device: Desktop
  • Operating system: Windows 10 Pro
  • Browser: Chrome 89.0
@gwwar gwwar added Needs Testing Needs further testing to be confirmed. [Block] Legacy Widget Affects the Legacy Widget Block - used for displaying Classic Widgets labels Feb 2, 2021
@manooweb manooweb changed the title [Feature widget screen] widget-id value used to build form field ids isn't consistent with hidden widget-id field [Feature widgets screen] widget-id value used to build form field ids isn't consistent with hidden widget-id field Feb 3, 2021
@draganescu draganescu changed the title [Feature widgets screen] widget-id value used to build form field ids isn't consistent with hidden widget-id field Widget-id value used to build form field ids isn't consistent with hidden widget-id field Feb 18, 2021
@draganescu
Copy link
Contributor

Following @noisysocks 's merge of the updates to the REST API widget endpoints in #29649 it's still the case that we get a bad value like widget-sow-button[-1][text] while in the old screen we get widget-sow-button[7][text] (tested with SiteOrigin widgets pack).

However it seems that when we add the widget in the old editor, the hidden inputs in the new editor are correct and the names of the form fields are also correct. Also inspecting the save widget call the response features a correct number and base

Debugging this I noticed that the correct number is used for everything if the widget is having any form field edited. E.g.

  • inserting a Site Origin image block and saving, does not return an error, but upon refresh the widget is now gone
  • inserting a Site Origin image block, adding an image title and saving, shows when inspected widget-sow-image[-1][title], but on refresh, the widget appears and it shows widget-sow-image[3][title] for the title input, which is correct.

@noisysocks is this something that can be addressed as part of the refresh of the Legacy Widget block in #29960 ?

@noisysocks noisysocks added Needs Dev Ready for, and needs developer efforts and removed Needs Testing Needs further testing to be confirmed. labels Mar 26, 2021
@noisysocks
Copy link
Member

I believe that this issue remains even after #29960 although all of the IDs now use 1 instead of -1.

It's a little tricky to solve this because we don't know what the correct number is until the widget is saved which, in the new screen, happens after the widget is added. Still, maybe we can "guess" what the number will be.

@manooweb
Copy link
Contributor Author

I believe that this issue remains even after #29960 although all of the IDs now use 1 instead of -1.

Hello @noisysocks, what do you mean ?
Simply set widget id to 1 in get_widget_form() method correct the issue ? But I didn't see this fix in the PR #29960 you mentionned.

Or we need the PR #29960 will be merged to test again ?

Thanks

@draganescu
Copy link
Contributor

How does the customizer work? I can add widgets there and they get the correct number and the form fields are correct without the widgets being saved or published. Can't we do something similar?

@noisysocks
Copy link
Member

@manooweb: What I mean is that this bug still exists 🙂 It's in the backlog waiting for a developer to pick up.

@draganescu: That's a good question. The Customizer can serve as inspiration here.

@manooweb
Copy link
Contributor Author

@noisysocks

@manooweb: What I mean is that this bug still exists 🙂 It's in the backlog waiting for a developer to pick up.

I would like but I was still busy with our code in our side. In addition as I said before I need some guidance to correctly understand how the part of Gutenberg code works to save time especially first where to look for to show how the widget_id is managed in the legacy widget block UI.

Thanks.

@manooweb
Copy link
Contributor Author

I created a github repository with a complete example to make the issue easier to test
https://github.com/manooweb/gutenberg-legacy-widget-test

I could start to explore Gutenberg code.

@manooweb
Copy link
Contributor Author

manooweb commented Apr 12, 2021

I created a github repository with a complete example to make the issue easier to test
https://github.com/manooweb/gutenberg-legacy-widget-test

I could start to explore Gutenberg code.

🤔 My example works fine with Gutenberg 10.3.2 but not with the trunk.
We've lost the hidden fields I mentioned in the description of the issue

<input type="hidden" name="widget-id" class="widget-id" value="gutenberg-11">
<input type="hidden" name="id_base" class="id_base" value="gutenberg">
<input type="hidden" name="widget_number" class="widget_number" value="11">
<input type="hidden" name="multi_number" class="multi_number" value="">
<input type="hidden" name="add_new" class="add_new" value="">

So the script can't find anymore the checkbox fields because there are based on the widget-id field value.

Is there something I missed ? Is there something has been changed since the 10.3.x release ?

@manooweb
Copy link
Contributor Author

manooweb commented Apr 12, 2021

I created a github repository with a complete example to make the issue easier to test
https://github.com/manooweb/gutenberg-legacy-widget-test
I could start to explore Gutenberg code.

🤔 My example works fine with Gutenberg 10.3.2 but not with the trunk.
We've lost the hidden fields I mentioned in the description of the issue

<input type="hidden" name="widget-id" class="widget-id" value="gutenberg-11">
<input type="hidden" name="id_base" class="id_base" value="gutenberg">
<input type="hidden" name="widget_number" class="widget_number" value="11">
<input type="hidden" name="multi_number" class="multi_number" value="">
<input type="hidden" name="add_new" class="add_new" value="">

So the script can't find anymore the checkbox fields because there are based on the widget-id field value.

Is there something I missed ? Is there something has been changed since the 10.3.x release ?

I think we've lost something since the #29960 iteration
What it was done here https://github.com/WordPress/gutenberg/blob/v10.3.2/packages/edit-widgets/src/blocks/legacy-widget/edit/dom-manager.js#L100-L131
seems to no longer exist.
Or I didn't see by what it has been replaced 🤔
https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/legacy-widget/edit/form.js#L23-L155

@manooweb
Copy link
Contributor Author

manooweb commented Apr 14, 2021

I created a github repository with a complete example to make the issue easier to test
https://github.com/manooweb/gutenberg-legacy-widget-test
I could start to explore Gutenberg code.

🤔 My example works fine with Gutenberg 10.3.2 but not with the trunk.
We've lost the hidden fields I mentioned in the description of the issue

<input type="hidden" name="widget-id" class="widget-id" value="gutenberg-11">
<input type="hidden" name="id_base" class="id_base" value="gutenberg">
<input type="hidden" name="widget_number" class="widget_number" value="11">
<input type="hidden" name="multi_number" class="multi_number" value="">
<input type="hidden" name="add_new" class="add_new" value="">

So the script can't find anymore the checkbox fields because there are based on the widget-id field value.
Is there something I missed ? Is there something has been changed since the 10.3.x release ?

I think we've lost something since the #29960 iteration
What it was done here https://github.com/WordPress/gutenberg/blob/v10.3.2/packages/edit-widgets/src/blocks/legacy-widget/edit/dom-manager.js#L100-L131
seems to no longer exist.
Or I didn't see by what it has been replaced 🤔
https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/legacy-widget/edit/form.js#L23-L155

This PR #30709 fixes the issue of hidden widget fields.

However a new issue appears now.
With an existing widget the widget-id and widget_number don't correspond on the other fields ids normally based on widget-id and widget_number

Steps to reproduce

  1. Add the Legacy Gutenberg Test Widget in the legacy widgets screen
  2. See the HTML code of the widget in this screen and the 2 hidden fields are consistent with all the other fields
  3. Go to the new block-based widgets screen
  4. See the HTML code of the widget: the widget-id and widget_number aren't consistent with all the other fields

Screenshot of the code in the old screen

image

Screenshot of the code in the new screen

image

@noisysocks
Copy link
Member

Thanks for testing @manooweb. I'll work on a fix shortly.

@greenlevel
Copy link

Hi, I see this issue is closed.

But I also noticed the difference in id from the hidden formfield when editing the widget compared to the id seen when actually rendering the widget. Any feedback or update on this?

Thanks in advance!

@jaworskimatt
Copy link

I still have the same problem in WordPress 6.1 - the filter gives me widget-1 in the form, but in the front-end it's widget-2. Either the fix didn't work or the problem has resurfaced.

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 Needs Dev Ready for, and needs developer efforts [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants