Harden panel/section UI code by removing contents from being logically nested (read: goodbye margin-top hacks)

[westonruter]

The UI concept of sliding panels were introduced to Customizer in 4.0 with the introduction of the “Panel”, a grouping of sections (see #27406). In 4.2 the panel UI paradigm was extended to Sections (groups of controls), which were originally represented as accordions in the Customizer UI (see #31336).

To accomplish the sliding panel UI paradigm in the Customizer while retaining the existing nested DOM structure in a large multidimensional unordered list (panels containing sections containing controls), there have been a lot of headaches trying to get the panels positioned properly with hacks using margin-top, and maintaining this when panels/sections change their active state. (Eliminating the recalculation of the margin-top will also improve DOM performance.) Several bugs have ensued, including:

• #33567: Panel margin-top glitches when widget areas added
• #34344: Expanded section margin-top glitches when other section is deactivated
• #33220: Customizer: layout issues with autofocus'd sections on small screens
• #34529: Customizer: wp.customizer.section(xxx).focus() resizing issue (WP 4.4 Beta 2)
• #35947: Customizer panel fails to fully expand leaving extra margin

It seems clear that another approach is needed to reduce the complexity and improve the robustness of the Customizer UI.

I'd like to propose that the root “panel” of the Customizer (the links to the panels and panel-less sections) is logically independent in the DOM from the panels and sections it links to, and likewise for the links to sections within a panel to be disconnected in the DOM from the container elements for the sections they link to. By keeping these separate, we won't have to do any more margin-top hacks: the panel/section to be shown simply needs to be positioned to the top of the Customizer pane. This will mean accessibility hacks which set the root of the Customizer to visibility:hidden but a nested child element to visibility:visible won't be needed anymore (see #33258). To maintain the accessibility benefit that comes “for free“ with the current nested hierarchical unordered list, we can implement ​aria-owns to relate the panel/section links with the panel/section containers they link to.

[delawski]

Quick update

The issue is mostly fixed. Besides of flattening panel/section nested markup, I have changed the way panes are animated. Now, instead of transitioning position property, transform/translateX is changed. Thanks to that animations should be smoother and use less resources.

I still need to fix themes section and perform thorough testing.

[westonruter]

#35947 was marked as a duplicate.

[westonruter]

Another report for an issue related to panel expansion: #35947

[celloexpressions]

#34529 was marked as a duplicate.

[delawski]

I have worked on the issue today:

• fixed issues related to focusing on controls (particularly when shift-clicking on widgets in the preview)
• fixed bugs in Themes section.

I still have to test new panel/section UI on various browsers/devices and clean up the CSS.

The progress on this ticket is tracked in the following PR: ​https://github.com/xwp/wordpress-develop/pull/150

Today I've also checked two free themes (Zerif Lite and Sydney) and how they work with the new UI. The only major issue I've noticed was with Zerif Lite. In short, here's the issue: Zerif prepends its own special <li> element to any <ul> element that is child of #customize-theme-controls container. In current hierarchical implementation, there is only one child <ul> element. After flattening panels/section markup, each section and panel is a child of #customize-theme-controls container and so all of them get the special <li> which is unintended.

It is a quick fix (the selector could be changed to ul:first or .customize-pane-parent, however I imagine there might be more themes and plugins that rely on current markup.

[westonruter]

@delawski thank you. Compatibility with Customizer extensions that directly interact with the DOM is truly going to be a challenge. This is partly why I wanted to add the early keyword for the 4.6, to give plenty of time for themes and plugins to fix their integrations. As backwards compatibility is important, the big question here is whether the restructuring of the DOM in this way is so important that backwards compatibility should be broken.

@celloexpressions You've done quite a bit with custom panels and sections. What do you think about the changes?

[celloexpressions]

Generally speaking, I am okay with breaking compatibility with things that aren't using the customizer API directly, but want to keep it as much as possible for anything following best practices and using specifically-blessed API features such as custom sections and panels.

With that being said, skimming through the current patch, I don't think we need to be particularly worried about back-compat. The only PHP change is in customize.php and is only adding a class name to the root element. In terms of the JS side, custom sections and panels that generally follow the core UI should be using the core functions for expanding/collapsing rather than implementing their own versions, so the changes there should work for them. If we don't change anything in the markup for an individual section or panel, essentially, I think we'll be okay in terms of custom sections and panels, even if the css and js changes are extensive. When the JS functions are overridden by a custom section, the UI is typically different anyway.

What's described above with the Zerif theme sounds like both generally a bad idea and something that a plugin could maybe do but a theme should not do. As a general rule themes should not do anything whatsoever to change the way the customizer UI works, only add their own panels/sections/controls as needed and build additional UI through custom object types when absolutely necessary. I'm totally fine with breaking things like that but we should make our best effort to inform anyone that may be affected. If this is a common issue (and even if it's only in a few themes), we should also probably have a broader discussion with the theme review team about what themes shouldn't be allowed to do with the customizer.

[delawski]

@westonruter and @celloexpressions - thank you for your input. In such case I'll do browser/device testing and then worry about letting the community know about potential issues with backwards compatibility. I'm counting on your experience and little help with this step :-)

[delawski]

As per ​discussion on Slack, I'm submitting patch for this ticket. I haven't tested it extensively so far but I hope the community will help in finding bugs anyway.

[westonruter]

@delawski you can see from the ​Travis build that the QUnit tests failed. You can run the tests locally via grunt qunit. It may be that the settings fixture needs to be updated based on the new structure.

[delawski]

Replying to westonruter:

@delawski you can see from the ​Travis build that the QUnit tests failed. You can run the tests locally via grunt qunit. It may be that the settings fixture needs to be updated based on the new structure.

@westonruter thanks for the comment. I've noticed unit tests only recently. I'm aware of them failing in Travis and will have to update some them to fit changed markup.

[celloexpressions]

This is looking great so far! One notable change in terms of usability is that animations will be able to run much more smoothly when opening and closing panels.

Testing with 34391_1.diff​, it is not possible to open nav menu sections. The JS error thrown is related to initializing the sortable behavior here; I'm guessing that deferring that until the panel is fully expanded for the first time may help.

Additionally, the custom "add menu" section, which is displayed inline, no longer works. If at all possible I would like to use this as an example of a custom section that shouldn't require modifications as a result of this ticket, since it completely overrides the core section behavior. If it does require changes, I think we should plan to commit this within the first week of the 4.7 cycle (ie, have a patch fully tested and ready to go before the end of 4.6) so that there is adequate notice to anyone who may be doing similar things. I think we can probably avoid this, however.

[delawski]

@celloexpressions - thank you for having a look at the patch!

I was able to replicate the issue with custom 'Add menu' section. I've attached a screenshot of what I got. Is it the same bug as you noticed? I have worked and tested nav menus some time ago and they were okay back then. I must have introduced this bug in my most recent updates. I will fix the issue hopefully on by the end of the week.

I don't think custom sections like this one are going to need additional changes, but we should probably chat more about it so that we're on the same page. I'd really like the patch to be released in 4.6 and still hoping for.

I cannot replicate the sortable issue. I haven't got such error throughout development and don't get it now as well. I will test my patch on some remote server and check if the issue occurs there. If you could provide more details, it would be really helpful.

[celloexpressions]

Menu sections won't open at all for me (stays on panel view), likely because of the error. Do you have any menus in your dev environment with multiple items in them? That may be necessary to get the error.

And yes, hopefully we can avoid needing additional changes and make 4.6!

[celloexpressions]

Punting for now - this should definitely happen early in a release cycle and we're getting close to beta with no activity in the past month. I'd love for this to have a final patch ready for commit right after 4.6 is released, do you think you'll have time to finish this up in the next couple months @delawski?

[delawski]

@celloexpressions - yeah, I had busy times recently closing up another project. I hope to have more capacity in near future so that I could fix any issues noticed so far and push patch early in next release.

[delawski]

@celloexpressions - I've been working on the patch lately and I think I've came up with a good solution. Could you, please, checkout the feature branch:

​https://github.com/xwp/wordpress-develop/pull/150

or use the new patch attached: 34391_2.diff

and test if you still have any issues with nav menus? I'm having not issues on my end, but wanted to make sure with you. The patch contains updated unit tests. Also, I've slightly modified the way transitions are now performed and added a bunch of other minor updates.

Thanks!

[delawski]

I've done today browser testing. The patch works great for me on:

• Chrome (latest) Mac/Win
• Firefox (latest) Mac/Win
• Safari,
• IE 11,
• IE 10.

In case of IE 9, there was a minor layout bug, but I have fixed it in the latest patch (attached). IE 9 doesn't support CSS transitions, so panels are usable , however there is no fancy sliding effect.

I haven't tested on IE 8.

[celloexpressions]

I tested the patch and it works well for me. I've done an initial pass through the code but want to dig in deeper to make sure I fully understand the scope of the changes.

The biggest backwards compatibility red flag is that [object].container will work much differently, and in most cases should be changed to .content. We'll need to do everything we can to alert theme and plugin developers of this change, and should search the theme and plugin repos for extends WP_Customize_Section and extends WP_Customize_Panel to alert specific authors, in addition to a make/core post.

@delawski could you take a look at the patch on #37661 and gauge how drastically that custom panel and section would need to change to account for the changes here? The sections need to be nested within the panel markup, but section headings are separated from content there. I don't think any 3rd parties will have changes this drastic, but I want to make sure it's feasible to update if there are any.

[delawski]

I've been working recently on refactoring of my original solution.

• container is now a set of two jQuery objects as suggested by @westonruter (​https://wordpress.slack.com/archives/core-customize/p1473792730000673). This way the backwards compatibility should be better, as most of jQuery's methods like find() should still work well with new panel/section structure.
• Introduced two new properties to the Container, namely: headContainer which stores the head or label of the panel/section and contentContainer which stores the content area of the section/panel (usually ul).
• Abstracted _animateChangeExpanded() method so that there is no code duplication now.
• Moved code responsible for screen flickering prevention to the _animateChangeExpanded() method.
• Removed all unnecessary minor changes in the code, so that the patch itself is more consistent.

Here's the diff: https://core.trac.wordpress.org/attachment/ticket/34391/34391_4.diff

Next, I'm planning to test the patch with @celloexpressions's #37661.

[celloexpressions]

@afercia can you verify that there is no net change to the usability of sliding panels for assitive technology with 34391_4.diff​?

@delawski a few notes:

• There is a double-margin before the theme section now. We should remove the margin-top from the themes section heading on the top level so that the margin on customize info remains for users that can't change themes.
• Tested on mobile and it looks good.
• There is a lot of churn in the CSS here, but I think it's a net red, which is good. Also eliminates some oddities like a left: -354px with no indication of why that's the number.
• What about putting some of the container classes on the content container now also to reduce 3rd-party CSS change? For example, I'd prefer for the customize-nav-menus.css changes to not be needed. We should also avoid adding any new selectors that use any of the accordion- classes, as those are legacy from the old UI that's no longer used.
• To reduce conflicts with #37661, let's avoid any changes to the themes section JS that aren't required (ie, if section.container still works let's leave that instead of switching to section.contentContainer).
• The patch adds an empty @since to customize-nav-menus.js.

[delawski]

@celloexpressions - I have addressed the issues/suggestions you listed in the comment. For now they can be previewed in the following PR: ​https://github.com/xwp/wordpress-develop/pull/150/files

I wasn't just sure about the last one - an empty @since tag. Should I remove it or put there a particular release number?

[celloexpressions]

You can put 4.7.0 for all @since tags added in the patch; and generally do the current release and the committer will update if it's later.

[delawski]

@celloexpressions - I've attached the latest patch with all your suggestions implemented and with other updates @westonruter mentioned ​in GitHub review comments.

In regards to recent Slack conversations, I'm intending to write a Make post about changes to panels/sections structure. I'll run it through some of you first.

[westonruter]

@delawski great. Let's get the Make/Core post published tomorrow, and I can commit the patch tomorrow as well. This would be a dev-note as opposed to a feature proposal, so there's no need to wait for a period for commenting.

@celloexpressions once you've done your final review, please add commit keyword.

[westonruter]

@delawski oh one bit of feedback as I've been using WP with the feature branch checked out. While the animations are very smooooth thanks to the transitions, it also seems like the animations are a bit slower than before and could potentially be sped up?

[westonruter]

In 38648:

Customize: Re-architect and harden panel/section UI logic.

Removes contents for sections and panels from being logically nested (in the DOM) in order to eliminate many issues related to using margin-top hacks. The element containing the link to expand the content element for panels and sections is now a sibling element to its content element: the content is removed from being nested at initialization. The content element is now available in a contentContainer property whereas the head element (containing the link to open the construct) is in a headContainer property. The existing container property is now a jQuery collection that contains both of these elements. Since the head element is no longer in an ancestor element to the content element, the aria-owns property is now used to maintain the relationship between the headContainer and the contentContainer. These changes are also accompanied by an improvement to the animation performance for the sliding panes.

Props delawski, celloexpressions.
Fixes #34391.
Fixes #34344.
Fixes #35947.

[celloexpressions]

We need to render panels' content when the container isn't detached. This is fixed by the latest patch on #37661 and not worth breaking out separately and cross-coordinating with the other patch again since that's most likely the only instance where it will break currently in existence.

However, I'm reopening this ticket for tracking so that we can ensure we pick it up regardless of the other ticket's status. We'd likely commit this fix separately from that feature, but it doesn't make sense to hinder testing on that project in the meantime by rushing this change in here.

[westonruter]

In 38668:

Customize: Fix focusing on controls for widgets and nav menu items after [38648].

Shift-click on nav menu items was not expanding the nav menu section, and shift-clicking on widgets would not always result in focus being added to an element in the control's container.

See #34391.

[celloexpressions]

Looks like when the UI for #37661 was reworked, it no longer required un-detaching the panel there. But the patch still moved panel.renderContent() out of the conditional, which I believe was the required fix here.

customize-controls.js:
1324	1591           if ( ! panel.contentContainer.parent().is( panel.headContainer ) ) { 
1325	1592                container.append( panel.contentContainer ); 
1326	        -           panel.renderContent(); 
1327	1593          } 
 	1594    +     panel.renderContent(); 

Closing as fixed for now; please report any additional issues in new tickets.

[westonruter]

In 39153:

Customize: Fix loading of theme screenshots in themes section and remove erroneous borders on hover.

Fixes regressions introduced in [38648].

Props delawski, mckernanin.
See #34391.
Fixes #38222.

[westonruter]

In 39376:

Customize: Fix regression in ability to create submenus for nav menus via drag and drop.

See #34391.
Fixes #38948.
Props delawski, adamsilverstein.

[westonruter]

In 39377:

Customize: Fix regression in ability to create submenus for nav menus via drag and drop.

See #34391.
Fixes #38948 for 4.7 branch.
Props delawski, adamsilverstein.

[westonruter]

In 39378:

Customize: Fix regression in ability to hide fields for advanced menu properties in nav menu item controls.

Props westonruter, celloexpressions.
See #34391.
Fixes #38952.

[westonruter]

In 39379:

Customize: Fix regression in ability to hide fields for advanced menu properties in nav menu item controls.

Props westonruter, celloexpressions.
See #34391.
Fixes #38952 for 4.7 branch.

[westonruter]

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.

[swissspidy]

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.