Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#39101 closed defect (bug) (fixed)

Customize: edit shortcuts for custom menu widgets do not work

Reported by: celloexpressions's profile celloexpressions Owned by: westonruter's profile westonruter
Milestone: 4.7.1 Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: has-patch commit fixed-major
Focuses: Cc:

Description

Custom menu widgets contain associated menu partials, which also get shortcuts. This has a few issues:

  • There are two shortcut icons, one for the menu and one for the widget.
  • The menu shortcut doesn't work. Menu partials look for the associated menu location control, but menus displayed via widgets are not in a menu location.
  • Depending on the theme, if the widget does not have a title the menu shortcut will likely be displayed on top of the widget shortcut. Since the menu shortcut doesn't work and the widget shortcut is essentially hidden, the shortcuts appear broken and essentially do nothing (unless you navigate to the widget shortcut with the keyboard).

We should either prevent the menu within a custom menu widget from getting an edit shortcut, or hide that icon with CSS. The widget shortcut is more appropriate in that the widget is the "location" of the menu, and the widget control links to edit the menu similarly to the menu location controls.

Attachments (3)

39101.diff (751 bytes) - added by sirbrillig 7 years ago.
Prevent adding edit shortcut for nav menus inside widgets
39101.2.diff (866 bytes) - added by sirbrillig 7 years ago.
Add inline comment
39101.3.diff (2.1 KB) - added by westonruter 7 years ago.

Download all attachments as: .zip

Change History (18)

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


7 years ago

#2 @celloexpressions
7 years ago

  • Owner set to sirbrillig
  • Status changed from new to assigned

#3 @sanket.parmar
7 years ago

Hi @celloexpressions ,

What if we don't load an edit icon for custom menu widgets. I mean on https://core.trac.wordpress.org/browser/tags/4.7/src/wp-includes/class-wp-customize-nav-menus.php#L1298 line we add one extra condition to check fallback_cb is not empty ( i.e., ! empty( $args->customize_preview_nav_menus_args['fallback_cb'] )). This way the edit icon will not load for custom menu widgets.

Let me know your thoughts.

@sirbrillig
7 years ago

Prevent adding edit shortcut for nav menus inside widgets

#4 @sirbrillig
7 years ago

  • Keywords has-patch added; needs-patch removed

@sanket.parmar That sounds like it would indeed work! But I think it might also prevent custom menu widgets from using partials at all? (I may be wrong.)

I attached a javascript patch which should prevent nav menus within widgets from getting their own shortcuts. Please let me know if it works for you!

Last edited 7 years ago by sirbrillig (previous) (diff)

#5 @celloexpressions
7 years ago

  • Keywords commit added
  • Owner changed from sirbrillig to westonruter
  • Status changed from assigned to reviewing

39101.diff looks good and works for me. Might want to add an inline comment explaining that it's disabled because the associated widget partial should take precedent, otherwise should be ready for commit.

@sirbrillig
7 years ago

Add inline comment

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


7 years ago

@westonruter
7 years ago

#7 @westonruter
7 years ago

@sirbrillig @celloexpressions I found the underlying problem was that the showControl logic was only accounting for wp_nav_menu() instances that referenced a given theme_location. The Custom Menu widgets never have a theme_location but rather instead refer to a given menu by ID directly. So the fix is just to add support for menu if theme_location is not set. This will also fix the edit shortcuts for any wp_nav_menu() instances that use menu in places other than the Custom Menu widget. See 39101.3.diff.

This new patch also fixes an issue where showControl would end up focusing on the last control that it found to be associated with a given setting. In this new patch, it will first sort the controls by their priority to then focus on the one that hast the priority with the smallest number, and thus the controls that appear at the top. For nav menus this ensures that the focus is given to the nav menu name control as opposed to the last checkbox control in the nav menu section.

Please confirm and I'll go ahead and commit.

#8 @sirbrillig
7 years ago

@westonruter nice sleuthing! Confirmed that that fixes the bug, in that it allows the edit shortcut for menus in widgets to work again.

That said (and your patch should be committed regardless), I wonder which is the better experience for users?

Clicking on the (fixed) menu shortcut brings up the editor for that menu.

https://cldup.com/7lTAzaHguo.png

Clicking on the widget shortcut brings up the widget settings, which allow changing which menu is displayed there.

https://cldup.com/mWvyLgbTc1.png

It seems to me that both could be what a user might be looking for, but obviously we can only allow one. Since you can click the "Edit Menu" button to get from the widget settings to the menu settings and not the other way around, I suggest we hide the menu shortcut anyway, but I'm open to a dissenting opinion.

#9 @westonruter
7 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 39622:

Customize: Fix visible edit shortcuts for wp_nav_menu() instances using the menu arg (such as in the Custom Menu widget) instead of the theme_location arg.

Also fix logic for focus-control-for-setting handler to focus on the first control (lowest priority value) associated with a given setting instead of the last control encountered when iterating over all controls, as this ensures the first control in a nav_menu section is focused rather than the last one.

Props westonruter, sirbrillig.
See #27403.
Fixes #39101.

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


7 years ago

#11 @westonruter
7 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for 4.7.1

@sirbrillig Good questions you're raising. I don't think it particularly hurts that there is two shortcuts for the two separate entities. On that note, one thing I am noticing is that the edit shortcut isn't appearing for the wp_nav_menu() instance when first adding the Custom Widget to the page. Likewise, the edit shortcut goes away for the wp_nav_menu() after making a change to the Custom Menu widget. So there seems to be an issue with nested partials and edit shortcuts.

#12 @sirbrillig
7 years ago

Created #39352 for the multiple-shortcuts question and #39353 for the missing shortcut bug.

#13 @dd32
7 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 39653:

Customize: Fix visible edit shortcuts for wp_nav_menu() instances using the menu arg (such as in the Custom Menu widget) instead of the theme_location arg.

Also fix logic for focus-control-for-setting handler to focus on the first control (lowest priority value) associated with a given setting instead of the last control encountered when iterating over all controls, as this ensures the first control in a nav_menu section is focused rather than the last one.

Props westonruter, sirbrillig.
See #27403.
Merges [39622] to the 4.7 branch.
Fixes #39101.

#14 @westonruter
7 years ago

In 40055:

Customize: Ensure edit shortcuts get re-created for nested partials when a parent partial is refreshed.

Fixes issue where the edit shortcut for a nav menu gets dropped when the containing Custom Menu widget is updated.

See #39101.
Fixes #39353.

#15 @dd32
7 years ago

In 40097:

Customize: Ensure edit shortcuts get re-created for nested partials when a parent partial is refreshed.

Fixes issue where the edit shortcut for a nav menu gets dropped when the containing Custom Menu widget is updated.

Props westonruter.
Merges [40055] to the 4.7 branch.
See #39101.
Fixes #39353.

Note: See TracTickets for help on using tickets.