WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#36678 closed defect (bug) (fixed)

Shift-click in Customizer when collapsed does nothing

Reported by: sixhours Owned by: westonruter
Milestone: 4.7 Priority: normal
Severity: normal Version: 3.9
Component: Customize Keywords: has-patch commit
Focuses: Cc:

Description

Shift-clicking while the Customizer is collapsed gives no indication that the sidebar is updated behind the scenes, and therefore makes Shift-click appear broken.

To reproduce:

1) Enter the Customizer
2) Collapse the Customizer
3) Shift-click on an element (site title, tagline, widget, etc.) to edit it
4) ...nothing happens. :-/

Upon Shift-click, I'd expect the Customizer to open and show me the panel I'm editing, or for there to be some indication that *something* is happening.

Attachments (3)

36678.diff (1.8 KB) - added by curdin 5 years ago.
36678.2.diff (2.6 KB) - added by curdin 5 years ago.
36678.3.diff (2.1 KB) - added by westonruter 5 years ago.

Download all attachments as: .zip

Change History (18)

#1 @westonruter
5 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.6
  • Version changed from trunk to 3.9

Great report! When expanding anything, the collapsed state of the entire panel should be checked and expanded first.

Currently the collapsed state can be checked via $( '.wp-full-overlay' ).hasClass( 'collapsed' ), and the logic is handled in customize-controls.js:

$( '.collapse-sidebar' ).on( 'click', function() {
        if ( 'true' === $( this ).attr( 'aria-expanded' ) ) {
                $( this ).attr({ 'aria-expanded': 'false', 'aria-label': api.l10n.expandSidebar });
        } else {
                $( this ).attr({ 'aria-expanded': 'true', 'aria-label': api.l10n.collapseSidebar });
        }

        overlay.toggleClass( 'collapsed' ).toggleClass( 'expanded' );
});

$( '.customize-controls-preview-toggle' ).on( 'click', function() {
        overlay.toggleClass( 'preview-only' );
});

This is bad, however. We need a new wp.customize.Value() on wp.customize.state to represent whether the sidebar is collapsed, and the pane should be expanded/collapsed based on changes to this value, in the same way that was done with previewedDevice and each section/panel's expanded state.

#2 @celloexpressions
5 years ago

  • Milestone changed from 4.6 to Future Release

Moving to future until we have a patch. If there's a patch in time for 4.6 this can definitely go back in!

#3 @westonruter
5 years ago

  • Milestone changed from Future Release to 4.7

@curdin
5 years ago

#4 @curdin
5 years ago

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

I have created a collapsed state in wp.customizer and am using triggers to control this. Keen to have someone review this. Thanks!

Last edited 5 years ago by curdin (previous) (diff)

#5 @celloexpressions
5 years ago

  • Keywords dev-feedback removed

Thanks for the patch @curdin, nice work!

This looks good to me. The only part I'm unsure about is the naming of the state. Instead of collapsed, maybe controlsVisible or possibly paneVisible? I'd like this to be named in a way that could remain logical if this UI changes drastically in the future. collapsed could also work though, it's just a bit ambiguous about what's collapsed.

Also, it looks like this doesn't work for widgets or menus; I think we'll need to trigger collapsed to false elsewhere for them, likely somewhere in customize-nav-menus.js and customize-widgets.js, as the have their own implementations of this functionality I believe.

#6 @westonruter
5 years ago

@curdin yes, this looks like good work.

@celloexpressions it doesn't work for widgets or menus in what way?

I also think that something like paneVisible or controlsVisible would be better and more explicit than just collapsed (better to be positive than negative at least).

@curdin
5 years ago

#7 @curdin
5 years ago

Thanks for the feedback folks. The new patch has a brighter outlook on life with the collapsed state now being called paneVisible. I have also added the opening trigger to the widget and menu shift-click action.

Last edited 5 years ago by curdin (previous) (diff)

#8 @celloexpressions
5 years ago

  • Keywords commit added
  • Owner set to westonruter
  • Status changed from new to reviewing

Great, thanks @curdin! Assigning to @westonruter for commit review.

Menus and widgets don't use focus-control-for-setting, which 36678.2.diff addresses by adding the paneVisible trigger where the menu item or widget control is focused. Tested and works now for custom logo, menus, and widgets (and presumably theme and plugin partials as well).

@westonruter
5 years ago

#9 @westonruter
5 years ago

@celloexpressions @curdin please review my additional changes in 36678.3.diff. I was able to simplify it by tying everything into the paneVisible state, setting its state value and listening to changes to the state value to cause the UI to change. By listening to changes on the paneVisible state value, this also allowed for the elimination of the separate paneVisible event. I also fixed an issue with the aria-expanded and aria-label attributes not getting updated.

#10 @celloexpressions
5 years ago

I think I would prefer only changing the state when a control is being focused after an action in the preview, as the previous patches had done (although this way would also work).

Other than that, 36678.3.diff looks good to me.

#11 @westonruter
5 years ago

@celloexpressions I'm not sure. If a given panel/section/control is expanded or focused, regardless of whether or not the action originated from the preview, I would expect the pane to become visible if it wasn't already shown.

#12 @westonruter
5 years ago

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

In 38492:

Customize: Introduce paneVisible state and ensure pane is visible when a construct is expanded (or focused).

Fixes issue whereby a user would see nothing happen if the pane is collapsed while they shift-click to edit an element in the preview.

Props curdin, celloexpressions, westonruter.
See #27403.
Fixes #36678.

#13 @celloexpressions
5 years ago

Follow up for mobile support: #38080.

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


5 years ago

#15 @westonruter
5 years ago

In 39010:

Customize: Reveal controls pane when clicking on edit shortcuts in mobile preview.

Re-use paneVisible state to also manage whether the mobile preview-only class is applied to the overlay.

Props seancjones.
See #36678, #36678, #27403.
Fixes #38080.

Note: See TracTickets for help on using tickets.