#52649 closed defect (bug) (fixed)
Themes admin page: provide labeling context on controls and links
Reported by: |
|
Owned by: |
|
---|---|---|---|
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.
- Make sure all links are clear and contain a theme name.
- 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)
Change History (47)
This ticket was mentioned in PR #1043 on WordPress/wordpress-develop by alexstine.
4 years ago
#2
- Keywords has-patch added
#3
@
4 years ago
- Keywords needs-testing added
To test:
- Apply the PR.
- Navigate to the Themes page in WordPress Admin.
- Test by tabbing around the page.
- 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.
4 years ago
#5
@
4 years 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.
#6
@
4 years 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
@
4 years 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
@
4 years 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:
- Theme Details
- Customize/Activate
- Live Preview
- Next theme starting from #1
Thanks.
#9
@
4 years 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
@
4 years ago
- Summary changed from Themes admin page: fix some accessibility issues to Themes admin page: provide labeling context on controls and links
#11
@
4 years 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
@
4 years 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.
#13
@
4 years 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
@
4 years 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
@
4 years 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.
#16
@
4 years 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
@
4 years 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 years ago
#19
@
4 years 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!
#21
@
4 years 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.
@
4 years ago
Screenshot with 52649-poc.patch applied. Active theme, hovered theme card, inactive theme card.
This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.
4 years ago
This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.
4 years ago
#27
@
4 years 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.
This ticket was mentioned in Slack in #design by paaljoachim. View the logs.
4 years ago
#29
@
4 years 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! :)
#30
@
4 years 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.
4 years ago
#32
@
4 years 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.
4 years ago
This ticket was mentioned in Slack in #core-test by jeffpaul. View the logs.
4 years ago
#35
@
4 years 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.
4 years ago
This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.
4 years ago
This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.
4 years ago
#39
@
4 years 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.
https://core.trac.wordpress.org/ticket/52649