Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#27521 closed defect (bug) (fixed)

Theme Install Page is unusable via the keyboard

Reported by: jorbin's profile jorbin Owned by: nacin's profile nacin
Milestone: 3.9 Priority: high
Severity: major Version: 3.9
Component: Themes Keywords: has-patch commit
Focuses: accessibility, javascript Cc:

Description

While testing wp-admin/theme-install.php, navigating the page via the tab key makes the page completely unusable.

First things noted on initial testing:

.theme-navigation links are not actually links so you cannot focus on them.

When you are focused on a theme (divs with the theme class) and press tab, you are moved to the theme preview screen.

Attachments (20)

27521.diff (2.5 KB) - added by jorbin 10 years ago.
27521.2.diff (4.1 KB) - added by jorbin 10 years ago.
27521.3.diff (4.3 KB) - added by jorbin 10 years ago.
27521.4.diff (9.2 KB) - added by jorbin 10 years ago.
27521.5.diff (5.4 KB) - added by jorbin 10 years ago.
27521.6.diff (5.0 KB) - added by jorbin 10 years ago.
27521.7.diff (5.0 KB) - added by jorbin 10 years ago.
27521.8.diff (9.0 KB) - added by jorbin 10 years ago.
27521.9.diff (733 bytes) - added by helen 10 years ago.
27521.10.diff (1001 bytes) - added by matveb 10 years ago.
27521.11.diff (1.5 KB) - added by adamsilverstein 10 years ago.
Theme install preview - Disable next/prev buttons when you reach an dead end.
27521.12.diff (1.5 KB) - added by adamsilverstein 10 years ago.
Theme install preview - Disable next/prev buttons when you reach an dead end.
27521.13.diff (1.3 KB) - added by adamsilverstein 10 years ago.
corrected: Disable next/prev buttons when you reach an dead end.
27521.14.diff (1.2 KB) - added by adamsilverstein 10 years ago.
Escape to close Preview
27521.15.diff (1.2 KB) - added by adamsilverstein 10 years ago.
corrected: Escape to close Preview (removed blank line)
27521.16.diff (2.8 KB) - added by kirasong 10 years ago.
Refreshed, Cleaned up, and Combined 13 & 15.
27521.17.diff (783 bytes) - added by adamsilverstein 10 years ago.
add left/right arrow key events to install preview mode
27521.18.diff (5.6 KB) - added by matveb 10 years ago.
27521.19.diff (5.3 KB) - added by matveb 10 years ago.
typo
27521.20.diff (362 bytes) - added by adamsilverstein 10 years ago.
only navigate to previous theme ._once

Download all attachments as: .zip

Change History (41)

@jorbin
10 years ago

#1 @jorbin
10 years ago

First pass on this fixes the main issues on the overview page.

  • Filters are navigable and selectable via the keyboard
  • You can navigate from theme to theme via the keyboard
  • Global keyboard shortcuts like CMD-L actually work

@todo the theme detail page doesn't work at all right now.

@jorbin
10 years ago

#2 @jorbin
10 years ago

I still haven't touched the preview panel, bt the above patch takels much of the issues on the overview panel.

This ticket was mentioned in IRC in #wordpress-ui by nacin. View the logs.


10 years ago

@jorbin
10 years ago

@jorbin
10 years ago

@jorbin
10 years ago

@jorbin
10 years ago

@jorbin
10 years ago

@jorbin
10 years ago

#4 follow-up: @jorbin
10 years ago

The above patch ( 27521.8.diff ) corrects all the issues I know about on the theme install screen. It doesn't do anything on the theme install details page.

One change that also affects the theme screen is that the action buttons are now focusable.

#5 in reply to: ↑ 4 @helen
10 years ago

Replying to jorbin:

One change that also affects the theme screen is that the action buttons are now focusable.

YAY!

#6 @helen
10 years ago

In 27804:

Bring keyboard accessibility to the theme install screen and theme action buttons. props jorbin. see #27521.

@helen
10 years ago

#7 @helen
10 years ago

  • Keywords has-patch added

27521.9.diff brings focus into the overlay and then puts it back on close. It copies some the existing pattern from theme management, but doesn't quite do it all (_.delay() or a separate function for setting/containing focus). Not sure what the preference is here.

#8 @matveb
10 years ago

Nice — quick mention that if we switch the Preview view back to the Details view we have in themes.php (updating the templates) we should get most of the work that went into keyboard accessibility for themes.php for free. I'll be tackling that soonish after finishing the other priority items if no one gets to that first.

#9 @nacin
10 years ago

  • Priority changed from highest omg bbq to high
  • Severity changed from blocker to major

This page is much better now, and we have a plan of action for finishing it off, so I'm turning off the red.

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.


10 years ago

@matveb
10 years ago

#11 follow-up: @matveb
10 years ago

Related to this, some nice things to have:

  • Escape to close Preview / go back to sidebar if you are in iframe.
  • Disable next/prev buttons when you reach an dead end.
  • Contain focus to sidebar + preview.

@adamsilverstein
10 years ago

Theme install preview - Disable next/prev buttons when you reach an dead end.

@adamsilverstein
10 years ago

Theme install preview - Disable next/prev buttons when you reach an dead end.

@adamsilverstein
10 years ago

corrected: Disable next/prev buttons when you reach an dead end.

@adamsilverstein
10 years ago

Escape to close Preview

@adamsilverstein
10 years ago

corrected: Escape to close Preview (removed blank line)

#12 in reply to: ↑ 11 @adamsilverstein
10 years ago

Replying to matveb:

Related to this, some nice things to have:

  • Escape to close Preview / go back to sidebar if you are in iframe.
  • Disable next/prev buttons when you reach an dead end.
  • Contain focus to sidebar + preview.

Adding these incrementally for clarity; I can definitely seeing some code duplication that could be combined.

This ticket was mentioned in IRC in #wordpress-dev by DH-Shredder. View the logs.


10 years ago

@kirasong
10 years ago

Refreshed, Cleaned up, and Combined 13 & 15.

#14 @kirasong
10 years ago

Refreshed (since 27521.13.diff was no longer applying) and cleaned up 27521.15.diff and 27521.13.diff, and put them into: 27521.16.diff

Since I was testing them together, right now they're in the same patch. If a committer would rather apply them in separate commits, I can split them out again.

#15 @kirasong
10 years ago

  • Keywords commit added

#16 @nacin
10 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 28033:

Theme Installer: Disable prev/next buttons on first/last themes, add Esc handling, use IDs.

props adamsilverstein, DH-Shredder.
fixes #27521.

#17 @nacin
10 years ago

The overlay issue is not new. New ticket for post-3.9: #27705. A simple fix *could* be pulled in post RC for 3.9.

@adamsilverstein
10 years ago

add left/right arrow key events to install preview mode

#18 @nacin
10 years ago

In 28036:

Theme Installer: Left/right arrow keys in the overlay.

props adamsilverstein.
see #27521.

@matveb
10 years ago

@matveb
10 years ago

typo

#19 follow-up: @matveb
10 years ago

Use Backbone events to handle keyups on Preview. Fix bugs with the current model state persisting after you opened-navigate-close the Preview. Also hard prevents previous theme from going into a loop.

$themeInstaller could be preview.$el instead if we were to pass it to the function.

#20 @nacin
10 years ago

In 28049:

Theme installer keyboard fixes. Updates [28033] and [28036].

props matveb.
see #27521.

@adamsilverstein
10 years ago

only navigate to previous theme ._once

#21 in reply to: ↑ 19 @adamsilverstein
10 years ago

Oh, Backbone events - thats how to handle events! :)

I noticed you added ._once around the nextTheme call on key-right, it was left off for previousTheme/key-left; Unless there is a specific reason for leaving off, we may want to add that, see 27521.20.diff

Replying to matveb:

Use Backbone events to handle keyups on Preview. Fix bugs with the current model state persisting after you opened-navigate-close the Preview. Also hard prevents previous theme from going into a loop.

$themeInstaller could be preview.$el instead if we were to pass it to the function.

Note: See TracTickets for help on using tickets.