In large menus javascript freezes the browser and you cannot work with the menu.

[tsdexter]

What steps should be taken to consistently reproduce the problem?

• create a large menu (in my case 1020 menu items)

In case it's relevant to the ticket, what is the expected output? What did you see instead?

• laying out and drag/dropping menu items should work - it does not as the javascript seems to have frozen the browser (definitely with chrome, firefox - it sometimes works in safari but VERY slowly)

Does the problem occur even when you deactivate all plugins and use the default theme?

• yes

Please provide any additional information that you think we'd find useful. (OS and browser for UI defects, server environment for crashes, etc.)

• tested on 2011 MacBook Pro, OSX 10.7, chrome and firefox freezes on load - safari sometimes manages to load but working with the menu is very slow - ie: dragging an item up an item block/level takes up to 30 seconds for each item it passes.

I've set severity to blocker as this caused us to have to downgrade back to 3.5.2 because we could not add to/remove from/manage our menus as we could not find anyway around it - even running on a brand new 2013 top spec 27" iMac it would still freeze the browser.

[SergeyBiryukov]

Some workarounds suggested in #14134 for large menus might be helpful.

[tsdexter]

Hi there, thanks for the suggestion, however, this is not related to not being able to save long menus - we've overcome that issue by adjusting the max_input_vars.

This issue is specifically that the new javascript layout/drag drop etc functionality in 3.6 causes the browser to freeze on large menus. This is not an issue at all in 3.5.2, even with ~1000 menu items there is absolutely no lag on page load or drag/drop - in 3.6 it freezes and/or hangs.

[SergeyBiryukov]

Moving for investigation.

[atimmer]

​25112.1.diff makes the menu editing much more performant.

The big bad function that made this so slow was "refreshAdvancedAccessibility", in my tests in chrome with around 170 items I went from 3000ms to 400ms on that function.

I don't really like the "menuItemDepth" code but it does make it more performant.

[atimmer]

Changing menuItemDepth in ​25112.1.diff was too aggressive. It is used to put the classes there in the first place.

​25112.2.diff is a new patch without those changes but with new changes. show() and hide() are heavy operations because they have to check the 'display' css property of the items they are going to show/hide. And that is heavy when used in a loop with a lot of iterations.

I replaced those calls with the equivalent, putting display on inline/hidden.

[SergeyBiryukov]

first() in line 411 can probably be removed.

[bpetty]

As this isn't a regression, and there are workarounds, this hardly counts as a blocker. Performance problems aren't bugs either actually, speed improvements are enhancements.

No time to review the patch myself just yet though, but this would probably be a nice improvement in 3.8.

Alternatively, a simple patch with just the hide() replacements might be fine to commit by Friday, assuming that is a large portion of the slowdown.

P.S. UX/UI feedback isn't necessary for performance enhancements (behavior doesn't actually change).

[tsdexter]

Can you explain the workarounds you mentioned to me? I'm not sure what they are and this bug has prevented us from upgrading an ~8000 page site that is critical to the business because we cannot modify the menus without the browser crashing. The thread SergeyBiryukov mentioned does not apply to this as it's different problem, unless you can elaborate on it? Not upgrading opens us up to hacks so we'd like to be able to upgrade before 3.8 comes out.

[markoheijnen]

If the menu page freezes it's a bug since you can't use it anymore. In a case I saw, my browser crashes when visiting the menu admin.
Not sure yet if the workarounds are really good workarounds.

[tsdexter]

In that case it's definitely a bug then because it crashes the latest version of chrome every single time you load the page on a brand 2013 27" iMac without fail. (on a 1011 item menu)

I'm aware thats a large menu but it's necessary as it's used for the mobile drilldown menu so it goes many levels deep and long to access the whole site and I'm sure other people have similarly long menus as well.

[nacin]

I'm moving this back to 3.7. There's some good data here as to why 3.6 was a regression here.

Does anyone have some decent profiling data on this, specifically pre-patch and post-patch, possibly even with individual changes isolated and explained? 'inline' is correct, not 'block', right? Maybe we could this to adding and removing a class instead, rather than changing CSS directly? (It's also not obvious why we're avoiding show() and hide() to the outsider.)

[nacin]

I don't think 3000ms (versus 400ms) should mean that the browser suddenly crashes. Are we just firing this function too often? Would debouncing help?

[atimmer]

The way to profile this is replacing line 1169 with the following:

$(document).ready(function(){ 
  console.profile( 'menus' ); 
  console.time( 'menus' ); 
  wpNavMenu.init();
  console.timeEnd( 'menus' ); 
  console.profileEnd( 'menus' ) 
});

Open the console, then reload the page. The time() is needed in chrome because chrome doesn't show total time of a profile. Do this with a unminified version of jquery or you won't be able to make any sense out of it.

I hope someone can confirm my findings.

As for my patch.

1.
closest() is exactly the same as parents() functionality wise, the difference being how they traverse the DOM tree. see ​http://api.jquery.com/closest/. closest() is significantly more performant in this case because there is a huge DOM tree and the parent element is very 'close' to the element we begin searching from.

I guess closest() would be slower if we expect to find no match, but in our case we know we always get a match.

2.
show() and hide(), I didn't know this before I began profiling but what happens when you call show() or hide() (they both map to the same function internally in jQuery) jquery first has to figure out to which css display value it needs to set the element. To do that it queries the current value of the display property.

Somehow that take a few milliseconds, I don't know why but it is the case which you see when you profile. The problem comes when we have 7 show/hide's in our code and it is a loop that loops through every menu item on the page. Even if you have a very performant browser that can do it in 1ms you get 1*7*[menu-items].

3.
$( '#menu-to-edit' ).children( '.menu-item-depth-0' ) instead of $( '.menu-item-depth-0' ). The second thing searches the whole DOM for a class, the first thing for an ID (which is fast, because ID's are unique) and then searches inside the element for the class.

The reason why these things matter is because they are in a loop that loops through all menu item's, a difference of 1ms means a difference of 1000ms with 1000 menu items.

Replying to nacin:

I don't think 3000ms (versus 400ms) should mean that the browser suddenly crashes. Are we just firing this function too often? Would debouncing help?

3000ms -> 400ms is the patch applied to 170 menu items. I first tried with 1000 menu item's and that crashed my browser.

Replying to tsdexter:

In that case it's definitely a bug then because it crashes the latest version of chrome every single time you load the page on a brand 2013 27" iMac without fail. (on a 1011 item menu)

I'm aware thats a large menu but it's necessary as it's used for the mobile drilldown menu so it goes many levels deep and long to access the whole site and I'm sure other people have similarly long menus as well.

Can you test my patch and check if the browser still crashes?

Btw. I think we won't get to levels where 1000 menu item's will be very performant considering refreshAdvancedAccessibility is called on every drag of a menu item. But at least we can do better than current 3.6.

[nacin]

In 25708:

Optimize the accessibility JS on the Menus screen to avoid browser crashes with very large menus.

props atimmer.
fixes #25112.

[SergeyBiryukov]

Should [25708] be committed to the 3.6 branch, or should the milestone be set to 3.7?

[SergeyBiryukov]

Reopening for 3.6.2 consideration.

[johnbillion]

Fixed in 3.7 and we didn't need a 3.6.2. Follow-up: #25698.

[bpetty]

There will still be a 3.6.2 release, and this could still go in it. I would re-open, except that I actually think there's no point in bothering with this one anyway for a point release.

[SergeyBiryukov]

#26054 was marked as a duplicate.