WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 6 months ago

Last modified 6 months ago

#38948 closed defect (bug) (fixed)

More difficult to create submenus in customizer

Reported by: adamsilverstein Owned by: westonruter
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: has-patch commit dev-reviewed
Focuses: javascript Cc:

Description

Some recent changes in the customizer have changed he behavior of the drag and drop menu editor in the customizer, making it more difficult to drag menus into the submenu position.


Previously it was possible to drag a menu item directly to the right and an outline indicating the submenu position would appear. dropping the menu left it as a submenu. 4.4 pictured here, also worked correctly in 4.6:

https://cl.ly/1a3l2c2N2C2r/Screen%20Recording%202016-11-26%20at%2002.30%20PM.gif



In trunk you have to drag the menu over to top of the menu above it to get it to drop into the submenu position:

https://cl.ly/0p3m3g181Z2B/Screen%20Recording%202016-11-26%20at%2002.29%20PM.gif


Possible related to CSS changes #34391, see Slack discussion.

Attachments (3)

38948_1.diff (1.7 KB) - added by delawski 6 months ago.
38948.diff (1.7 KB) - added by adamsilverstein 6 months ago.
38948_2.diff (1.9 KB) - added by delawski 6 months ago.

Download all attachments as: .zip

Change History (19)

#1 @westonruter
6 months ago

@delawski as this is likely related to #34391, would you be able to help work out a solution?

#2 @westonruter
6 months ago

  • Milestone changed from Awaiting Review to 4.7

#3 follow-up: @lukecavanagh
6 months ago

@adamsilverstein

Never found the customizer that easy for creating complex menus in.

#4 in reply to: ↑ 3 @adamsilverstein
6 months ago

Replying to lukecavanagh:

@adamsilverstein

Never found the customizer that easy for creating complex menus in.

Far enough, however it is now much more difficult. You can no longer drag an item to the right, instead you have to drag it right and then up over the previous item to get it to snap into the submenu position.

Part of the issue may be the code in nav-menus.js that determines the depth based on position relative to the draggable area container. After these css changes, the positioning for this container is offset 299 pixels to the right in my testing, instead of at position 0 as you would expect for absolute positioning left:0;

#5 @lukecavanagh
6 months ago

@adamsilverstein

But I do agree that is is harder to add sub-menu items in the customizer at the moment. I can see the same issue when editing the main menu on a local dev site.

#6 @adamsilverstein
6 months ago

@lukecavanagh -
this calculation of menuEdge is incorrect after the CSS changes in r38648: https://core.trac.wordpress.org/browser/branches/4.7/src/wp-admin/js/nav-menu.js#L619 - I think this is whats breaking the drag action. before the change this returns 0, after it returns 299

@delawski
6 months ago

#7 @delawski
6 months ago

  • Focuses javascript added
  • Keywords has-patch added; needs-patch removed

Thank you @adamsilverstein for investigating the issue. It helped me a lot to find out what was wrong and create a fix. You can also review it on the GitHub.

So the issue was that the initSortables() has been called before the nav menu section was actually expanded. Because of that incorrect offset().left value has been calculated (as noted by @adamsilverstein: instead of 0 it returned 299).

The initSortables() method call has been bound to the expandedSection event, so that the calculation is done after the section is fully expanded. It fixed the issue on my end.

#8 @adamsilverstein
6 months ago

@delawski Nice work here tracking down a solution! In my initial testing the new code didn’t completely fix the positioning, I think the code potentially fires while the element is still animating, that is sliding in from the right:

https://cl.ly/182R0t2T2b2a/Screen%20Recording%202016-11-29%20at%2008.53%20AM.gif

I updated the patch by introducing a slight delay before calculating the left edge and this resolved the issue for me. Ideally, I'd like to have the calculation triggered by the end of the animating panel itself, it looks like expandedSection is firing as the animation starts. I haven't looked into where the animation happens to see how to control the timing.

After my updated patch drag and drop works as expected:

https://cl.ly/1i1M15310h1i/Screen%20Recording%202016-11-29%20at%2008.51%20AM.gif

@delawski
6 months ago

#9 follow-up: @delawski
6 months ago

@adamsilverstein - You're totally right! My mistake was to use expandedSection that is fired before the animation is started instead of just args.completeCallback function which is called right after the transition is completed (https://github.com/xwp/wordpress-develop/blob/master/src/wp-admin/js/customize-controls.js#L1006-L1021 - _animateChangeExpanded takes a callback as an argument, that is called after the transition is complete).

So, just as you suggested, instead of using delay function, the completeCallback function can be extended, and the exact end of the transition can be used. I've attached an updated patch that is following that path.

#10 @celloexpressions
6 months ago

  • Keywords commit added

Confirmed that 38948_2.diff restores the expected behavior. I believe this is the same issue that was peviously encountered at least once during 4.3 development. With the change, I'm more confident that we won't run into it again.

This bug seems to manifest itself differently depending on the device and browser; the severity may vary accordingly. Drag-and-drop sorting and submenus work quite well for me with mouse or touch on Chrome/Windows 10 with the patch.

Patch looks ready to commit, my only hesitation is the two completeCallbacks may be a bit confusing to look at at first but I think it's fine. Nice work everyone!

#11 follow-up: @westonruter
6 months ago

  • Keywords dev-feedback added
  • Owner set to westonruter
  • Status changed from new to accepted

Diff of changes with whitespace changes suppressed: https://github.com/xwp/wordpress-develop/pull/209/files?w=1

+1 on commit. Needs dev-reviewed.

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


6 months ago

#13 in reply to: ↑ 9 @adamsilverstein
6 months ago

Replying to delawski:

So, just as you suggested, instead of using delay function, the completeCallback function can be extended, and the exact end of the transition can be used. I've attached an

@delawski Nice work! I knew there was a better way than my delay, glad you knew right where to find that!

#14 in reply to: ↑ 11 @mikeschroder
6 months ago

  • Keywords dev-reviewed added; dev-feedback removed

Replying to westonruter:

Diff of changes with whitespace changes suppressed: https://github.com/xwp/wordpress-develop/pull/209/files?w=1

+1 on commit. Needs dev-reviewed.

+1

#15 @westonruter
6 months ago

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

In 39376:

Customize: Fix regression in ability to create submenus for nav menus via drag and drop.

See #34391.
Fixes #38948.
Props delawski, adamsilverstein.

#16 @westonruter
6 months ago

In 39377:

Customize: Fix regression in ability to create submenus for nav menus via drag and drop.

See #34391.
Fixes #38948 for 4.7 branch.
Props delawski, adamsilverstein.

Note: See TracTickets for help on using tickets.