WordPress.org

Make WordPress Core

Opened 7 weeks ago

Last modified 2 days ago

#52849 accepted defect (bug)

Accessibility/Application passwords: Keyboard navigation issue on user profile screen

Reported by: audrasjb Owned by: alexstine
Milestone: 5.8 Priority: normal
Severity: normal Version: 5.6
Component: Application Passwords Keywords: has-screenshots has-patch needs-refresh
Focuses: ui, accessibility Cc:

Description

Usually, when navigating inside a form using keyboard, the enter key submits the form.

Since 5.6 which implemented the Application password section in the profile screen, using enter focuses the Application password input instead of submitting the form.

Attachments (1)

052e702d231e320c137dce858a2900fe.gif (477.6 KB) - added by audrasjb 7 weeks ago.
Screenshot

Download all attachments as: .zip

Change History (19)

#1 @audrasjb
7 weeks ago

  • Focuses ui added
  • Keywords has-screenshots added

#2 @audrasjb
7 weeks ago

  • Milestone changed from Awaiting Review to 5.8

This ticket was mentioned in PR #1120 on WordPress/wordpress-develop by pramodjodhani.


7 weeks ago

  • Keywords has-patch added; needs-patch removed

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


2 weeks ago

#5 @alexstine
2 weeks ago

  • Keywords needs-testing added
  • Owner set to alexstine
  • Status changed from new to accepted

#6 @alexstine
2 weeks ago

  • Keywords needs-refresh added

I just tested the patch and it seems that it is still not working great. Here's what happened while testing in Firefox.

  1. Focus in a profile field, First Name.
  2. Type some text.
  3. Press Enter.
  4. Focus is moved to Application Passwords name field.
  5. Profile changes saved.
  6. Focus is placed back at the Application Passwords form.

We need to figure out how to disable the focus bounce to the Application Passwords section when form submit event occurs on main profile form.

It's getting better, but not quite there yet.

Also the patch needs refresh, there is a merge conflict.

Thanks.

#7 @promz
9 days ago

@alexstine I have updated the code again. I think it's working much better now.

I have replaced all submit buttons with regular <button> except the 'Update Profile' button.

Because when enter key is pressed on any input element, browser would trigger click event on all the submit buttons. This was creating unexpected behaviour as all the button click events
were fired.

#8 @alexstine
6 days ago

@promz Thanks for your reply.

After testing your latest changes, I can confirm all is working as expected. When in the main profile form, pressing Enter is just as good as selecting Update Profile. While in the Application Passwords input field, pressing Enter is just as good as clicking the Add Application Password button.

It is working fine from a screen reader perspective and I would say this greatly improves the overall UX.

The one thing left to do would be to fix the merge conflicts between your branch and upstream. Please see this guide: https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/resolving-a-merge-conflict-using-the-command-line

Thanks.

#9 @promz
6 days ago

Hello @alexstine,

Thanks for reviewing.

This is the part where I'm most confused. I only changed these 3 files:

src/js/_enqueues/admin/application-passwords.js
src/wp-admin/includes/class-wp-application-passwords-list-table.php
src/wp-admin/user-edit.php

The below files didn't touch but they are still automatically updated. Wondering if I should keep those changes or remove them:

src/wp-includes/assets/script-loader-packages.php
package-lock.json

Any help is appreciated 🙏

#10 @alexstine
6 days ago

Hello @promz,

I would just trash those changes, checkout with upstream branch.

Thanks.

#11 @promz
6 days ago

Hi @alexstine,

I think it's fine now. Please can you check again?

#12 @alexstine
6 days ago

  • Keywords needs-refresh removed

@promz I think it looks good. Let me flag this ticket for more testing and hopefully we'll get it included in 5.8.

Thanks.

#13 @audrasjb
5 days ago

  • Keywords needs-refresh added; needs-testing removed

I left a comment in the PR to replace one esc_attr with esc_html.

Otherwise, I tested the pull request and it works as expected. Thanks!

#14 @prbot
4 days ago

pramodjodhani commented on PR #1120:

@audrasjb I replaced it with esc_html__ now. I hope it's fine. Please feel free to review again.

#15 @promz
4 days ago

@audrasjb Thank you for the review. Good catch. I have updated the PR again. Please feel free to test again.

#16 @audrasjb
4 days ago

Dominik also made a point in his comment: Core translatable strings that are not going to be used in attributes don't need to be escaped at all :)

#17 @promz
4 days ago

@audrasjb okay, thanks. I will keep that in mind the next time. I have implemented the suggested changes. Please would you check again?

#18 @prbot
2 days ago

alexstine commented on PR #1120:

Hey @ocean90

Why is it that GitHub keeps showing the file as conflicted? I battle this also when I submit PRs.

src/wp-includes/assets/script-loader-packages.php

Does this happen when wp-env starts and changes the script-loader.php paths to another destination for testing?

This would obviously fall in another ticket, but wondering if there would be a pitfall to adding to .gitignore as I constantly have to keep checking this file out myself when I make PRs on wordpress-develop.

Note: See TracTickets for help on using tickets.