Application Passwords: Accessibility Improvements

[georgestephanis]

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.

WIP improvements mitigating feedback for Application Passwords.

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

[georgestephanis]

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)"

[georgestephanis]

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.

[TimothyBlynJacobs]

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.

[georgestephanis]

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.

[TimothyBlynJacobs]

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.

[georgestephanis]

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?

[georgestephanis]

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.

[TimothyBlynJacobs]

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?

[georgestephanis]

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.

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.

[georgestephanis]

Addressed concerns are addressed

[afercia]

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.

[afercia]

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

[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?

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

[ocean90]

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].

[georgestephanis]

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.

[TimothyBlynJacobs]

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.

Fixed in 5e31ccfee21547844d889e3475ecf34177874983.

[sabernhardt]

​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.

[sabernhardt]

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)

[afercia]

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.

[TimothyBlynJacobs]

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?

[afercia]

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".

[TimothyBlynJacobs]

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.

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

[audrasjb]

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

[sabernhardt]

missing input from auth-app script

[sabernhardt]

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>',
				);

[TimothyBlynJacobs]

@sabernhardt Are you testing the latest PR?

[sabernhardt]

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

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

[TimothyBlynJacobs]

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.

[kebbet]

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.

[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.

Thoughts?

Thanks.

[SergeyBiryukov]

In 49562:

I18N: Add trailing punctuation to some translator comments.

Follow-up to [49549].

See #51580.

[TimothyBlynJacobs]

@SergeyBiryukov Patch looks good to me.

[SergeyBiryukov]

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 :)

[TimothyBlynJacobs]

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

[SergeyBiryukov]

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

[kebbet]

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

[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.

[TimothyBlynJacobs]

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.

[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?

[alexstine]

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.