WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 8 weeks ago

#52849 closed defect (bug) (fixed)

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 commit
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 (2)

052e702d231e320c137dce858a2900fe.gif (477.6 KB) - added by audrasjb 4 months ago.
Screenshot
52849.diff (3.4 KB) - added by sabernhardt 8 weeks ago.

Download all attachments as: .zip

Change History (24)

#1 @audrasjb
4 months ago

  • Focuses ui added
  • Keywords has-screenshots added

#2 @audrasjb
4 months ago

  • Milestone changed from Awaiting Review to 5.8

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


4 months ago

  • Keywords has-patch added; needs-patch removed

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


3 months ago

#5 @alexstine
3 months ago

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

#6 @alexstine
3 months 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
3 months 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
3 months 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
3 months 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
3 months ago

Hello @promz,

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

Thanks.

#11 @promz
3 months ago

Hi @alexstine,

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

#12 @alexstine
3 months 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
3 months 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
3 months 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
3 months ago

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

#16 @audrasjb
3 months 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
3 months 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
3 months 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.

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


8 weeks ago

@sabernhardt
8 weeks ago

#20 @sabernhardt
8 weeks ago

  • Keywords needs-testing added; needs-refresh removed

In addition to refreshing against trunk, 52849.diff has two main edits:

  • Numbered placeholders in the printf function so the name and id attributes share %1$s
  • Consistent syntax for the "Add New Application Password" and "Revoke all application passwords" buttons (which includes the name and id attributes)
Last edited 8 weeks ago by sabernhardt (previous) (diff)

#21 @joedolson
8 weeks ago

  • Keywords commit added; needs-testing removed

#22 @joedolson
8 weeks ago

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

In 51086:

Application Passwords: Allow enter key to submit profile form.

Fix the enter key in profile form fields moving focus to the application password input instead of submitting the profile update for. Replace the submit button type used for application passwords with button type="button" and ensure that the enter key's native behavior isn't overwritten.

props audrasjb, alexstine, promz, sabernhardt.
Fixes #52849.

Note: See TracTickets for help on using tickets.