Customizer panel fails to fully expand leaving extra margin

[tronoxas]

Here's a short video of the bug - ​https://www.youtube.com/watch?v=va2CvpoBgZM

There's this unnecessary spacing when you go inside a panel.

Steps to be taken to reproduce the problem - this is the code that you can use in functions.php to replicate the problem.

<?php
function aristath_custom_test_settings( $wp_customize ) {

    $panels_nr   = 20;
    $sections_nr = 20;
    $fields_nr   = 2;

    // Create panels
    for ( $panel_id = 0; $panel_id <= $panels_nr; $panel_id++ ) {

        $wp_customize->add_panel( 'panel_' . $panel_id, array(
            'title'      => sprintf( __( 'Panel %s', 'my_theme' ), $panel_id ),
            'capability' => 'edit_theme_options',
        ) );

        // Create sections
        for ( $section_id = 0; $section_id <= $sections_nr; $section_id++ ) {
            $wp_customize->add_section( 'panel_' . $panel_id . '_section_' . $section_id, array(
                'title'      => sprintf( __( 'Panel %1s Section %2s', 'my_theme' ), $panel_id, $section_id ),
                'capability' => 'edit_theme_options',
                'panel'      => 'panel_' . $panel_id,
            ) );

            // create fields
            for ( $field_id = 0; $field_id <= $fields_nr; $field_id++ ) {
                $setting = 'panel_' . $panel_id . '_section_' . $section_id . '_field_' . $field_id;
                $wp_customize->add_setting( $setting, array(
                    'default'    => '',
                    'type'       => 'theme_mod',
                    'capability' => 'edit_theme_options',
                ) );
                $wp_customize->add_control( $setting, array(
                    'label'    => sprintf( __( 'Panel %1s Section %2s Field %3s', 'my_theme' ), $panel_id, $section_id, $field_id ),
                    'section'  => 'panel_' . $panel_id . '_section_' . $section_id,
                    'settings' => $setting,
                    'type'     => 'text',
                ) );
            }
        }
    }
}
add_action( 'customize_register', 'aristath_custom_test_settings' );

Problem occurs in twentysixteen. All the plugins have been deactivated.

OS & Browser - Windows 8 Pro 64 bit, Firefox.

[westonruter]

@tronoxas thanks very much for the report, and for the plugin code to reproduce this. The issue you have reported will be fixed with the resolution of #34391.

[westonruter]

I notice this issue elsewhere when I resize the browser window smaller vertically, to a point where the controls in a section take up the entire available space (and more). It seems that the scrollbar may be the issue here.

[ryankienstra]

Proposed Patch

Hi @westonruter,
This patch and identical ​pull request remove extra space at the bottom of an active Customizer panel. As this ticket notes, there's "unnecessary spacing when you go inside a panel."

This occurs when there are many panels, using the code provided in this ticket to produce the panels. The non-active sections and panels still occupied space in the DOM. So the active panel had a margin-top property to move it to the top.

This PR ​sets the height of the non-active sections to 0. And it ​doesn't add a margin-top value to the active panel's content.

I tested this patch with the functions.php code from this ticket on the following themes:

​Customizr , ​Graphene , ​Heuman , ​Twenty Fifteen , ​Twenty Sixteen

There was no longer a space at the bottom of the panels. I couldn't find any side-effects. But this would definitely need to be tested more.

This patch may need to be reverted when trac-34391 is complete. It looks like trac-34391 provides a more long-term solution to this margin-top issue.

[westonruter]

@ryankienstra I found a bug. Try doing shift-click on a widget or nav menu item. You'll see that the sections get shown partially out of view: ​https://youtu.be/RIioag-7NAQ

Shift-clicking on an element that exists in a root section (e.g. site title, tagline, or logo) works fine. It's just the focusing on controls that exist in sections are contained within panels.

I'm also keen to get @delawski's closer review. I'm trying to remember why exactly margin-top was used to begin with. Maybe trying to ensure that a panel and section can be seen at the same time while the animation is happening? Actually, I'm having a hard time seeing if this the case, since the panel sliding animation happens so fast.

[westonruter]

See also #36050 (Customizer: gap above widget section when focusing from preview shift+click)

[delawski]

Different approach proposal

I have pushed my proposal for the fix to the issue. Instead of removing _recalculateTopMargin() which is risky in current sliding panels implementation and may cause new issues, I have delayed focus event on back button.

While working on #34391, I've noticed that focusing on element during CSS transition may cause unexpected results, very similar to the one presented. In case of #34391 I have implemented full solution taking advantage of transitionEnd event listening. In case of this ticket, I have used simpler solution - focus() is delayed by 180 ms which is the time needed for the transition to end.

I've tested the fix on Chrome/OS X, Firefox/OS X, Firefox/Windows 8.1, Firefox/Windows 7, IE11/Windows 7 (Twenty Sixteen in each case).

As a side note: I was able to reproduce the original issue only in Firefox.

---

Please, use 35947_clean.patch patch as it doesn't contain initial commits and reverts.

[ryankienstra]

Edited Comment

Hi @delawski,
(Edited) This ticket's defect is different than I originally thought. The screenshots above don't describe this ticket.

[ryankienstra]

Tested, Works As Expected

Hi @delawski,
The commits in your new clean patch work as expected on all of the browser and OS combinations where I tested them with "Twenty Sixteen":

Windows 10, IE 11
Windows 8, Firefox 44 (Where this ticket was reported, though I'm not sure about the version of Firefox)
Windows 7, Firefox 44
Windows 7, IE11
OSX 10.10.5, Chrome 49
OSX 10.10.5, Firefox 43.0.4
OSX 10.10.5, Safari 9.0

Like @delawski commented, I only reproduced this defect on Windows 8 and Firefox 48.

[westonruter]

I can confirm (in Firefox 44) that the latest patch ensures that the panel fully expands without leaving that extra margin on the right ​https://cloudup.com/cOI9J4Ab-Yv

@tronoxas could you try try to apply the patch and confirm that it fixes the issue on your end?

@ryankienstra @delawski But I also note that the latest patch does not fix the extra unexpected margin at the bottom of the panel. Since this wasn't the original problem raised in this ticket, is that why the fix for that is removed? Or was it due to other concerns in relation to #34391?

[westonruter]

See also closely related #34344 (Expanded section margin-top glitches when other section is deactivated)

[delawski]

I'm sorry. I have fixated on the horizontal gap and haven't really looked closer at the vertical space issue yesterday.

Thanks to great input of @ryankienstra in his earlier proposal, I think the vertical gap issue is solved now as well. Basing on @ryankienstra approach, I have applied height: 0; to list items that are inactive at the moment. I've used a bit different selectors, so that unsupported by IE8 :not() is avoided. No additional JS code was needed (besides updates in previous patch).

@ryankienstra @westonruter - could you take a look at ​my commit and check if my latest patch works for you. So far I had no chance to do more tests (only OS X Chrome/Firefox).

[ryankienstra]

Tested Newest Commit
Resolved Vertical Spacing Issue

Hi @delawski,
Your latest ​commit in this ticket's ​pull request fixed the vertical spacing issue shown in these screenshots. There's now no extra space at the bottom when you open a panel (​screenshot).

And the positioning issue that @westonruter found with my patch isn't present. I tested this in the same OS and browser combinations as my earlier tests:

Windows 10, IE 11; Windows 8, Firefox 44; Windows 7, Firefox 44; Windows 7, IE11; OSX 10.10.5, Chrome 49; OSX 10.10.5, Firefox 43.0.4; OSX 10.10.5, Safari 9.0

[westonruter]

@delawski thank you for the follow up. The positioning is feeling solid. There is one regression I am noticing, however. If you shift-click on a nav menu item or a widget, you find that the containing nav menu section or sidebar section gets the focus rather than the actual control inside the section. It seems the focus is being added to the section after the focus is made on the control?

[westonruter]

Cherry-pick patch from #34344

[ryankienstra]

Tested With Commit From trac-34344
Works As Expected

@westonruter cherry-picked the ​commit from the ​pull request for trac-34344 into this ticket’s ​pull request.

Having tested this ​pull request with the new commit, it fixes the defect in trac-34344 (Expanded section margin-top glitches when other section is deactivated).

I tested it using ​Twenty Fifteen, and the following OS and browser combinations:

Windows 10, IE 11; Windows 8, Firefox 44; Windows 7, IE11; OSX 10.10.5, Chrome 49; OSX 10.10.5, Firefox 43.0.4; OSX 10.10.5, Safari 9.0

[delawski]

Fix focus() inside widget customize control

[delawski]

@westonruter - I have investigated an issues with wrong element being focused inside widget controls after shift-click. I've noticed that the problem was present there earlier and is not related to this ticket (you could checkout master branch and see the same issue).

I have, however, found out that ​the order of focusable elements is wrong. For some reason anchor tag was picked as the first element to focus on.

So what I did is split the procedure for finding focusable elements into two stages:

1. Find first visible input, textarea, button or select.
2. If no such form element is found, search for any anchor, object or other element having tabindex property.

​You can see the changes in the same PR, the latest commit.

I couldn't find any better, more elegant solution. I'm open to other ideas.

I've attached a patch file with the latest update.

@ryankienstra - Could you, please, take a look at the solution and confirm it's working for you?

[westonruter]

@delawski @ryankienstra:

Here is what I'm seeing with your patch: ​https://cloudup.com/irgzcwCvAlX

Notice how the focus doesn't seem to be as reliably applied as doing the same shift-click behavior in trunk: ​https://cloudup.com/iVsmLGcS52v

[westonruter]

@delawski @ryankienstra I think the problem is that the :visible filter is applying before the panel/section is fully expanded. I can get the focus to apply for widgets and nav menu items if I tweak your patch with a delay:

src/wp-admin/js/customize-controls.js

@@ -102 +102 @@
                 if ( params.completeCallback ) {
                         completeCallback = params.completeCallback;
                         params.completeCallback = function () {
-                                focus();
+                                setTimeout( function() {
+                                        focus();
+                                }, 500 );
                                 completeCallback();
                         };
                 } else {

src/wp-admin/js/customize-nav-menus.js

@@ -1479 +1479 @@
                         params.completeCallback = function() {
                                 var focusable;
 
-                                // Note that we can't use :focusable due to a jQuery UI issue. See: https://github.com/jquery/jquery-ui/pull/1583
-                                focusable = control.container.find( '.menu-item-settings' ).find( 'input, select, textarea, button, object, a[href], [tabindex]' ).filter( ':visible' );
-                                focusable.first().focus();
+                                setTimeout( function() {
+                                        // Note that we can't use :focusable due to a jQuery UI issue. See: https://github.com/jquery/jquery-ui/pull/1583
+                                        focusable = control.container.find( '.menu-item-settings' ).find( 'input, select, textarea, button, object, a[href], [tabindex]' ).filter( ':visible' );
+                                        focusable.first().focus();
 
-                                if ( originalCompleteCallback ) {
-                                        originalCompleteCallback();
-                                }
+                                        if ( originalCompleteCallback ) {
+                                                originalCompleteCallback();
+                                        }
+                                }, 500);
                         };
 
                         control.expandForm( params );

[ryankienstra]

Issue With Inconsistent Focus
Seems To Be Resolved

@westonruter and @delawski,
The issue that Weston showed in his ​screencast with the inconsistent focus of nav menu items looks to be fixed with his diff above. I applied it to this ticket's ​pull request.

Before, on shift-clicking a menu item in the preview iframe, sometimes the focus was on the "Navigation Label" input field. And sometimes it was on its containing control (​screenshot).

With the diff applied, there is a consistent focus on the menu "Navigation Label," whenever clicking a menu item (​screenshot).

[westonruter]

@ryankienstra I should note that I didn't intend my patch to be the definitive solution :) but more of a workaround to help illustrate where the problem seems to lie.

[chriscct7]

@delawski + @westonruter is this going to be ready in time or should the ticket be punted out?

[westonruter]

Looks like we'll need to re-visit early in 4.6.

[westonruter]

@delawski @ryankienstra we can move this back to 4.5 if we can hash out a fix to commit this week. I'm wary of getting close to RC next week, so I'm punting it in case a patch can't be finished so focus can be placed on other 4.5 issues that do have to get done by RC.

[rheinardkorf]

My tests shows that the issue seems to be partially resolved. Testing with @ryankienstra 's ​pull request.

I am noticing that the focus does not get applied when first attempting to get focus. A subsequent click fixed the issue. I am not sure if this is a result of the patch or need another ticket raised.

To reproduce on "Twenty Sixteen".
1) Focus on a navigation element.
2) Focus on a widget.
3) Focus on a navigation element again.

You should notice that the first time attempting a focus that the inner control does not receive focus. A subsequent click will give you focus and if you're using navigation elements, each focus on navigation element will have appropriate focus. Same is true for widgets.

@westonruter this sounds similar to your issue described in your screencast.

[delawski]

I have looked into this issue for the second time.

It was really hard for me to replicate the problem. Thanks to @westonruter and @rheinardkorf instructions I was able to notice it, but only in case of refocusing on nav item after focusing on any widget.

I have slightly refactored focus method in customize-nav-menus.js. Now, if initial focus() fails (due to inputs and other focusable elements not being :visible), another focus() attempt is made with delay. Please, see the actual commit:

​https://github.com/xwp/wordpress-develop/pull/145/commits/7f607e7c0882ab155cff072ec6b6c7ebd309c962

I know this isn't the most elegant solution. However, since:

• this is an edge-case,
• #34391 targeted at early-4.6 is going change the way elements are focused,
• this ticket fixes other, much more evident issues (slight horizontal and huge vertical gaps when the number of panels/sections is high enough),

then I suggest keeping it in 4.5 rather punting to early-4.6.

Anyway, it'd be great if someone else ( @ryankienstra , @rheinardkorf ) would check if latest update fixes remaining issues. Here's the link to the PR:

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

[delawski]

Try to refocus with delay on nav control in case no element was :visible initially (edge-case scenario).

[westonruter]

I'm not going to have time to try to get this in for RC1 (today), so either we can continue iterating on this patch and commit it once trunk is open for 4.6, or we focus on the larger improvements to how panels work (#34391).

[celloexpressions]

Punting along with #34391. Since it looks like that's a more promising approach, we might want to close this ticket as a duplicate and focus efforts there.

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