Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#52849 closed defect (bug) (fixed)

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

Reported by: audrasjb's profile audrasjb Owned by: alexstine's profile 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 3 years ago.
Screenshot
52849.diff (3.4 KB) - added by sabernhardt 3 years ago.

Download all attachments as: .zip

Change History (24)

#1 @audrasjb
3 years ago

  • Focuses ui added
  • Keywords has-screenshots added

#2 @audrasjb
3 years ago

  • Milestone changed from Awaiting Review to 5.8

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


3 years ago
#3

  • Keywords has-patch added; needs-patch removed

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


3 years ago

#5 @alexstine
3 years ago

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

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

Hello @promz,

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

Thanks.

#11 @promz
3 years ago

Hi @alexstine,

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

#12 @alexstine
3 years 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 years 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!

pramodjodhani commented on PR #1120:


3 years ago
#14

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

#15 @promz
3 years ago

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

#16 @audrasjb
3 years 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 years ago

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

alexstine commented on PR #1120:


3 years ago
#18

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.


3 years ago

@sabernhardt
3 years ago

#20 @sabernhardt
3 years 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 3 years ago by sabernhardt (previous) (diff)

#21 @joedolson
3 years ago

  • Keywords commit added; needs-testing removed

#22 @joedolson
3 years 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.