WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 4 months ago

#39430 closed defect (bug) (fixed)

Customize: sections and panels that are open and become inactive should be closed

Reported by: celloexpressions Owned by: westonruter
Milestone: 4.7.4 Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: has-screenshots has-patch commit fixed-major
Focuses: javascript Cc:

Description (last modified by westonruter)

If you have a section open with an active callback that's true, then navigate to a page where it isn't active, the section remains open. Since it's no longer intended to be available, it should be collapsed as soon as it becomes inactive if it's currently expanded.


If a panel or section has an active_callback set, and if the panel or section is expanded, when a user navigates from previewing a URL where the active_callback returns true to one where it returns false, these the non-contextual panel or section remain expanded unexpectedly. If you collapse the panel or section to go to the root you then see that the references to the panel/section are hidden.

I believe this is a regression introduced in #34391, as I'm sure that the panels would collapse automatically when de-activated in the past.

Video depicting the problem: https://youtu.be/h1kaRLn7A8g
Plugin that reproduces the problem as used in the video: https://gist.github.com/westonruter/4a4c378c750cb5432ecaf88f5d9ba539

Attachments (4)

39430.before.gif (3.7 MB) - added by celloexpressions 6 months ago.
Example on a theme supporting header media only on the front page - when navigating to another page the options are still visible and open but do nothing since header media is only active on the front page. The section should be closed since it becomes hidden.
section_gets_empty.gif (1.7 MB) - added by nikeo 6 months ago.
39430.diff (1.2 KB) - added by dlh 4 months ago.
39430.2.diff (2.0 KB) - added by westonruter 4 months ago.

Change History (26)

@celloexpressions
6 months ago

Example on a theme supporting header media only on the front page - when navigating to another page the options are still visible and open but do nothing since header media is only active on the front page. The section should be closed since it becomes hidden.

#1 @celloexpressions
6 months ago

  • Keywords has-screenshots added

Added a screencast demonstrating the issue. If a section becomes inactive, it should be closed. In the header video example there's a warning that the video isn't shown here. But since the entire section's active_callback is false here, it should be hidden or we could show a generic notification on the section that it isn't in the preview.

#2 @celloexpressions
6 months ago

#39663 was marked as a duplicate.

#3 @celloexpressions
6 months ago

  • Milestone changed from Future Release to 4.7.2
  • Version changed from 4.1 to 4.7

Per @westonruter this seems to be a regression caused by #34391.

#4 @westonruter
6 months ago

  • Description modified (diff)

#5 @nikeo
6 months ago

Related use case illustrated with the gif : if a section is currently expanded and the user navigates in the preview to a page where all controls of this section are getting contextally inactive, the section becomes empty.
In this case, shouldn't the focus be set to the parent panel level ?

#6 follow-up: @westonruter
6 months ago

@nikeo Thank you very much for this. It is something else I noticed but I couldn't figure out how to reproduce the issue. Yes, the section should absolutely be collapsed with the user returned to the panel level (or the root).

#7 in reply to: ↑ 6 @nikeo
6 months ago

Replying to westonruter:

@nikeo Thank you very much for this. It is something else I noticed but I couldn't figure out how to reproduce the issue. Yes, the section should absolutely be collapsed with the user returned to the panel level (or the root).

@westonruter You're welcome. I actually noticed the problem quite recently ( regression or always been like that? Not sure) and was about to open an enhancement when I stumbled upon this ticket.

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


5 months ago

#9 @westonruter
5 months ago

  • Milestone changed from 4.7.3 to 4.7.4

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


5 months ago

#11 @westonruter
5 months ago

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

#12 @westonruter
5 months ago

I'm noting this as a dependency for #37471.

@dlh
4 months ago

#13 @dlh
4 months ago

  • Keywords has-patch added; needs-patch removed

39430.diff contains two changes:

  1. Reapplies the fix from [34557], which it looks like was lost in [38648].
  1. Removes the calls to stop() inside Container.onChangeActive().

(2) exists to try to address behavior that I saw after applying (1): If I go to Test Panel > Test Section N (from the demo plugin above), then navigate away from the front page in the preview, the section and panel close as expected, but then the entire .wp-full-overlay-sidebar-content disappears. The only way to get it back is to click an inline edit link.

I can't explain yet why stop() would cause this behavior except for a general guess that it interferes with the animation-related changes in [38648].

If that seems incorrect, or if stop() is serving a need, I can look for another approach.

Last edited 4 months ago by dlh (previous) (diff)

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


4 months ago

#15 @swissspidy
4 months ago

  • Keywords needs-testing added

#16 @jipmoors
4 months ago

I have been working on documenting JavaScript throughout the WordPress admin and I noticed that there is a fairly opaque command without an explaining comment accompanying it.

You would make me very happy if you could add a description to go along the first changed line in the patch @dlh

@westonruter
4 months ago

#17 @westonruter
4 months ago

  • Keywords needs-testing removed
  • Owner changed from dlh to celloexpressions
  • Status changed from assigned to reviewing

@dlh In my testing the changes work really well. The panel and section collapse when deactivated as expected, in Chrome and Firefox.

@jipmoors 39430.2.diff adds some more clarifying comments.

@celloexpressions @delawksi If either of you could provide any final confirmation on the patch, it would be appreciated, as you both were heading up [38648]. (As well as confirmation of fix in IE.)

Once that feedback is received, this can be committed.

#18 @westonruter
4 months ago

  • Keywords commit added

I just tried the patch in IE9 on a Windows 7 VM and it works as expected.

I'll commit this on Monday unless there is any feedback to the contrary before then.

#19 @celloexpressions
4 months ago

  • Owner changed from celloexpressions to westonruter

Verified that 39430.2.diff fixes the issue, testing in Chrome and Edge. Note that I was unable to verify the fix in IE11 due to #40198.

#20 @westonruter
4 months ago

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

In 40304:

Customize: Fix failure to collapse expanded sections and panels that become deactivated.

Improve jsdoc for onChangeActive function. Restores fix from [34557] which got dropped in [38648].

Props dlh, westonruter.
See #34391, #33509.
Fixes #39430.

#21 @westonruter
4 months ago

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

Re-opening for consideration in 4.7.4

#22 @swissspidy
4 months ago

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

In 40375:

Customize: Fix failure to collapse expanded sections and panels that become deactivated.

Improve jsdoc for onChangeActive function. Restores fix from [34557] which got dropped in [38648].

Props dlh, westonruter.
See #34391, #33509.
Fixes #39430.

Merges [40304] to the 4.7 branch.

Note: See TracTickets for help on using tickets.