Make WordPress Core

Opened 11 years ago

Closed 8 years ago

Last modified 7 years ago

#22237 closed enhancement (fixed)

Add keyboard shortcuts for collapse/expand, panel-back, and close to the Customizer

Reported by: designsimply's profile designsimply Owned by: westonruter's profile westonruter
Milestone: 4.6 Priority: normal
Severity: normal Version: 3.4
Component: Customize Keywords: good-first-bug has-patch commit
Focuses: accessibility, javascript Cc:

Description

Can we add a keyboard shortcut to the customizer for collapsing/expanding the left panel?

Attachments (10)

customize-controls-escape-handler.diff (1.0 KB) - added by purcebr 9 years ago.
Patch for esc slide up / collapse in the customize view
customizeresc.diff (1.3 KB) - added by purcebr 9 years ago.
Refreshed ESC event listener for the customizer
customize-controls-escape-handler-20150629.diff (1.3 KB) - added by purcebr 9 years ago.
Updating the patch
customizer-esc-2015-06-30.diff (1.3 KB) - added by purcebr 9 years ago.
customizer-esc-no-collapse.diff (1.2 KB) - added by purcebr 9 years ago.
22237.diff (1.3 KB) - added by celloexpressions 8 years ago.
Add esc keyboard shortcut to close sections and panels.
22237.1.diff (1.3 KB) - added by celloexpressions 8 years ago.
Fix preventDefault handling.
22237.2.diff (4.5 KB) - added by westonruter 8 years ago.
22237.3.diff (4.7 KB) - added by celloexpressions 8 years ago.
Don't use toggleForm in expandForm, remove widget reference in comment.
22237.4.diff (4.8 KB) - added by westonruter 8 years ago.
Restore params param to toggleForm

Download all attachments as: .zip

Change History (61)

#1 @designsimply
11 years ago

  • Type changed from defect (bug) to enhancement

#3 in reply to: ↑ description @jorbin
11 years ago

  • Keywords close added

Suggest won't fix due to potential conflicts with assistive technology. Might be a good plugin though.

As pointed out by esmi in #21283:

For that reason my own recommendation would be to not employ keystrokes here.

Loudly seconded! The last time a group of us looked at what keys were available once you removed all AP software keys and took multiple languages into account, there were only about 3 keys left apart from the numerics. Use anything apart from 0-9 and you will hijack someone's AT. Ten numbers don't exactly leave you a lot to play with given that you also need to use them consistently right across the board.

#4 @designsimply
11 years ago

I use WordPress an inordinate amount of time :) and I'm a big fan of keyboard shortcuts. In WordPress, comment moderation keyboard shortcuts are opt in (Users → Your Profile → Keyboard Shortcuts), so maybe customizer keyboard shortcuts could be the same.

#5 @celloexpressions
10 years ago

  • Focuses accessibility javascript added
  • Keywords needs-patch good-first-bug added; close removed
  • Milestone changed from Awaiting Review to Future Release
  • Summary changed from Add a keyboard shortcut for collapse/expand in the customizer to Add keyboard shortcuts for collapse/expand, panel-back, and close to the Customizer
  • Version set to 3.4

In the past two years, we've shifted to using keyboard shortcuts much more prominently, often to aid with keyboard nav/accessibility. @helen mentioned using keyboard shortcuts for Panels and closing the Customizer several times during 4.0 development; we should look at those too.

In fact, I'd suggest that we start by adding esc to exit a panel (if in a sub-panel -- there's a class added to a container that we can check), and also esc to close the Customizer, if not in a panel (we now have an AYS to prevent accidental closures when changes are pending).

Once we've implemented that, let's look at whether there's a good option for expanding/collapsing the controls area. Not sure what the shortcut should be, but we should only add it if it's intuitive enough that we don't need to make it a configuration option. Based on @jorbin's note above, there may not be a good option here in terms of accessibility, but I'd be interested in any more updated thoughts from the accessibility team.

Marking as good first bug since this is a fairly isolated request, and should be reasonably straightforward code-wise once the UX is nailed down (and a good place to jump in and start, with additional iterations expected). I'd put the js for it in wp-admin/js/customize-controls.js, in the less structured part of the file (after the control models).

#6 @helen
9 years ago

esc would probably be as far as I'd be comfortable with "hijacking" keyboard shortcuts.

#7 @joedolson
9 years ago

I think it would be best, for now, to simply use esc to exit the current panel. It's a standard, well understood keyboard usage. Any other shortcuts would introduce potential problems.

There is the option of defining the entire customizer as having role=application for ARIA, but I'm not sure that's justified; this UI isn't that complex.

#8 @purcebr
9 years ago

Hi There!

How about if the user taps escape, and the customizer is expanded, with a subtab expanded, we slide up the expanded tab. if they tap escape again, or when there are no sub tabs expanded, the customizer sidebar collapses. The slide up and collapse actions don't clear out unsaved changes, so we don't need to show an AYS message. I don't think escape should cause any destructive action - just hide the modal that's expanded. I've attached a patch to the customize-controls.js file to demo this functionality.

Thoughts?

-Bryan

@purcebr
9 years ago

Patch for esc slide up / collapse in the customize view

#9 @DrewAPicture
9 years ago

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

#10 @joedolson
9 years ago

I like the sound of that; it's simple and intuitive, and shouldn't break any expected behavior for AT.

#11 @ipm-frommen
9 years ago

  • Keywords has-patch added; needs-patch removed

#12 @helen
9 years ago

Can we get a11y testing on this, @joedolson?

#13 @joedolson
9 years ago

Definitely. @rianrietveld, can you get this in the testing docket for the next round of tests? (Rian coordinates the accessibility user testing group.)

#14 @rianrietveld
9 years ago

@joedolson, Yes, I will do that.

This ticket was mentioned in Slack in #core-customize by afercia. View the logs.


9 years ago

#16 @afercia
9 years ago

Asked @celloexpressions if we should still test this but seems some relevant changes will be in soon: https://wordpress.slack.com/archives/core-customize/p1423966725000697

This ticket was mentioned in Slack in #accessibility by rianrietveld. View the logs.


9 years ago

This ticket was mentioned in Slack in #accessibility by rianrietveld. View the logs.


9 years ago

#19 follow-up: @celloexpressions
9 years ago

Let's get this refreshed and into 4.3 once #31336 is committed.

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


9 years ago

#21 @samuelsidler
9 years ago

  • Milestone changed from Future Release to 4.3

#22 @ocean90
9 years ago

  • Keywords needs-refresh added

@purcebr Would you like to refresh your patch for [32649]?

#23 in reply to: ↑ 19 @obenland
9 years ago

  • Owner changed from purcebr to celloexpressions

@celloexpressions, we're one week away from beta1, could you make sure this gets a refreshed patch and committed/punted until then?

#24 @purcebr
9 years ago

I'm working on the revamped patch now - will get this up asap!

#25 @celloexpressions
9 years ago

Thanks @purcebr - I'll review it tomorrow evening and let you know how it's looking. Should be able to make it in but we can always put if it ends up being trickier than expected.

@obenland - will do.

@purcebr
9 years ago

Refreshed ESC event listener for the customizer

#26 @purcebr
9 years ago

@celloexpressions - I've attached the revamped patch. Be sure to try out the feature on both the sections (Site Title and Tagline, Colors, Header Image, Background Image, Static Frontpage on my default install) and the panels (Widgets, Menus) and their sub sections.

Let me know how it looks from your end!

thanks,
Bryan

#27 @purcebr
9 years ago

Just wanted to touch base to make sure this was on track, and you've got everything you need from me for the refreshed patch.

thanks!
Bryan

#28 @celloexpressions
9 years ago

  • Keywords needs-refresh removed

Patch looks good, although needs a few extra spaces added. Note that we use spaces around either side of conditionals, within functions, etc. For example, if( overlay.hasClass('expanded') ) { should become if ( overlay.hasClass( 'expanded' ) ) {.

I can't test the patch right now, can someone confirm that it functions as intended?

This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.


9 years ago

#30 @purcebr
9 years ago

@celloexpressions - thanks for the feedback, attached a new patch. Let me know if you see any other issues!

#31 @celloexpressions
9 years ago

Thanks. One other thing I didn't notice before: please always patch from the root of WordPress, rather than from the file directly. So the path in the patch file should be something like src/wp-admin/js/customize-controls.js instead of customize-controls.js.

#32 @celloexpressions
9 years ago

  • Keywords commit added

Testing, patch works great. Other than the patch path, this is ready for commit.

#33 @purcebr
9 years ago

Updated the patch with the proper path -

thanks!
Bryan

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


9 years ago

#35 @helen
9 years ago

  • Keywords needs-patch added; has-patch commit removed

I don't know about collapsing the sidebar with escape - I had always thought of escape as closing the customizer, not a one way toggle for the sidebar. It's very frustrating when it collapses the sidebar, as you can't quickly expand it again, or at least I didn't find any way to do that. It's also not super useful (as noted in many other places), so let's either make escape close the customizer or not do anything if you're at the top level. Need to get this done in the next couple of hours if we want it for 4.3.

#36 @purcebr
9 years ago

@helen - I see what you're saying here. I like that escape performs non-destructive transitions. If an escape at the top level closed the customizer i'd think that would be startling considering the other ways it's set up in this patch. The ESC collapse feature is useful for testing out how the theme would look, but yeah the arrow in the lower left to reexpand is small and easy to miss. And there's no similar key command to reexpand, so it's not super intuitive. (Unless Enter to reexpand? which might be confusing or unworkable for other reasons :) )

I'll reattach a patch that doesn't collapse the customizer at the root level. Let me know if you'd prefer to go the other route instead, and I can get another patch together!

Thanks for the feedback,
Bryan

This ticket was mentioned in Slack in #core-customize by purcebr. View the logs.


9 years ago

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


9 years ago

#39 @obenland
9 years ago

  • Milestone changed from 4.3 to Future Release

Let's look at this again in a future release.

This ticket was mentioned in Slack in #accessibility by rianrietveld. View the logs.


8 years ago

@celloexpressions
8 years ago

Add esc keyboard shortcut to close sections and panels.

#41 @celloexpressions
8 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 4.6

22237.diff refreshes @purcebr's latest patch for current trunk.

This is really smooth. esc closes the current section or panel, and is the only keyboard shortcut added here (it doesn't close or collapse the panel). Also noting some of the initial concerns in the accessibility team chat linked above, the exited section/panel is focused after a section is closed, so you can quickly get back into the section or panel by pressing enter after pressing escape.

This should be ready for commit.

#42 @westonruter
8 years ago

Why event.preventDefault() at the end? Is that needed?

#43 @purcebr
8 years ago

agreed, we should be able to get rid of that. At the moment, there's no handler for esc in this view, so it works as expected, but there's no guarantee of that in the future, or that there aren't other handlers loaded to listen for esc presses.

#44 @westonruter
8 years ago

Aside: This Esc keyboard shortcut will make a good power user workaround while waiting for #35186 (Put the Customizer "back" button next to the "Close" button)

@celloexpressions Assign to me with commit when you're confident it's good to go.

@celloexpressions
8 years ago

Fix preventDefault handling.

#45 @celloexpressions
8 years ago

  • Keywords commit added
  • Owner changed from celloexpressions to westonruter
  • Status changed from assigned to reviewing

I realized that we should preventDefault when we do close out of a section of a panel, but not otherwise. If, for example, esc is a shortcut to exit a full screen mode for a browser or something like that, this would prevent that from happening when it triggers section collapses but not when you're at the top level. That's probably the best way to account for anything browsers or devices might be doing with esc, allowing that action to occur one all levels of ours are complete.

I think 22237.1.diff is ready to commit.

@westonruter
8 years ago

#46 @westonruter
8 years ago

  • Keywords commit removed
  • Owner changed from westonruter to celloexpressions

@celloexpressions take a look at my new patch 22237.2.diff . What do you think about also recognizing Esc when a widget or nav menu item is expanded? This adds support for that and also improves parity between the controls for nav menu items and widgets. I noticed the former lacked the same interface as the latter.

#47 @celloexpressions
8 years ago

Good idea, I think that's a nice additional improvement. I'll test tomorrow.

@celloexpressions
8 years ago

Don't use toggleForm in expandForm, remove widget reference in comment.

#48 @celloexpressions
8 years ago

  • Keywords commit added
  • Owner changed from celloexpressions to westonruter

Found a minor issue with menus that's fixed in 22237.3.diff (was still using toggleForm in one place). Should be good to go now.

@westonruter
8 years ago

Restore params param to toggleForm

#49 @westonruter
8 years ago

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

In 37347:

Customize: Allow Esc key to collapse the currently-expanded panel, section (or control).

Pressing Esc collapses any expanded widget or nav menu item controls, or any control that implements the expanding interface. Also improves alignment between WidgetControl and MenuItemControl, adding the expanded state and associated expand/collapse methods to nav menu items.

Props purcebr, celloexpressions, westonruter.
Fixes #22237.

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


8 years ago

#51 @westonruter
7 years ago

In 39120:

Customize: Prevent collapsing expanded panel/section/control when Esc is pressed on another closable UI element.

Prevents collapsing constructs when hitting Esc on a TinyMCE toolbar, lightbox, or expanded color picker.

See #22237.
Fixes #38091.

Note: See TracTickets for help on using tickets.