WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#35947 closed defect (bug) (fixed)

Customizer panel fails to fully expand leaving extra margin

Reported by: tronoxas Owned by: delawski
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.4.2
Component: Customize Keywords: needs-patch
Focuses: Cc:

Description

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.

Attachments (9)

200b49.diff (1.9 KB) - added by ryankienstra 4 years ago.
35947.patch (10.2 KB) - added by delawski 4 years ago.
35947_clean.patch (1.3 KB) - added by delawski 4 years ago.
image-empty-space.png (541.7 KB) - added by ryankienstra 4 years ago.
inside-panel.png (764.3 KB) - added by ryankienstra 4 years ago.
35947_3.patch (11.1 KB) - added by delawski 4 years ago.
35947.4.patch (1.9 KB) - added by westonruter 4 years ago.
Cherry-pick patch from #34344
35947.5.patch (1.7 KB) - added by delawski 4 years ago.
Fix focus() inside widget customize control
35947.6.patch (15.7 KB) - added by delawski 4 years ago.
Try to refocus with delay on nav control in case no element was :visible initially (edge-case scenario).

Download all attachments as: .zip

Change History (43)

#1 @westonruter
4 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

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

#2 @westonruter
4 years ago

  • Summary changed from Customizer Bug to Customizer panel fails to fully expand leaving extra margin

#3 @westonruter
4 years ago

  • Milestone set to 4.5
  • Resolution duplicate deleted
  • Status changed from closed to reopened

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.

#4 @westonruter
4 years ago

  • Keywords needs-patch added

@ryankienstra
4 years ago

#5 @ryankienstra
4 years ago

  • Keywords has-patch added; needs-patch removed

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.

Last edited 4 years ago by ryankienstra (previous) (diff)

#6 @westonruter
4 years ago

  • Keywords needs-patch added; has-patch removed
  • Owner set to ryankienstra
  • Status changed from reopened to assigned

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

#7 @westonruter
4 years ago

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

@delawski
4 years ago

#8 @delawski
4 years ago

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.

#9 @ryankienstra
4 years ago

Tested
Question About Original Defect

Hi @delawski,
I might not be looking at the right defect. The way I understand it, the bug in this ticket is that some panels have empty space at the bottom, under their sections.

For example, I opened Panel 10 (screenshot). At the bottom, there was a large space under all of the sections (screenshot).

I didn't see that issue fixed with your updates to the pull request. But maybe we're looking at different defects.

This ticket's description refers to "unnecessary spacing when you go inside a panel."

Version 0, edited 4 years ago by ryankienstra (next)

#10 @ryankienstra
4 years ago

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.

Last edited 4 years ago by ryankienstra (previous) (diff)

#11 @westonruter
4 years ago

  • Keywords has-patch reporter-feedback added; needs-patch removed

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?

Last edited 4 years ago by westonruter (previous) (diff)

#12 @westonruter
4 years ago

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

@delawski
4 years ago

#13 @delawski
4 years ago

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

#14 @ryankienstra
4 years ago

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

#15 @westonruter
4 years ago

@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
4 years ago

Cherry-pick patch from #34344

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


4 years ago

#17 @ryankienstra
4 years ago

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

Fix focus() inside widget customize control

#18 @delawski
4 years ago

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

#19 @westonruter
4 years ago

@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

#20 @westonruter
4 years ago

@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

     
    102102                if ( params.completeCallback ) {
    103103                        completeCallback = params.completeCallback;
    104104                        params.completeCallback = function () {
    105                                 focus();
     105                                setTimeout( function() {
     106                                        focus();
     107                                }, 500 );
    106108                                completeCallback();
    107109                        };
    108110                } else {
  • src/wp-admin/js/customize-nav-menus.js

     
    14791479                        params.completeCallback = function() {
    14801480                                var focusable;
    14811481
    1482                                 // Note that we can't use :focusable due to a jQuery UI issue. See: https://github.com/jquery/jquery-ui/pull/1583
    1483                                 focusable = control.container.find( '.menu-item-settings' ).find( 'input, select, textarea, button, object, a[href], [tabindex]' ).filter( ':visible' );
    1484                                 focusable.first().focus();
     1482                                setTimeout( function() {
     1483                                        // Note that we can't use :focusable due to a jQuery UI issue. See: https://github.com/jquery/jquery-ui/pull/1583
     1484                                        focusable = control.container.find( '.menu-item-settings' ).find( 'input, select, textarea, button, object, a[href], [tabindex]' ).filter( ':visible' );
     1485                                        focusable.first().focus();
    14851486
    1486                                 if ( originalCompleteCallback ) {
    1487                                         originalCompleteCallback();
    1488                                 }
     1487                                        if ( originalCompleteCallback ) {
     1488                                                originalCompleteCallback();
     1489                                        }
     1490                                }, 500);
    14891491                        };
    14901492
    14911493                        control.expandForm( params );

#21 @ryankienstra
4 years ago

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

#22 @westonruter
4 years ago

  • Keywords needs-patch added; has-patch reporter-feedback removed
  • Owner changed from ryankienstra to delawski

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

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


4 years ago

#24 @chriscct7
4 years ago

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

#25 @westonruter
4 years ago

  • Keywords 4.6-early added
  • Milestone changed from 4.5 to Future Release

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

#26 @westonruter
4 years ago

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

#27 @rheinardkorf
4 years ago

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.

#28 @delawski
4 years ago

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

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

#29 @westonruter
3 years ago

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

#30 @westonruter
3 years ago

  • Milestone changed from Future Release to 4.6

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


3 years ago

#32 @celloexpressions
3 years ago

  • Keywords 4.6-early removed
  • Milestone changed from 4.6 to Future Release

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.

#33 @westonruter
3 years ago

  • Milestone changed from Future Release to 4.7

#34 @westonruter
3 years ago

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

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.

Note: See TracTickets for help on using tickets.