Make WordPress Core

Opened 8 years ago

Closed 2 years ago

Last modified 2 years ago

#34436 closed defect (bug) (fixed)

Issue with wp.customizer.panel('name').focus().

Reported by: wpweaver's profile wpweaver Owned by: hellofromtonya's profile hellofromTonya
Milestone: 5.9 Priority: normal
Severity: normal Version: 4.3.1
Component: Customize Keywords: has-patch commit
Focuses: javascript Cc:

Description

This is a small bug (feature?) when using wp.customizer.panel('panel-name').focus().

If you have a Section open (the options being displayed), wp.customizer.panel('parent-of-that-section').focus(); will not switch focus from the section display to the parent panel. Using wp.customizer.panel('any-other-panel').focus(); works.

My new theme interface builds an auxiliary menu to more easily navigate all the panels and sections, and it all works nicely except this case.

I can provide a beta of my theme that demos this if needed. This is likely not really critical, but it does not work as one would expect. This issue manifests in both 4.3.1 and 4.4 beta 1.

Attachments (3)

#34436 - With PR #1322.gif (844.7 KB) - added by costdev 3 years ago.
#34436 - With PR #1322
34436-before-patch.gif (2.7 MB) - added by hellofromTonya 2 years ago.
Test Report: Before applying PR 1322 => Reproduces reported issue
34436-after-patch.gif (4.3 MB) - added by hellofromTonya 2 years ago.
Test Report: After applying PR 1322 => Resolves the issue ✅

Change History (21)

#1 @celloexpressions
8 years ago

  • Keywords needs-patch added

Essentially, the issue here is that focus() doesn't work to move from a section to its parent panel. That should be a case we can fix, if someone can dig into that code.

It sounds like for your use case it may be more appropriate to use section|panel.expand()/collapse() directly. But I think we should be able to add a check to focus out of a section to a parent panel.

#2 @westonruter
8 years ago

  • Milestone changed from Awaiting Review to Future Release

#3 @celloexpressions
3 years ago

  • Focuses javascript added
  • Keywords good-first-bug added
  • Milestone set to Future Release

This should be a relatively straightforward bug to patch. Likely a matter of adding a conditional to the panel.focus() JS method that closes any open sections.

To test: open the customizer to editing a specific menu. Enter this into the browser console: wp.customize.panel( 'nav_menus' ).focus();. Expected behavior is for the menu to close and the list of menus (top-level menu panel) to appear.

This ticket was mentioned in PR #1322 on WordPress/wordpress-develop by costdev.


3 years ago
#4

  • Keywords has-patch added; needs-patch removed

Look for an expanded section whose panel param matches construct.id.
If one is found, collapse it.

Fixes #34436

Trac ticket: https://core.trac.wordpress.org/ticket/34436

#5 @costdev
3 years ago

I've just submitted a patch for this bug.

Apologies for the second commit - I had mistakenly added the code block to the bottom of the focus function in an effort to keep things tidy, without noticing that focusElement.focus() was already called by this point, rendering the code block useless. I've moved it to the top of the focus function so as not to disrupt the smooth sequence of declaration and usage of focusContainer and focusElement.

#6 @costdev
3 years ago

Javascript coding standards test failed. I've updated the patch to correct this.

#7 @costdev
3 years ago

I've attached a GIF showing the result of the patch. This issue seems to be fixed.

#8 @Hareesh Pillai
3 years ago

  • Milestone changed from Future Release to 5.9

Adding to the milestone for inspection.

#9 @costdev
3 years ago

Pinging Customize Component Maintainers @dlh and @celloexpressions to request inspection.

#10 @dlh
2 years ago

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

Thanks for the ping! I'll try to review the Pull Request this week.

#11 @costdev
2 years ago

Thanks @dlh!

dlh01 commented on PR #1322:


2 years ago
#12

Thanks for the updates!

Unfortunately, there seems to be a case where the logic doesn't work: If I open the Customizer with a theme that has only one widget area and run wp.customize.panel( 'widgets' ).focus() then I'm always dumped back at the top-level controls. I haven't had a chance to dig more into it, so I'm not sure whether it's a problem that happens specifically with the widgets panel or with any panel containing only one section.

#13 @costdev
2 years ago

  • Keywords needs-testing added

I've updated the PR with a check to make sure that the panel has more than one section. While this may only be with the widgets panel currently, this should facilitate any other panel that contains only one section in the future.

Adding needs-testing.

Testing Instructions

  1. Apply the patch.
  2. Run npm run build.
  3. Login to WordPress.
  4. Load the Customizer.
  5. Open DevTools:
    • On Windows, press Ctrl + Shift + J.
    • On Mac, press Cmd + Option + J.
  6. Paste the code below into the box and press Enter.
  7. Minimize DevTools.
  8. Follow the instructions on the right of your screen.
const style = document.createElement('style'),
      panelSections = document.querySelectorAll('[id^=accordion-section], [id^=accordion-panel'),
      sections = document.querySelectorAll('[id^=accordion-section], [id^=accordion-panel'),
      css = `
        .test-suite {
            background: #fff;
            bottom: 0;
            box-sizing: border-box;
            display: flex;
            flex-direction: column;
            height: 100vh;
            overflow-y: scroll;
            padding: 20px;
            position: fixed;
            right: 0;
            width: 400px;
            z-index: 9999999;
        }

        .test-suite p {
            margin-block: .5em;
        }

        .test-option {
            cursor: pointer;
            padding: 10px;
            transition: all .1s ease-in-out;
            user-select: none;
        }

        .test-option:nth-of-type(1) {
            border-top: 1px solid #ddd;
            margin-top: 1em;
        }

        .test-option:not(:last-of-type) {
            border-bottom: 1px solid #ddd;
        }

        .test-option:hover {
            background: #000;
            color: #fff;
        }
      `;

let areasToTest = [];
panelSections.forEach( (panelSection) => {
    if ( 'accordion-section-themes' !== panelSection.id
        && 'accordion-section-publish_settings' !== panelSection.id ) {
        areasToTest.push({
            'name': panelSection.id.replace( 'accordion-section-', '' ).replace( 'accordion-panel-', '' ),
            'type': 0 === panelSection.id.indexOf('accordion-section-') ? 'section' : 'panel'
        });
    }
});

const testSuite = document.createElement('div');
testSuite.classList.add('test-suite');
testSuite.innerHTML = `
    <h1>
        <a href='https://core.trac.wordpress.org/ticket/34436' title='Open ticket #34436 in a new tab' target='_blank'>#34436</a>
        Test Suite
    </h1>
    <p>
        Thank you for taking the time to test this patch!
    </p>
    <p>
        Please click on each area below to check that it is focused.
    </p>
    <p>
        Test every scenario you can think of, including double-clicking each area, quick-clicking, etc.
    </p>
    <p>
        When you're done, please leave a comment on <a href='https://core.trac.wordpress.org/ticket/34436' title='Open ticket #34436 in a new tab' target='_blank'>the ticket</a> and let us know your results.
    </p>
    <p>
        Thanks again!<br>
        <strong><a href='https://github.com/costdev'>@costdev</a></strong>
    </p>
`;
document.body.appendChild(testSuite);

areasToTest.sort((a,b) => (a.name > b.name) ? 1 : ((b.name > a.name) ? -1 : 0))
areasToTest.forEach( area => {
    const option = document.createElement('div');
    option.classList.add('test-option');

    option.tabIndex = '0';

    let name = area.name.replaceAll('_', ' ').replaceAll('-', ' ').split(' ');
    clean_name = '';
    name.forEach( word => clean_name += word[0].toUpperCase() + word.slice(1) + ' ' );
    option.innerHTML = clean_name.replace('[', ' ').replace(']', '');
    option.addEventListener('click', () => {
        if ( 'section' === area.type ) {
            wp.customize.section( area.name ).focus()
        } else if ( 'panel' === area.type ) {
            wp.customize.panel( area.name ).focus()
        }
    })
    option.addEventListener('keypress', () => {
        if ( 'section' === area.type ) {
            wp.customize.section( area.name ).focus()
        } else if ( 'panel' === area.type ) {
            wp.customize.panel( area.name ).focus()
        }
    })
    testSuite.appendChild(option)
})

if (style.styleSheet) {
    style.styleSheet.cssText = css;
} else {
    style.appendChild(document.createTextNode(css));
}

document.head.appendChild(style);
Last edited 2 years ago by costdev (previous) (diff)

#14 @dlh
2 years ago

  • Keywords commit added; good-first-bug removed

The latest updates in PR #1322 are working as expected for me. Another round of testing from another contributor couldn't hurt, but I'd say this is ready for commit consideration.

#15 @hellofromTonya
2 years ago

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

Test Report

Env:

  • OS: macOS Big Sur
  • Browsers:
    • Chrome version 94.0.4606.81
    • Firefox version 93.0
    • Safari version 15.0
    • Edge version 95.0.1020.40
  • Localhost: wp-env

Test Steps

Follow the steps given above.

Tip: Clear the Console before testing. Makes it easier to view errors and messages (if there are any).

Retested without the script:

  1. Navigate to the "Primary menu" panel
  2. Copy and paste this code into the Console: wp.customize.panel('nav_menus').focus()
  3. Press enter/return. Then notice the behavior:
    • With the patch: the parent "Menus" panel shows
    • Without the patch: nothing happens
  4. Repeat steps with "View All Locations" and "Secondary menu" panels

Results

Able to reproduce the reported issue ✅

Each panel gains focus:

  • Chrome ✅
  • Firefox ✅
  • Safari ✅
  • Edge ✅

No errors in the console when changing panel focus:

  • Chrome ✅
  • Firefox ✅
  • Safari ✅
  • Edge ✅

Works as expected in each of the tested browsers.

Last edited 2 years ago by hellofromTonya (previous) (diff)

@hellofromTonya
2 years ago

Test Report: Before applying PR 1322 => Reproduces reported issue

@hellofromTonya
2 years ago

Test Report: After applying PR 1322 => Resolves the issue ✅

#16 @hellofromTonya
2 years ago

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

In 52003:

Customize: Fix focus() to collapse child panels and show parent panel.

When a child panel is open, wp.customize.panel('parent_panel').focus() (e.g. 'parent_parent' might be 'nav_menus') collapses the child panel(s) to show the parent panel.

Follow-up to [30102], [31920], [38648].

Props celloexpressions, costdev, dlh, hareesh-pillai, hellofromTonya, westonruter, wpweaver.
Fixes #34436.

#18 @hellofromTonya
2 years ago

Thank you everyone for contributing to resolving this issue. The fix has been committed and will ship with WordPress 5.9.

Note: See TracTickets for help on using tickets.