Screen Options in Appearance -> Menu not saved correctly sometimes

[Asgaros]

Hello everyone!

I found a strange behavior when I want to change the "Screen Options" in the "Appearance -> Menus" area inside the administration interface.

Lets assume that in the first step all checkboxes are selected:

Now I deselect all "Boxes" checkboxes:

After that I leave the "Appearance -> Menus" page (lets go to the dashboard for example) and go back to it again.

When I try to unselect one of the "Show advanced menu properties" checkboxes like the "Title Attribute", leave the "Appearance -> Menus" page and go back to it, the checkbox is selected again. Sometimes even all of the "Boxes" checkboxes are selected again.

It is not always possible to reproduce it because the problem does not occur everytime and seems to be a little bit random. Sometimes if I unselect all "Boxes" checkboxes and come back to the page, some of them are selected again. Another example: If I select two/three of the "Boxes" checkboxes after all of those checkboxes were unselected and refresh the page, all or some of the not-selected "Boxes" are selected as well.

I tested this with version 4.4/trunk on different sites with different browsers (Chrome/Edge) and different themes (default and own).

I hope someone can reproduce this problem too. If not I can try to upload some screen capture later.

[afercia]

[34991] removed the IDs from the "columns" checkboxes :( but they're needed for getting checkboxes to be saved via AJAX. Not sure about the "Boxes" checkboxes, still investigating.

[afercia]

Additionally, as far as I see, in the Menus screen the "Boxes" checkboxes have 2 click events attached. The first one from postbox.js by postboxes.add_postbox_toggles() and the second one from nav-menus.js by wpNavMenu.initToggles(). When the first one is triggered, the AJAX call in the second one is triggered too but the ID values are not get yet. So there are always 2 AJAX calls and the first one has empty parameters.
This happens also in previous WP versions and shouldn't be directly related to the reported bug.

[afercia]

Replying to afercia:

When the first one is triggered, the AJAX call in the second one is triggered too

Almost :) Actually, both functions trigger an AJAX call, see postboxes.save_state()

[afercia]

In the proposed patch:

• On the Screen Options in the Menus screen, restore the checkboxes IDs removed in [34991]. Also, avoid a useless AJAX call.

Some testing would be greatly appreciated :)

[Asgaros]

@afercia Thanks for the patch! The Screen options in the Appearance -> Menu area are working great with it.

But I think I found another related problem. Here are the relevant steps to reproduce it:

Step 1: Lets assume that in the first step all checkboxes are selected again.

Step 2: Click on Manage in Customizer

Step 3: Click the settings icon. All checkboxes are selected. So far so good.

Step 4: Now de-select one checkbox, refresh the page and click the settings icon again. All checkboxes are selected again now.

If I change checkboxes more than one time in this step, everything is saved correctly. It looks like the first AJAX request is missing some parameters/data.

Example:

When i de-select Link Target in Step 4 this request-data will be sent:

But it should send this request-data (which will be sent when I un-select, select and un-select the checkbox again):

I hope it helps! :)

[Asgaros]

I modified the hidden-function in the customize-nav-menus.js a little bit. The

this.hidden = function() { 
	// ...
};

wrapper prevented it from working. More testing would be great.

[afercia]

@Asgaros thanks very much for your thorough test :)
Yep looks like a separate issue in customize-nav-menus.js since when clicking the first time on a checked checkbox, the returned value is undefined instead of an ID. This should probably go in a separate ticket but I will defer to @westonruter :)

[westonruter]

@Asgaros @afercia I cannot reproduce the issue. With 35112.2.patch​ applied, I am able to uncheck one checkbox and reload and see that one checkbox still unchecked: ​https://cloudup.com/cyH16zBhoVD

[afercia]

@westonruter Yep, 35112.2.patch​ seems to fix it by the way the first bug is a regression from 4.4 and the second one is unrelated, probably has worked this way forever. Not sure why it was coded that way.

Just to clarify, with the first patch applied which fixes just the regression, here's what happens for me. See screenshot, dumping hidden in customize-nav-menus.js at line 616.
The first time I click on "Link Target", hidden is undefined, the second time I click on "Title Attribute" and it correctly returns both IDs.

[azaozz]

35112.patch looks good for 4.4.1 here too.

35112.2.patch should probably go in 4.5. There is another typo in hidden: function()... that has probably been there for a while:

return id.substring( id, id.length - 5 );

should be

return id.substring( 0, id.length - 5 );

[afercia]

In 36137:

Admin: Restore the "Show advanced menu properties" checkboxes IDs.

These checkboxes are used on the Menus screen options and the Customizer Menus options.
Their IDs were removed in [34991] but they're needed to get the checkboxes to be saved
via AJAX. Also, avoids a useless AJAX call.

Fixes #35112 for trunk.

[afercia]

Reopening for 4.4.1.

[afercia]

The additional issues discovered here are now split in #35273.

[dd32]

In 36145:

Admin: Restore the "Show advanced menu properties" checkboxes IDs.
These checkboxes are used on the Menus screen options and the Customizer Menus options.
Their IDs were removed in [34991] but they're needed to get the checkboxes to be saved
via AJAX. Also, avoids a useless AJAX call.

Merge [36137] to the 4.4 branch.
Props afercia.
Fixes #35112.