WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 7 weeks ago

Last modified 7 weeks ago

#52649 closed defect (bug) (fixed)

Themes admin page: provide labeling context on controls and links

Reported by: alexstine Owned by: alexstine
Milestone: 5.8 Priority: normal
Severity: normal Version: 5.7
Component: Themes Keywords: has-patch has-screenshots commit
Focuses: accessibility, administration Cc:

Description

Fix up some accessibility issues on the Themes admin page.

  1. Make sure all links are clear and contain a theme name.
  2. Render Theme Details as a button that can have focus.

These are just some ideas that I think will greatly improve the page, open to hear feedback.

Attachments (6)

52649.2.diff (8.4 KB) - added by joedolson 5 months ago.
Patch with focus handling.
52649-poc.patch (14.2 KB) - added by poena 5 months ago.
POC: Position details, activate and preview below screenshot
wordpress-themes-20210415-52649-poc.png (389.4 KB) - added by joedolson 3 months ago.
Screenshot with 52649-poc.patch applied. Active theme, hovered theme card, inactive theme card.
wordpress-themes-20210415.png (365.0 KB) - added by joedolson 3 months ago.
Themes in current trunk. Active theme, hovered theme card, inactive theme.
design-review-1.jpg (430.3 KB) - added by critterverse 3 months ago.
Here's a comp of the revisions I proposed in case that's helpful for design review.
52649.3.diff (8.9 KB) - added by joedolson 8 weeks ago.
Adds aria-label on the Delete button.

Download all attachments as: .zip

Change History (47)

#1 @alexstine
5 months ago

  • Focuses multisite removed
  • Status changed from assigned to accepted

This ticket was mentioned in PR #1043 on WordPress/wordpress-develop by alexstine.


5 months ago

  • Keywords has-patch added

#3 @alexstine
5 months ago

  • Keywords needs-testing added

To test:

  1. Apply the PR.
  2. Navigate to the Themes page in WordPress Admin.
  3. Test by tabbing around the page.
  4. Start a screen reader such as NV Access or Voiceover and test tabbing around the page. Listen to see if all buttons and links are clearer now. E.g. instead of hearing "Live Preview", you should hear "Live Preview theme_name".

Also open to feedback on if others feel this is a good idea or not, there may be a reason the page was how it was.

Thanks!

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


5 months ago

#5 @poena
5 months ago

Tested with NVDA and Chrome version 88.0.4324.190, Windows 10.

The code looks good to me.

The context is clearer, but for inactive themes, I need to press tab 4 times.

When I press tab and focus lands on the theme div, I hear:
"clickable View Theme Details for Twenty Eleven button
Twenty Eleven heading level 2 Activate Twenty Eleven link Live Preview Twenty Eleven"

When I press tab again, nothing is announced and I can no longer see where the focus is. There is no visible focus on the theme div, activation or preview link.

Next tab: "Activate Twenty Eleven link".
There is a visible border on the activation link.

Next tab: "Live Preview Twenty Eleven link"
There is a visible border on the preview link.

For the active theme, I need to press tab 3 times, and it behaves the same with a silent tab with no visual focus between the theme div and the customize link.

Last edited 5 months ago by poena (previous) (diff)

#6 @alexstine
5 months ago

@poena Just pushed another update. I've removed tabindex and aria-describedby from the parent div. Now the tab order feels a lot better. Agree or disagree?

Thanks.

#7 @poena
5 months ago

Theme admin page:
There is no visible focus indicator on the first tab.

The span with the text "Theme Details" is not visible when the theme div is focused, sighted keyboard users won't know how to view the theme details.

The change to the JavaScript also affects the add themes page:
It seems that the theme div is no longer focusable.
"Details & Preview" is not announced by the screen reader.

When I press tab, the focus order is:
install theme link
preview theme button
install theme link, for the next theme.

#8 @alexstine
5 months ago

@poena How is it now? I converted the spans to real buttons, there may be visual indication now. If not, may need to get a CSS expert to help me with the rest of this because from a screen reader perspective it's working perfectly for me.

Tab order:

  1. Theme Details
  2. Customize/Activate
  3. Live Preview
  4. Next theme starting from #1

Thanks.

#9 @johnjamesjacoby
5 months ago

@poena your feedback here is amazing.

If you’re comfortable making recommendations on the necessary changes to make this page maximally accessible, I for one would love to hear them.

For example, should the theme div actually be focusable? Or even “it’s all bad right now and here’s how it should work” because lots of the multisite admin screens need attention.

I’d love-love to see initiatives like this ticket here be clearly guided by the necessary accessibility improvements, to help reduce the amount of trial & error.

But if you’re both enjoying yourselves and making good progress, don’t let me stop you! 😅

#10 @joedolson
5 months ago

  • Summary changed from Themes admin page: fix some accessibility issues to Themes admin page: provide labeling context on controls and links

#11 @poena
5 months ago

I am sure we can improve the page for 5.8.
Reviewing and redoing the multipage admin screens is a massive task compared to only adding the context.
I would say it is worth doing but needs careful planning, time and attention. :)

I would prefer if the theme details was a plain link instead of the current overlay.

The theme div does not need to be focusable as long as the theme details, Customize/Activate and
Live Preview can be easily accessed by all users.

#12 @joedolson
5 months ago

Looks good! In testing, I found that the removal of tabindex on the .themes div caused two issues: CSS attached to .theme:focus was broken, and the collapse function was no longer able to send focus back to the closed theme.

This patch changes .theme:focus to .theme.focus and sends focus to the theme details button on collapse.

@joedolson
5 months ago

Patch with focus handling.

#13 @joedolson
5 months ago

This time, the patch was run using SVN. Oddly enough, if you generate your patch using NPM, you don't get the right patch...

#14 @alexstine
5 months ago

Just tested your patch @joedolson and it's still working fine. Focus order still works for screen reader. Theme Details, Activate/Customize, Live Preview.

#15 @poena
5 months ago

With patch 52649.2.diff, the Add themes screen is unaffected (which is what we want for now).

On the theme admin screen, the theme details button needs some additional styling to override browser specific styles for the button element.
-The position and size does not match the design before the patch, and it has a visible focus style that is inconsistent with the rest of the UI.

What if we instead moved the theme details, activate and preview below the theme name?
Reduce complexity and let these 3 links/buttons be visible at all times and not only on hover and focus, and without obscuring the theme screenshot.

Making space for information below the screenshot would also be needed for further improvements of the theme pages, like displaying the date of the last update, version, and star rating.

Last edited 5 months ago by poena (previous) (diff)

@poena
5 months ago

POC: Position details, activate and preview below screenshot

#16 @alexstine
5 months ago

@poena I gave your patch a test and this works better in the way that the Theme Details button is now below the main heading. This works better if navigating in browse mode rather than focus/edit mode where these details don't matter at much.

#17 @joedolson
4 months ago

  • Keywords needs-design-feedback needs-refresh needs-screenshots added
  • Milestone changed from Future Release to 5.8

I'd like to try and get this into 5.8, but we should probably get some design feedback first. I think this is a better workflow, and definitely improves some accessibility issues.

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


4 months ago

#19 @paaljoachim
4 months ago

Ticket was brought up during a design triage.
We need a before and after screenshot to get an idea of what needs design feedback.
Thanks!

#20 @poena
3 months ago

I am afraid I don't follow which one of the patches we need screenshots of.

#21 @joedolson
3 months ago

I think there are only intentional visual changes in your patch, @poena, so I think that's what we need screenshots for.

Attaching screenshots shortly.

@joedolson
3 months ago

Screenshot with 52649-poc.patch applied. Active theme, hovered theme card, inactive theme card.

@joedolson
3 months ago

Themes in current trunk. Active theme, hovered theme card, inactive theme.

#22 @joedolson
3 months ago

  • Keywords has-screenshots added; needs-screenshots removed

#23 @joedolson
3 months ago

  • Keywords needs-refresh removed

#24 @joedolson
3 months ago

Patch applied without error; not sure why needs refresh was there.

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


3 months ago

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


3 months ago

#27 @critterverse
3 months ago

Hi all, this is such a great improvement for the Theme admin page. I have a few design notes — sorry I’m not able to share this in a patch yet but I’m hoping to figure out how to do that in the future!

I love how this solution eliminates the horizontal overlap that happens on smaller viewport sizes now that the text and buttons have a lot more breathing room.

However, having all 3 buttons visible at all times (rather than only on hover and focus) is very overwhelming when seeing a bunch of these theme tiles together on the admin page. So I think we should reconsider this aspect.

What if only the bar containing the theme title was visible at first, and the second bar containing the 3 buttons appeared below it on hover and focus? The second bar could slide in from the bottom and push the title bar upwards.

I think it would also help to separate the buttons so that Theme Details is anchored to the left side, and Customize or Activate/Live Preview are right-aligned.

One consideration with this direction is that on mobile, we may want to make the Customize button for the active theme appear on the right side of the title bar so that it's persistently visible.

Last edited 3 months ago by critterverse (previous) (diff)

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


3 months ago

#29 @tomjdevisser
3 months ago

As discussed during the Tuesday Design triage:

I think the spacing around the buttons vs the spacing around the text looks a little awkward, but I do think the idea is an improvement.

When that's a bit more balanced, I like this adjustment!

Another solution would be to make the top or bottom bar a bit lighter/darker, so they are more clearly seperated like on the lighter boxes. Then it will be more clear the buttons have their own small container row.

Both would work, no preference here! :)

@critterverse
3 months ago

Here's a comp of the revisions I proposed in case that's helpful for design review.

#30 @johnjamesjacoby
3 months ago

I like @critterverse’s suggestion of hiding/showing the buttons, because I think it looks nice and feels intentional. It’s a bit outside the scope of the original issue, and the behavior change should be reviewed for accessibility, but my preference would be to pursue something in that direction?

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


3 months ago

#32 @joedolson
3 months ago

  • Keywords needs-refresh added; needs-design-feedback removed
  • Type changed from enhancement to defect (bug)

On discussion in the weekly accessibility meeting, we agreed that this ticket has expanded beyond it's original scope. We'll move design change considerations to a new ticket, and iterate on the original scope as represented by 52649.2.diff. Those changes are all clear accessibility improvements, and should be prioritized as such.

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


2 months ago

This ticket was mentioned in Slack in #core-test by jeffpaul. View the logs.


2 months ago

#35 @JeffPaul
2 months ago

@joedolson we just reviewed this in the test scrub for 5.8 and I'm a little confused with you re-applying the needs-refresh keyword if the design changes are moved to a different ticket. It would seem like going back to @poena's comment https://core.trac.wordpress.org/ticket/52649#comment:15 and validating whether 52649.2.diff or 52649-poc.patch is the best approach and then either refreshing the best patch or moving it along for needs-testing seems like what's needed. However I wasn't in the weekly accessibility meeting so I'm not certain if there were changes decided upon there that aren't listed in this ticket, so I'm hoping you can provide details on what's needed in the refresh?

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


2 months ago

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


2 months ago

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


8 weeks ago

@joedolson
8 weeks ago

Adds aria-label on the Delete button.

#39 @joedolson
8 weeks ago

  • Keywords commit added; needs-testing needs-refresh removed

I've checked over the patch, and made a small modification to add an additional aria-label on the Delete button. While Delete is in a theme details modal, so the context is available, I think having confirmation immediately before taking a destructive action is beneficial to the user. Otherwise, I think this is good.

#40 @joedolson
7 weeks ago

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

In 51083:

Themes: Fix accessibility issues with controls in themes screen.

Add accessible names to several theme controls so provide better context for screen reader users. Change theme details element into a button that can receive focus. Ensure focus is set back on existing theme when theme details modal is closed.

props alexstine, poena.
Fixes #52649.

#41 @joedolson
7 weeks ago

See #53356 for continuing issues on the design of the theme admin screen.

Note: See TracTickets for help on using tickets.