Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#35112 closed defect (bug) (fixed)

Screen Options in Appearance -> Menu not saved correctly sometimes

Reported by: asgaros's profile Asgaros Owned by: afercia's profile afercia
Milestone: 4.4.1 Priority: normal
Severity: normal Version: 4.4
Component: Administration Keywords: has-patch fixed-major
Focuses: ui, javascript Cc:

Description

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:

http://fs5.directupload.net/images/151215/7hx5hp2r.png

Now I deselect all "Boxes" checkboxes:

http://fs5.directupload.net/images/151216/khzqh833.png

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.

Attachments (2)

35112.patch (2.1 KB) - added by afercia 9 years ago.
35112.2.patch (3.0 KB) - added by Asgaros 9 years ago.

Download all attachments as: .zip

Change History (21)

#1 @SergeyBiryukov
9 years ago

  • Component changed from Administration to Menus

#2 @SergeyBiryukov
9 years ago

  • Focuses administration added

#3 @afercia
9 years ago

  • Component changed from Menus to Administration
  • Focuses ui javascript added; administration removed
  • Milestone changed from Awaiting Review to 4.4.1

[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.

#4 follow-up: @afercia
9 years ago

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.

#5 in reply to: ↑ 4 @afercia
9 years ago

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
9 years ago

#6 @afercia
9 years ago

  • Keywords has-patch added

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 :)

#7 @Asgaros
9 years ago

@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.

http://fs5.directupload.net/images/151215/7hx5hp2r.png

Step 2: Click on Manage in Customizer

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

http://fs5.directupload.net/images/151216/76fyx42y.png

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:

http://fs5.directupload.net/images/151216/cd5pzyjr.png

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

http://fs5.directupload.net/images/151216/sjlvdysu.png

I hope it helps! :)

@Asgaros
9 years ago

#8 @Asgaros
9 years ago

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.

#9 @afercia
9 years ago

@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 :)

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


9 years ago

#11 @westonruter
9 years ago

@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

#12 @afercia
9 years ago

@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.

https://cldup.com/D0Dr9UINiY.png

#13 @jorbin
9 years ago

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

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


9 years ago

#15 @azaozz
9 years ago

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 );

#16 @afercia
9 years ago

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

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.

#17 @afercia
9 years ago

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

Reopening for 4.4.1.

#18 @afercia
9 years ago

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

#19 @dd32
9 years ago

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

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.

Note: See TracTickets for help on using tickets.