Speed up slow Menus Panel for menus with many items

[sevenspark]

Overview

When the new accessibility enhancements (​relevant commit) were added to the Appearance > Menus Panel in WordPress 3.6, it added a lot of extra javascript processing per menu item. For large menus (say, > 50 menu items), this creates a bottleneck and slows down both the menu panel load time and the ability to manage the menu, resulting in a poor user experience - for a menu with 100 items, the lag is around 6 seconds in my tests (and in extreme cases, users have experienced browser crashing due to not being able to handle the javascript load). Basically, the processing time for these accessibility enhancements increases linearly with each new menu item, slowing things down considerably once there are many menu items in play.

Details & Demonstration

I've written up a detailed article on the issue here: ​Speeding up the Appearance > Menus Screen in WordPress 3.6

I've also created a screencast demonstrating the issue: ​Video - which shows and times the lag, as well as demonstrates the performance improvement using my "lazy" solution.

(Please note I also tested in WordPress 3.7, and the same issue/solution applies.)

Issue Source Synopsis

After doing some troubleshooting, I believe that the issue occurs in wp-admin/js/nav-menu.js, in the function refreshAdvancedAccessibility(). The problem is that both on page load and any time a change is made, the entire set of menu items needs to be reprocessed, causing the page to become unresponsive for several seconds until the processing completes. (Please see linked article and video above for more detailed explanation)

Potential Solution (including available plugin)

I've outlined my solution in the article I linked to above. Instead of processing every menu item every time, this solution takes a "lazy" approach and only processes menu items when the item is about to be interacted with (triggered on hover, focus, or touch). This greatly reduces the processing load and allows the page to become immediately responsive, improving user experience. The lag when moving menu items is also eliminated. I am not an accessibility expert, but I believe triggering the processing on focus will ensure that the accessibility features are present for those interacting with the site without a pointer device.

I've also written and released a plugin - ​Faster Appearance - Menus (also on ​github) which implements this solution by replacing the default nav-menu.js with my modified version. The plugin was released about 2 months ago, and has received positive feedback from many users who were encountering slow Menus Panel speeds (as someone who writes plugins dealing with the creation and management of menus with many items, I've encountered this issue with a lot of customers). That being said, I'm sure it could be improved and needs more stringent testing and code review.

Thanks! This is my first ever submission to trac, so I do apologize if I have done anything wrong here.

[atimmer]

This is a duplicate of #25112.

I made some performance improvements in the refreshAdvancedAccessibility function, those are in 3.7. You should see a performance improvement in 3.7 compared to 3.6, but it probably wasn't enough.

This is a refreshing solution, are you going to write a core patch for this or should I?

[atimmer]

​25698.1.diff takes the changes in the plugin and puts them together with the changes made in 3.7/#25112. It also has an extra check to make sure open items always get refreshed.

Another way to improve this would be to improve the logic in the new refreshAdvancedAccessibility function, instead of marking all menu items as needing a refresh only mark a subset of menu items that actually need a refresh.

This needs some testing though, we don't want a case where there is no accessibility. Because that would essentially be a regression.

[sevenspark]

Sorry for the late reply, had some emergency tasks to deal with in the last 48 hours that required my full attention.

My apologies for the duplicate! I did search for similar tickets, but seem to have missed that one. Sorry about that!

Thank you for writing the core patch - while I would have been happy to attempt it, I'm sure it's more efficient that you've written it and made sure it's done right the first time, as I have not written a core patch before.

I did look into the idea of marking only certain items for refresh when I initially analyzed the problem. I believe the logic ended up getting fairly convoluted when you consider the cascading effects that moving and adding items have. I believe part of the issue is that the menu items aren't in a true tree structure (child items are visually indented, but not actually children of the parent menu item within the DOM), so traversing the one-dimensional "tree" of menu items actually requires a fair bit of processing - I'm not sure if it actually saves anything in the end. That's why I ended up landing on the 'lazy' solution which, while not perfectly efficient, gave the biggest win with the smallest processing overhead.

I did test the plugin with a keyboard, and that (rather non-rigorous) test worked well. Unfortunately, I don't know enough about screen-reader and other types of accessibility to know if and how they might be affected, or what corner cases might crop up.

Thanks!

[joedolson]

So, I tested only with NVDA/Firefox and NVDA/Chrome, and with keyboard without screen reader. Using those tools, I didn't find any regressive changes - although I did find a bug which exists in both versions of the code. The bug did not effect keyboard use, but did effect NVDA.

1) Use 'move to top' to move an item to the first position in the menu.
2) Tab to next control.

Expected behavior: remain in current menu item and access options for that item. True with keyboard use. Actual result: move to menu item that was the next item for the original location of the menu item moved.

I don't have a suggestion for resolving that, unfortunately...but I'll play around and see if I can find something, and create a new ticket for that issue. This is just an FYI.

I'm not confirming that there's no regression of accessibility, but if so, it's not obvious.

[rianrietveld]

Tested the plugin with keyboard on Lion with and without MP6
Safari: acts the same
Firefox: acts the same
Chrome: acts the same

Only on Firefox (with and without the plugin), the focus skips the menu-to-edit completely, from save_menu_header the focus jumps to auto-add-pages.

[atimmer]

25698.2.diff refreshes 25698.1.diff after [26163].

Also makes sure we are not introducing new jshint errors.

[gezginrocker]

My site has about 150 menu items & I'm having this problem since 3.6. Currently I'm on 3.7.1.

I tried both suggested plugin & patch separately. Either of them didn't make any difference. As soon as I opened the menu editor, I got "unresponsive script" warning & page completely hanged. No way to move menu items.

But when I tried both plugin & patch together, I have seen major improvements. I still got "unresponsive script" warning in the beginning, but I can move the menu items with little delay.

Platform: Firefox 20 running on Linux Mint

[atimmer]

Replying to gezginrocker:

My site has about 150 menu items & I'm having this problem since 3.6. Currently I'm on 3.7.1.

I tried both suggested plugin & patch separately. Either of them didn't make any difference. As soon as I opened the menu editor, I got "unresponsive script" warning & page completely hanged. No way to move menu items.

But when I tried both plugin & patch together, I have seen major improvements. I still got "unresponsive script" warning in the beginning, but I can move the menu items with little delay.

Platform: Firefox 20 running on Linux Mint

Have you cleared the cache after applying the patch/activating the plugin?

There cannot be any difference between plugin + patch or only plugin, because the plugin replaces the script where the patch is applied.

[sevenspark]

Came on to respond and I see atimmer already covered that - sounds like a case of not clearing the cache.

But in the meantime I also realized that the code should be modified to handle Microsoft's touch events as well (MSPointerUp and pointerup). So I think this code should be modified

//Setup the refresh on hover/focus/touch event
$( '#menu-management' ).on( 'mouseenter.refreshAccessibility focus.refreshAccessibility touchstart.refreshAccessibility' , '.menu-item' , function(){
                  api.refreshAdvancedAccessibilityLazy( $( this ).find( '.item-edit' ) );
});

to this

//Setup the refresh on hover/focus/touch event
$( '#menu-management' ).on( 'mouseenter.refreshAccessibility focus.refreshAccessibility touchstart.refreshAccessibility MSPointerUp.refreshAccessibility pointerup.refreshAccessibility' , '.menu-item' , function(){
                  api.refreshAdvancedAccessibilityLazy( $( this ).find( '.item-edit' ) );
});

to ensure this will work in Internet Explorer as well on Windows 8 touch-enabled devices.

[gezginrocker]

I thought I cleared the cache. But I was running Linux from a USB drive, probably this was the problem.

I tried again on my friend's Windows box. Patch is slightly better then the original version, but still really slow to respond.

On the other hand, plugin makes a really big difference. I can move items with almost no delay. Thanks a lot sevenspark, you're a life saver.

[atimmer]

I refreshed the patch for current trunk, I also added comments. See 25698.3.diff

[meshmarketer]

Testing this plugin on menu structure of over 100 items found immediate responsiveness improvements.

[SergeyBiryukov]

Version number indicates the earliest affected version. The ticket description specifically mentions 3.6.

[atimmer]

You can easily generate loads of menu items by using the following shell script:

#!/bin/bash

c=1
while [ $c -le 50 ]
do
	wp menu item add-custom small-menu "Title $c" "http://example.com/?var=$c"
	(( c++ ))
done

[johnbillion]

#29511 was marked as a duplicate.

[atimmer]

Refreshed the patch against current trunk.

[helen]

Would like another review of the patch, but otherwise looks okay and would really like to get this done.

[DrewAPicture]

Per @helen's comment, the patch could use a second set of eyes. Moving for commit consideration.

[helen]

In 31604:

Nav menus: Better JS performance on initial load of edit screen.

The accessibility helpers previously processed all items when editing a menu, which was quite slow to the point of being unresponsive for large menus. They now only process items when they are expanded or a user comes near them in some way, such as hover or focus.

Also simplifies a redundant set of click event handlers down to one, which further enhances performance.

props atimmer, sevenspark.
fixes #25698.

[helen]

In 31605:

Fix some inline doc typos that were missed in [31604]. see #25698.