WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#32800 closed defect (bug) (duplicate)

Customizer Menus: Creating/saving a new menu creates duplicate menus in Custom Menu widget select

Reported by: tywayne Owned by:
Milestone: Priority: high
Severity: normal Version: 4.3
Component: Customize Keywords: needs-patch
Focuses: javascript Cc:

Description

Steps to reproduce:

  1. Create a new menu in the Customizer
  2. Save & Publish
  3. Navigate to Widgets in Customizer
  4. Add a Custom Menu widget
  5. Dropdown for menu selection will include duplicates of menus

gif walkthrough of those steps here: https://cloudup.com/cxN8AnPZx3j

Attachments (3)

32800.diff (497 bytes) - added by adamsilverstein 5 years ago.
Remove duplicate menu item insert
32800.2.diff (877 bytes) - added by adamsilverstein 5 years ago.
first step for adding menus on the fly to custom menu widgets
32800.3.diff (963 bytes) - added by adamsilverstein 5 years ago.

Download all attachments as: .zip

Change History (22)

#1 @celloexpressions
5 years ago

  • Focuses javascript added
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.3

Reproduced. Probably a reasonably simple fix one someone finds where the code is.

@adamsilverstein
5 years ago

Remove duplicate menu item insert

@adamsilverstein
5 years ago

first step for adding menus on the fly to custom menu widgets

#2 follow-up: @adamsilverstein
5 years ago

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

In 32800.diff:

  • Remove a single line that is inserting the duplicate options into the widget template - this fixes the issue mentioned on the ticket
  • This patch is ready to go and can be committed.

32800.2.diff is an incomplete attempt to insert newly created menus to the custom menu widget template on the fly. With this patch, if you a new menu (without saving), then switch to the widgets and insert the custom menu widget, the new menu will be available in the dropdown. However, the menu id is the placeholderId, and needs updating during save/publish. Since this a different issue, we can probably commit 32800.diff and create a new ticket to address this issue.

#3 follow-up: @adamsilverstein
5 years ago

Opened #32814 to cover the enhancement covered in 32800.2.diff. 32800.diff is ready to go and solves the issue on this ticket.

Last edited 5 years ago by adamsilverstein (previous) (diff)

#4 in reply to: ↑ 2 ; follow-up: @stevenkword
5 years ago

Replying to adamsilverstein:

In 32800.diff:

  • Remove a single line that is inserting the duplicate options into the widget template - this fixes the issue mentioned on the ticket
  • This patch is ready to go and can be committed.

Patch applies, but I'm seeing a new issue as a result. Following the same reproduction steps WITHOUT Save & Publish, the Custom Menu Widget is not aware of newly created Menus.

Steps to reproduce:

1.) Create a new menu in the Customizer
2.) Navigate to Widgets in Customizer
3.) Add a Custom Menu widget

"No menus have been created yet. Create some."

#5 in reply to: ↑ 3 ; follow-up: @tywayne
5 years ago

  • Keywords has-patch commit removed

Replying to adamsilverstein:

Opened #32814 to cover the enhancement covered in 32800.2.diff. 32800.diff is ready to go and solves the issue on this ticket.

Patch does not fix the issue completely. It does keep it from being added to new Custom Menu Widgets, but if you already had a Custom Menu widget in the Widget Area, it still appends a duplicate. - https://cloudup.com/ca0VyaOMdcZ

#6 in reply to: ↑ 4 @adamsilverstein
5 years ago

I see this issue as well, and working to address it in #32814. We need to use the temporary id until the user saves, then swap that with the real ID on save and publish.

Replying to stevenkword:

Replying to adamsilverstein:

In 32800.diff:

  • Remove a single line that is inserting the duplicate options into the widget template - this fixes the issue mentioned on the ticket
  • This patch is ready to go and can be committed.

Patch applies, but I'm seeing a new issue as a result. Following the same reproduction steps WITHOUT Save & Publish, the Custom Menu Widget is not aware of newly created Menus.

Steps to reproduce:

1.) Create a new menu in the Customizer
2.) Navigate to Widgets in Customizer
3.) Add a Custom Menu widget

"No menus have been created yet. Create some."

#7 in reply to: ↑ 5 @adamsilverstein
5 years ago

Hmmm, thought I tested that. I will revisit and try to fix.

So new steps to reproduce, right?

  1. Create a new menu in the Customizer
  2. Save & Publish
  3. Navigate to Widgets in Customizer
  4. Open an existing Custom Menu widget
  5. Dropdown for menu selection will include duplicates of menus

Replying to tywayne:

Replying to adamsilverstein:

Opened #32814 to cover the enhancement covered in 32800.2.diff. 32800.diff is ready to go and solves the issue on this ticket.

Patch does not fix the issue completely. It does keep it from being added to new Custom Menu Widgets, but if you already had a Custom Menu widget in the Widget Area, it still appends a duplicate. - https://cloudup.com/ca0VyaOMdcZ

#8 follow-up: @adamsilverstein
5 years ago

@tywayne To clarify: did you test with 32800.diff? Thats the one meant to resolve this issue, the .2 patch was the start of further work and can be safely ignored. I'll retest with the first patch.

#9 in reply to: ↑ 8 ; follow-up: @tywayne
5 years ago

Replying to adamsilverstein:

@tywayne To clarify: did you test with 32800.diff? Thats the one meant to resolve this issue, the .2 patch was the start of further work and can be safely ignored. I'll retest with the first patch.

Correct, tested with 32800.diff - patch resolves issue for newly added Custom Menu widgets, but still duplicates on existing widgets for me.

#10 in reply to: ↑ 9 @adamsilverstein
5 years ago

  • Keywords has-patch added

Replying to tywayne:

Replying to adamsilverstein:

@tywayne To clarify: did you test with 32800.diff? Thats the one meant to resolve this issue, the .2 patch was the start of further work and can be safely ignored. I'll retest with the first patch.

Correct, tested with 32800.diff - patch resolves issue for newly added Custom Menu widgets, but still duplicates on existing widgets for me.

Thanks for the feedback and testing.

This should be fixed in 32800.3.diff, can you please retest?

The remaining issue I see is that if you create a new menu and DON'T save and publish, then try to select it in a custom menu widget, the selection is not saved correctly. Working on that issue in #32814.

#11 @tywayne
5 years ago

Tested and confirmed that .3 fixes the duplication bug presented in this ticket.

I'll watch and help test #32814 as needed as well. Looks like there is a @todo at the bottom of api.Menus.applySavedData that could be removed with a fix of #32814.

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


5 years ago

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


5 years ago

#14 @jorbin
5 years ago

  • Priority changed from normal to high

@westonruter @ adamsilverstein Is this blocked by #32814 or should it go in now?

#15 @westonruter
5 years ago

@jorbin I think this is bound up with #32814, yes. The patch in its current form here will not have the desired results if the user had selected the new nav menu in the Custom Menu widget, since the menu gets removed without regard to whether it was selected or not.

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


5 years ago

#17 follow-up: @celloexpressions
5 years ago

  • Keywords needs-patch added; has-patch removed

Should we close this in favor of #32814, which seems to encapsulate this fix, or keep them separate?

#18 in reply to: ↑ 17 @adamsilverstein
5 years ago

Replying to celloexpressions:

Should we close this in favor of #32814, which seems to encapsulate this fix, or keep them separate?

I think you can close as the fix on #32814 (when completed)‚ will include this fix.

#19 @westonruter
5 years ago

  • Milestone 4.3 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #32814.

Note: See TracTickets for help on using tickets.