Make WordPress Core

Opened 9 years ago

Last modified 2 years ago

#36212 accepted defect (bug)

Empty menu items are deleted without warning

Reported by: cameronjonesweb's profile cameronjonesweb Owned by: audrasjb's profile audrasjb
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Menus Keywords: has-patch has-screenshots
Focuses: ui, administration Cc:

Description

I recently discovered that menu items with with an empty label are deleted from the menu upon save without any warning or notice. The particular situation was that I was adding a menu with an icon font for the items, so the labels were empty but had classes. Obviously a HTML comment can be used in this instance, but there should be some way to either communicate with the user that items with an empty label will be removed, disable saving of a menu with empty labels, or some way to handle empty labels without deleting the item.

Attachments (11)

36212.diff (715 bytes) - added by farookibrahim 7 years ago.
6ce75a0d8883e74384a12eb87c1a2304.gif (433.9 KB) - added by audrasjb 5 years ago.
Before the patch: blank menu item removed
c58d1de87b52fe7e5faea0375d5bb2d9.gif (346.1 KB) - added by audrasjb 5 years ago.
After the patch: blank menu item remains but its label is not blank anymore
36212.patch (2.2 KB) - added by samful 5 years ago.
admin-view-menu-editor-with-no-titles.png (16.2 KB) - added by samful 5 years ago.
front-end-menu-with-symbols-in-empty-menu-items.png (36.1 KB) - added by samful 5 years ago.
36212.2.diff (2.8 KB) - added by samful 4 years ago.
refeshed with correct src/ path and got rid of some unused variables
36212.3.diff (2.8 KB) - added by samful 4 years ago.
refreshed for development version (5.5-alpha-20200717.105532)
36212.4.diff (654 bytes) - added by maxpertici 4 years ago.
Keep blank label with a non-breaking space
Capture d’écran 2020-09-19 à 22.37.07.png (194.8 KB) - added by audrasjb 4 years ago.
36212.4.diff
36212.5.diff (763 bytes) - added by audrasjb 4 years ago.
Menus: Replace blank nav menu item titles with a non-breaking space

Download all attachments as: .zip

Change History (30)

#1 @obenland
9 years ago

  • Version trunk deleted

#2 @welcher
8 years ago

  • Keywords needs-patch added

Thanks for contributing to core! Would you like to propose a solution with a patch?

@farookibrahim
7 years ago

#3 @farookibrahim
7 years ago

  • Keywords has-patch added; needs-patch removed

#4 @audrasjb
6 years ago

  • Keywords needs-refresh added
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to audrasjb
  • Status changed from new to accepted

The patch doesn't applies cleanly anymore. Will need a refresh.

Moving to Future Realse as the solution proposed in the patch looks fine on my side.

#5 @audrasjb
5 years ago

  • Milestone changed from Future Release to 5.5

@audrasjb
5 years ago

Before the patch: blank menu item removed

@audrasjb
5 years ago

After the patch: blank menu item remains but its label is not blank anymore

#6 @audrasjb
5 years ago

The proposed patch (needs refresh) works partially. The menu item remains, but its title is not blank, it takes the object’s original title. I think it will also need to make some changes in wp_setup_nav_menu_item in wp-includes/nav-menu.php.

#7 @samful
5 years ago

  • Keywords needs-testing added; needs-refresh removed

@audrasjb had the right advice here, so thanks for pointing me in the right direction. This patch should solve the issue you were having.

In my testing, the patch seemed to work perfectly, but I also want to note that after testing, I found that the theme "Twenty Nineteen" (My other installed themes had no issues) has a CSS rule which hides any empty <a> tags. So, to get round this I made a quick CSS rule to display empty <a> elements again just for Twenty Nineteen. You may want to include this in your custom css if you are using Twenty Nineteen, or edit it to something similar if using another theme that hides empty <a> elements:

.main-navigation .sub-menu > li > a:empty{
	display: block;
}

I then went on to test what @cameronjonesweb was initially trying to do (put an icon font or a symbol in the menu item):

.main-navigation .sub-menu > li > a:empty:after {
	content: "\00a9";
}

I'll also attach screenshots of the admin view where I edited the menu items and the front end where the empty items have the copyright symbol \00a9. This is just an example of how it can be done using the current patch and the CSS above on the "Twenty Nineteen" theme.

If someone else could give this patch a test too, that would be very helpful :)

@samful
5 years ago

@samful
4 years ago

refeshed with correct src/ path and got rid of some unused variables

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


4 years ago

#9 @whyisjake
4 years ago

  • Keywords needs-refresh added

The patch is not applying cleanly, can we get an update?

@samful
4 years ago

refreshed for development version (5.5-alpha-20200717.105532)

#10 @samful
4 years ago

  • Keywords needs-refresh removed

thank you for following up on this @whyisjake

#11 @SergeyBiryukov
4 years ago

  • Milestone changed from 5.5 to 5.6

Looks like this removes some code for handling empty titles. Even if removing is the correct approach, that code was originally added for a reason, and I don't feel comfortable making this change late in the release cycle without a proper review of the history of these lines. Moving to 5.6 for now.

@maxpertici
4 years ago

Keep blank label with a non-breaking space

#12 @maxpertici
4 years ago

While reading the ticket, if we want to avoid removing too much code, I thought of this "solution". Can we say that it works? (Patch 36212.4.diff)

@audrasjb
4 years ago

Menus: Replace blank nav menu item titles with a non-breaking space

#13 @audrasjb
4 years ago

  • Keywords commit has-screenshots added; needs-testing removed

This is a great alternative @maxpertici, thanks. Works fine on my side (see screenshot bellow).
I’d only change the comment wording, see 36212.5.diff.

Markin this for commit as the last proposed patch is pretty straightforward and doesn't introduce any big change.

#14 @desrosj
4 years ago

I may be missing it, but what was the reason for removing the "(no title)" in favor of a non-breaking space? That matches how posts without a title are handled elsewhere in the admin ( "(no title)" is displayed ).

#15 @SergeyBiryukov
4 years ago

  • Keywords commit removed

Haven't tried to reproduce the issue yet, but if the original problem we're trying to address here is that items marked as "(no title)" are removed on saving, then it seems like the outcome of this ticket should be to keep these items as is, without removing them or forcing them to have a title.

Changing "(no title)" to something else seems more like a workaround rather than an actual solution.

It could be that 36212.3.diff is on the right path, it just needs someone to review the history of the affected code, see if the concerns it aims to address are still relevant, and whether it can be safely removed now without any backward compatibility breaks or other unintentional effects.

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


4 years ago

#17 @hellofromTonya
4 years ago

  • Milestone changed from 5.6 to Future Release

In discussing with Sergey at today's scrub, we'll moving this ticket to Future Release as it needs more time per his comments here.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

#18 @audrasjb
2 years ago

  • Milestone changed from Future Release to 6.1

Moving for 6.1 consideration.

#19 @audrasjb
2 years ago

  • Milestone changed from 6.1 to Future Release

Unfortunately, as this ticket didn't get enough momentum from milestoning to 6.1, let's put it back to Future Release milestone.

Note: See TracTickets for help on using tickets.