WordPress.org

Make WordPress Core

Opened 5 weeks ago

Closed 2 weeks ago

Last modified 2 weeks ago

#51580 closed defect (bug) (fixed)

Application Passwords: Accessibility Improvements

Reported by: georgestephanis Owned by: TimothyBlynJacobs
Milestone: 5.6 Priority: normal
Severity: normal Version: 5.6
Component: Login and Registration Keywords: has-patch commit
Focuses: accessibility, rest-api Cc:

Description

We've gotten some direction in Slack from @alexstine -- https://wordpress.slack.com/archives/C02RP4X03/p1602742787172500

"Add New" button should be on next line below form, not inline.
I believe the password should be wrapped in a readonly type of field so that way copying is easier.
The dismiss button does not contain screen reader text.
The "Revoke" button is not descriptive enough. Revoke what? I would make it read "Revoke (application password name here)"
Add aria-required="true" to the "New Application Password Name" field.

and a bit more context, and an aria-describedby attribute as mentioned a bit further down.

Attachments (5)

missing-input-from-auth-app.jpg (33.6 KB) - added by sabernhardt 3 weeks ago.
missing input from auth-app script
diff-51580.patch (1.2 KB) - added by alexstine 2 weeks ago.
51580.diff (1.2 KB) - added by SergeyBiryukov 2 weeks ago.
51580.2.diff (1.2 KB) - added by SergeyBiryukov 2 weeks ago.
51580.2.png (10.7 KB) - added by SergeyBiryukov 2 weeks ago.

Download all attachments as: .zip

Change History (60)

#1 @TimothyBlynJacobs
5 weeks ago

  • Component changed from General to Login and Registration
  • Focuses rest-api added

This ticket was mentioned in PR #638 on WordPress/wordpress-develop by georgestephanis.


5 weeks ago

  • Keywords has-patch added; needs-patch removed

WIP improvements mitigating feedback for Application Passwords.

Trac ticket: https://core.trac.wordpress.org/ticket/51580#ticket

#3 @georgestephanis
5 weeks ago

I'll be back to add more in on the PR, but for now it handles

"Add New" button should be on next line below form, not inline.
The dismiss button does not contain screen reader text.
The "Revoke" button is not descriptive enough. Revoke what? I would make it read "Revoke (application password name here)"

#4 @georgestephanis
5 weeks ago

On the others, a couple thoughts --

Add aria-required="true" to the "New Application Password Name" field.

As the html has this structured as being /inside/ the larger profile edit form, would adding required attributes to this cause issues with being able to submit the larger form if they had no info in the application password field?

I believe the password should be wrapped in a readonly type of field so that way copying is easier.

Does anyone have an xref for how core does this sort of thing for generated passwords? I'd like to copy that so I'm not diverging from prior art, and mishandling any browser quirks.

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


5 weeks ago

#6 @TimothyBlynJacobs
5 weeks ago

Does anyone have an xref for how core does this sort of thing for generated passwords? I'd like to copy that so I'm not diverging from prior art, and mishandling any browser quirks.

@georgestephanis There was this recently, #48463.

#7 @georgestephanis
5 weeks ago

Alright got that switched over to a readonly input field here: https://github.com/WordPress/wordpress-develop/pull/638/commits/4151dfdb194015df75565ca5c3ae8ac608cb0763

I don't have any JS tying in to it to autoselect the content on focus or a "copy" link that dumps it to the clipboard -- but we have prior art if those are worth including.

#8 @TimothyBlynJacobs
5 weeks ago

That looks good to me. We also need to update the auth app flow.

From that ticket @afercia makes the point that it makes things a lot easier for mobile users and reduces the inputs required for mouse users. I'd say let's give it a try.

#9 @georgestephanis
5 weeks ago

I've got the results updated to be input fields for the auth flow as well.

Part of me sees the "click to copy" as more of an enhancement and would kinda rather see a subsequent ticket so we're not holding this up, if it makes sense?

#10 @georgestephanis
5 weeks ago

Now, for

Add aria-required="true" to the "New Application Password Name" field.

I added this to the name field in the auth flow -- but not the user edit page, as I'm concerned it would mess with the submit-ability of the user edit form.

#11 @TimothyBlynJacobs
5 weeks ago

Part of me sees the "click to copy" as more of an enhancement and would kinda rather see a subsequent ticket so we're not holding this up, if it makes sense?

Sure, let's do that then. We can open a new issue after this is merged.

Add aria-required="true" to the "New Application Password Name" field.

I added this to the name field in the auth flow -- but not the user edit page, as I'm concerned it would mess with the submit-ability of the user edit form.

Yeah I agree. It seems like ideally we'd have our own <form> we could work in. But I don't see how that would be remotely possible on that page. Maybe we could iFrame it? That's probably a terrible idea. Maybe we add an aria-label to the name input indicating that it is required?

#12 @georgestephanis
5 weeks ago

I mean, the "add new" button just doesn't do anything and focuses back on the text input if it's left blank (0 === name.length).

https://github.com/WordPress/wordpress-develop/blob/530493396b324f5bed518a494e2843e7fdb020f1/src/js/_enqueues/admin/application-passwords.js#L22-L25

We could do some sort of wp.a11y.speak() in that conditional to state that it's required but I don't have any experience with that and would appreciate if someone else can draft a change for that aspect.

This ticket was mentioned in Slack in #core-passwords by georgestephanis. View the logs.


5 weeks ago

#14 @prbot
5 weeks ago

TimothyBJacobs commented on PR #638:

It also looks like each id for the revoke button is the same. I think we need to update them to each have a unique name/id.

We also need to update WP_Application_Passwords_List_Table::print_js_template_row() to not call column_revoke() anymore since it requires an item now. Passing the underscore template to submit_button() might work? Otherwise we'll have to build the HTML manually.

#15 @georgestephanis
5 weeks ago

Addressed concerns are addressed

This ticket was mentioned in Slack in #core-passwords by georgestephanis. View the logs.


5 weeks ago

#17 @afercia
5 weeks ago

Thanks for this ticket and the explored improvements.

Notice a couple more things:

1
The input field has no visible label and instead does have a visible placeholder attribute. This is an accessibility anti-pattern: placeholders are not replacement for labels. I'd strongly recommend to remove the placeholder attribute and make the label element visible.
Also, the label text and the placeholder text are duplicate: "New Application Password Name".

2
When using a keyboard and submitting, the form elements get disabled, including the submit button. This way, the element that had keyboard focus loses focus and there's a focus loss. ideally, interactive elements that have focus should never get disabled or removed/hidden from the DOM while they're focused. Ideally, instead of disabling, the button should get an aria-disabled="true" attribute and noop. If this isn't possible, a temporary workaround would be setting focus on the button again once the AJAX action has completed.

#18 @afercia
5 weeks ago

Quick note that at this point of the release this ticket should have an owner :) Anyone willing to own it?

#19 follow-up: @afercia
5 weeks ago

Quick question: on my VVV environment, to make the form appear I had to add define( 'WP_ENVIRONMENT_TYPE', 'local' ); in my wp-config.php file. @TimothyBlynJacobs is this expected?

Would be great to clarify this for folks willing to test this feature on trunk.

#20 in reply to: ↑ 19 @ocean90
5 weeks ago

Replying to afercia:

Quick question: on my VVV environment, to make the form appear I had to add define( 'WP_ENVIRONMENT_TYPE', 'local' ); in my wp-config.php file. @TimothyBlynJacobs is this expected?

Either that or you have to use HTTPS, see also [49139].

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


5 weeks ago

#22 @georgestephanis
5 weeks ago

  • Owner set to georgestephanis
  • Status changed from new to assigned

I've never really been clear whether owner means "person who is making the fix" or "person who will commit the fix" -- if the former tack me on. If the latter 🤷🏽‍♂️

Got the label issue fixed -- swapped the placeholder text to something more placeholder-y -- a suggestion of how to format the name.

#23 @TimothyBlynJacobs
5 weeks ago

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

In 49294:

App Passwords: Improve accessibility.

  • Make form inputs stacked instead of inline.
  • Provide a visible label for the app name.
  • Add screen reader text to dismiss button.
  • Make "Revoke" button label more descriptive.
  • Use aria-disabled instead of disabled to avoid focus loss.
  • Display password in a readonly input to assist copy and paste.
  • Remove large sections of italic text.
  • Use .form-wrap and .form-field to give consistent form styling.
  • Improve labeling and placeholder text.

Props alexstine, georgestephanis, afercia, TimothyBlynJacobs.
Fixes #51580.

#24 @prbot
5 weeks ago

TimothyBJacobs commented on PR #638:

Fixed in 5e31ccfee21547844d889e3475ecf34177874983.

#25 @sabernhardt
4 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

more discussion on Slack:

alexstine

Additional feedback:

  • Change "Add New" to "Add New Application Password".
  • The notice is read multiple times, not sure how to fix this. Maybe remove role="alert" if focus is being placed on the notice?
  • I'd still like aria-required="true". This will not effect the main form, it only speaks required to screen readers unlike the required HTML attribute which will stop the processing unless the field is filled out. In a perfect world, change password and application passwords would be on sub pages of profile page, it would be much better for accessibility. One page doesn't necessarily fit all.

timothybjacobs

Thanks for the additional testing! I can make those changes. Do you think it will be clear to screen reader users that the aria-required is only applying when creating an app password?

alexstine

I think it would be, but maybe additional feedback from accessibility team couldn't hurt. I am of the belief that self-contained forms should have these fields indicated as required, however, as I said above, it is hard to communicate sub-forms inside a main form. It doesn't make a lot of sense to a screen reader user who must explore a bit at a time like it would to someone with vision who gets a complete overview of the page.

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


4 weeks ago

#27 @sabernhardt
4 weeks ago

For a visible label on each application-password-display readonly input (both user-edit.php and authorize-application.php), I'd like the text to be the same string for both:
__( 'Your new password for %s is:' )

I haven't tested this for a proper patch, but it would be something like the following in authorize-application.php:

				<p class="application-password-display">
					<label for="application-password-display">
					<?php
					printf(
						/* translators: %s: Application name. */
						__( 'Your new password for %s is:' ),
						'<strong>' . esc_html( $app_name ) . '</strong>'
					);
					?>
					</label>
					<?php
					sprintf(
						'<input type="text" id="application-password-display" class="code" readonly="readonly" value="%s" />',
						esc_attr( WP_Application_Passwords::chunk_password( $new_password ) )
					);
					?>

(I'm sure this can be improved)

#28 @afercia
4 weeks ago

Question: has the problem related to the focus loss been solved? I still see a focus loss when using the UI.

When using a keyboard and submitting, the form elements get disabled, including the submit button. This way, the element that had keyboard focus loses focus and there's a focus loss. ideally, interactive elements that have focus should never get disabled or removed/hidden from the DOM while they're focused. Ideally, instead of disabling, the button should get an aria-disabled="true" attribute and noop. If this isn't possible, a temporary workaround would be setting focus on the button again once the AJAX action has completed.

#29 @TimothyBlynJacobs
4 weeks ago

I'm happy to change the markup to whatever. Could we get a final recommendation on that? As well as marking the field as aria-required?

Question: has the problem related to the focus loss been solved? I still see a focus loss when using the UI.

I thought I solved it. I've changed it to use aria-disabled and no-op. Based on earlier feedback, if the password is successfully created, the focus is automatically moved to the success notice. Should that no longer happen?

#30 @afercia
4 weeks ago

Re: focus: I stand corrected: yes, focus is moved to the alert notice. It's just that it doesn't have a focus style so at a first glance I was confused. The tabindex attribute value should be -1 though a not 0.

There is a focus loss when clicking the "Revoke" button though. The button gets removed together with the table row and focus is not managed. Of course, it happens also when there's only one password and on removal the whole table gets removed. In either cases, focus needs to be moved to a proper place, not sure what the best place is though.

The readonly input field doesn't have an associated <label> element. As pet the accessibility coding standards, all input fields need to have a label associated with for/id attributes. See the sample code provided by @sabernhardt above.

Regarding:

The notice is read multiple times, not sure how to fix this. Maybe remove role="alert" if focus is being placed on the notice?

The double announcement actually depends on the browser / screen reader combination being used. Setting focus will also bring the notice into view in case it isn't thus ensuring all visual users will see it. The alert role ensures screen readers will announce the notice. Using both focus and the alert role is the same technique already suggested and implemented in other places, see for example #47147 under "Remediation Guidance".

#31 @TimothyBlynJacobs
4 weeks ago

The tabindex attribute value should be -1 though a not 0.

Will update.

In either cases, focus needs to be moved to a proper place, not sure what the best place is though.

I'm not sure either. Looking at the comments table, it appears that the current row is instead hidden with display: none and the focus stays on the "Trash" link.

The readonly input field doesn't have an associated <label> element. As pet the accessibility coding standards, all input fields need to have a label associated with for/id attributes

When I looked thru core for usages of readonly="readonly" the only instance I saw with a <label> was the media manager.

See the sample code provided by @sabernhardt above.

Will implement.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 weeks ago

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


3 weeks ago

#34 @TimothyBlynJacobs
3 weeks ago

  • Owner changed from georgestephanis to TimothyBlynJacobs
  • Status changed from reopened to assigned

#36 @audrasjb
3 weeks ago

@TimothyBlynJacobs this PR looks good to me. I think it fixes all the issues discussed during today’s accessibility bug scrub. Thank you!

@sabernhardt
3 weeks ago

missing input from auth-app script

#37 @sabernhardt
3 weeks ago

The profile page seems fine.

However, the previously committed patch (PR) had %1$s and %2$s for placeholders in the auth-app.js script, and now it's missing the second. So the input does not show after approval on the Authorize Application page.

Is there a better way to write it than this?

				message = wp.i18n.sprintf(
					/* translators: %s: Application name */
					'<label for="new-application-password-value">' + wp.i18n.__( 'Your new password for %s is:' ) + '</label> <input id="new-application-password-value" type="text" class="code" readonly="readonly" value="" />',
					'<strong></strong>',
				);

#38 @TimothyBlynJacobs
3 weeks ago

@sabernhardt Are you testing the latest PR?

#39 @sabernhardt
3 weeks ago

Sorry, I missed the "be sure" update to the PR.

The input from auth-app.js is there now.

Last edited 3 weeks ago by sabernhardt (previous) (diff)

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


3 weeks ago

#41 @TimothyBlynJacobs
2 weeks ago

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

In 49549:

App Passwords: Further accessibility improvements.

  • Add a label to the readonly password input.
  • Handle focus loss after revoking app passwords.
  • Handle focus loss after dismissing notices.
  • Mark app name as aria-required.
  • Use aria-label for detailed revoke button text instead of title.
  • Use -1 for tabindex instead of 0.

Props alexstine, afercia, sabernhardt, audrasjb, joedolson, TimothyBlynJacobs.
Fixes #51580.

#42 @kebbet
2 weeks ago

Another note on the accessibility on the profile/user-page. The position of the "Update user/profile"-button is a bit hidden below the section App-passwords, especially if the table grows.. Could the button be moved to before App passwords? And a user needs to tab through the whole table if using only keyboard.

#43 follow-up: @alexstine
2 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

There are 2 more issues.

  1. The aria-describedby takes an ID, not an actual description.
  2. I have removed the placeholder text and put it in a <p> with class="description".

I am attaching my patch.

Thoughts?

Thanks.

#44 @SergeyBiryukov
2 weeks ago

In 49562:

I18N: Add trailing punctuation to some translator comments.

Follow-up to [49549].

See #51580.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


2 weeks ago

@SergeyBiryukov
2 weeks ago

#46 @TimothyBlynJacobs
2 weeks ago

@SergeyBiryukov Patch looks good to me.

#47 in reply to: ↑ 43 @SergeyBiryukov
2 weeks ago

Replying to alexstine:

There are 2 more issues.

  1. The aria-describedby takes an ID, not an actual description.
  2. I have removed the placeholder text and put it in a <p> with class="description".

I am attaching my patch.

Thanks for the patch! Some quick notes:

  • Splitting : not required to update profile into a new string is not friendly for translators, this should be a single string with the full sentence for better context.
  • esc_attr_e() should be replaced with _e(), as the string is no longer in an attribute.
  • The same placeholder is also used in wp-admin/authorize-application.php, so removing it in this one instance would be inconsistent. There are also some placeholders in other areas of core, so I'm not sure whether that's an issue, as long as the associated field is properly described.

51580.diff corrects the aria-describedby attribute.

@TimothyBlynJacobs: Thanks for the review! Do you have an opinion on removing the .screen-reader-text class here and making this description visible for everyone? I think that brings some additional clarity :)

Last edited 2 weeks ago by SergeyBiryukov (previous) (diff)

#48 @TimothyBlynJacobs
2 weeks ago

I don't have any opinion really, I'm fine with whichever.

#49 @SergeyBiryukov
2 weeks ago

51580.2.diff corrects the aria-describedby attribute and makes the description visible: 51580.2.png.

#50 @SergeyBiryukov
2 weeks ago

  • Keywords commit added

#51 @kebbet
2 weeks ago

Still the "Update Profile"-button is decoupled from the actual profile forms. Could it be moved above the "Application Passwords"-section?

Last edited 2 weeks ago by kebbet (previous) (diff)

#52 follow-up: @TimothyBlynJacobs
2 weeks ago

@kebbet I think that would require more consideration, it seems like it'd be better served by the tabbed user profile concept. Maybe something for 5.7.

#53 @TimothyBlynJacobs
2 weeks ago

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

In 49573:

App Passwords: MOAR accessibility improvements.

Corrects the "Application Name" field's aria-describedby attribute and makes the description visible.

Props alexstine, SergeyBiryukov.
Fixes #51580.

#54 in reply to: ↑ 52 ; follow-up: @kebbet
2 weeks ago

Replying to TimothyBlynJacobs:

@kebbet I think that would require more consideration, it seems like it'd be better served by the tabbed user profile concept. Maybe something for 5.7.

I kindly disagree, and I understand that we need to ship 5.6, which is great. I was not ware of that concept, is there an existing ticket for it, I can't find one when searching?

#55 in reply to: ↑ 54 @alexstine
2 weeks ago

Replying to kebbet:

Replying to TimothyBlynJacobs:

@kebbet I think that would require more consideration, it seems like it'd be better served by the tabbed user profile concept. Maybe something for 5.7.

I kindly disagree, and I understand that we need to ship 5.6, which is great. I was not ware of that concept, is there an existing ticket for it, I can't find one when searching?

Yep, let's wait for 5.7. I agree this is pretty terrible UX, but at least it works for now and can be improved later.

Thanks.

Note: See TracTickets for help on using tickets.