Beta phase evaluation
Issue category | Task because this is adding accessiblity functionality for accessiblity purposes. |
---|---|
Issue priority | Major because it improves the accessibility of the login page, which for many will be the initial experience with Drupal |
Prioritized changes | The main goal of this accessibility |
Problem/Motivation
"password matches" and "password strength" accessibility could be better. Much of the problem is that to screen readers, the content is not presented in a consistent logical fashion. Furthermore, changes to the page (password strength & password confirmation) are not passed along to the screen reader effectively via ARIA.
Proposed resolution
"password matches" indicator (label followed up by the value) should be placed after the "confirm password" field, not before.
In addition to this (I don't know if this is possible this is possible) it would be great that when a user is typing into the "confirm password" field he/she could ear when password and "confirm password" field matches, as it happens for value changes of the "password strength" indicator.
These are really important for drupal accessibility!
Remaining tasks
Find out if the improvements are possible.
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Update the issue summary | Instructions | yes | |
Update the issue summary noting if allowed during the beta | Instructions | ||
Add automated tests | Instructions | we need more | |
Manually test the patch | Novice | Instructions | |
Add steps to reproduce the issue | Novice | Instructions | |
Embed before and after screenshots in the issue summary | Novice | Instructions | |
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions |
User interface changes
None to sighted users.
API changes
None
Original report by @falcon03
Comment | File | Size | Author |
---|---|---|---|
#138 | interdiff_137-138.txt | 2.9 KB | heddn |
#138 | drupal-improve_password-1811240-138.patch | 1.81 KB | heddn |
Comments
Comment #1
tim.plunkettRecategorizing as a feature, as indicated by the OP.
Comment #2
tim.plunkettAlso, since #1811228: Improve password strength indicator accessibility is dealing with the same exact widget on the same form, let's merge the two.
Comment #3
mgiffordJust wanted to confirm that the order of the elements does not follow how they are visually presented. It would be more consistent for sure to switch the order so that the confirmation (and strength for that matter) is displayed after the password form rather than before it.
With the "confirm password" field, are you looking for a more assertive ARIA? Do you have other examples we could look at to see where this is done better?
Comment #4
mgiffordAttaching screenshot.
Comment #5
falcon03 CreditAttribution: falcon03 commentedHi mgifford,
with "confirm password" I am referring to the field where you can enter the chosen password again (for confirmation).
Also, I would like to remember that (differently from what happens with the "password strength" indicator) the "password matches" indicator value is shown before the label (and both them are shown before the "confirm password" field).
Comment #6
mgiffordThis should just be an order of operations thing. Someone needs to write the patch though.
Comment #7
Devin Carlson CreditAttribution: Devin Carlson commentedA patch to change the markup order from adding the password strength indicator and passwords confirm text above the field to below the field.
Some margins had to be removed because the original placement of the text (above the fields) required some padding to move the text inline with the actual fields.
Comment #8
mgiffordWe should have more cross browser testing, but from the looks of this patch we're very close to RTBC.
Comment #9
falcon03 CreditAttribution: falcon03 commented@Devin Carlson: ok, the "confirm password" indicator is placed after the "confirm password" field. Very good.
Since
http://drupal.org/node/1811228
has been closed as a duplicate of this issue, this patch still needs to address a problem from that issue:
the password strength indicator value should be placed after the "password strength" label, not before.
And, finally, just a wish: could we emulate the password strength indicator value behavior for the password match indicator value, so that screen readers will read the new value on change?
Btw, since this is only refactoring, I am turning this issue into a task, so that feature freeze won't block it...
Comment #10
Devin Carlson CreditAttribution: Devin Carlson commentedUpdated patch that addresses the issues described in #9.
Changes:
passwordCheckMatch
function as is done with thepasswordCheck
function.<div>
and<span>
wrappers to the password confirm title and text as are added to the password strength title and text.aria-live="assertive"
to the password confirm text.Comment #11
YesCT CreditAttribution: YesCT commentedslightly related: #432962: Add option to disable password strength checking
which found related: #1882474: Password strength indicator display broken in install process
might need a reroll (if it does and someone new wants to try, here is a reroll doc: http://drupal.org/patch/reroll)
Comment #12
YesCT CreditAttribution: YesCT commented#10: move-password-match-and-strength-1811240-10.patch queued for re-testing.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedInteresting - I'm not sure without checking how this is affecting the work myself and @dags have been doing on #432962: Add option to disable password strength checking but FYI we've split the password strength indicator and the password matches functionality into two functions now, they were part of the same thing before. Will have a look when I get a mo (i.e. when not replying to emails I get at 3.50am ;)
Comment #15
mgiffordTagging
Comment #16
mgiffordAdding a new patch, but want to ask that the same aria tool be added to the Passwords match so that the user is alerted when no=yes ->
<span class="error">no</span>
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedHi again. I've just posted an updated patch for #432962-70: Add option to disable password strength checking which seems to be on the same code you're working on.
Did mention previously - not sure best approach, I guess depends on which one gets committed first? I don't think there's much of an issue, just that code is moving around so thought I'd give a heads up.
If this is not an issue for you then fine, thought better out than in ;)
Comment #18
mgiffordThanks Steve. Really appreciate knowing that. I'm mostly working on just pushing ahead the patch. I'm not a JS guy. Couldn't even add the "password matches" piece with my level of JS knowledge.
Not sure that it would speed up either of the issues to try to merge them. I'm always happy for suggestions and will keep an eye on the disable password strength checking code.
Comment #19
mgiffordSince #432962-98: Add option to disable password strength checking is now RTBC, I'm just re-rolling #16 to accommodate. If that patch gets in, we should be good.
Comment #20
mgifford#19: move-password-match-and-strength-1811240-19.patch queued for re-testing.
Comment #22
mgiffordLet's try that again.
Comment #23
mgifford#22: move-password-match-and-strength-1811240-22b.patch queued for re-testing.
Comment #24
falcon03 CreditAttribution: falcon03 commentedTesting on Windows/FF/NVDA.
So to get this issue fixed, I think we need to edit the patch to that it:
Comment #25
mgifford#22: move-password-match-and-strength-1811240-22b.patch queued for re-testing.
Comment #27
mgiffordJust a re-roll.
I do think the password strength indicator is showing up after the DOM properly.. Not sure.. The aria-live is there I think.
However, haven't looked at drupalAnnounce(). But the password match indicator just doesn't seem to appear.
Comment #28
benjifisher#27: move-password-match-and-strength-1811240-27.patch queued for re-testing.
Comment #29
mgiffordDon't think this is right either, but it's now using Drupal.announce() in this patch.
Comment #30
mgifford#29: move-password-match-and-strength-1811240-29.patch queued for re-testing.
Comment #32
mgiffordre-roll as user.css moved to css/user.module.css
Comment #33
mgifford#32: move-password-match-and-strength-1811240-32.patch queued for re-testing.
Comment #35
mgiffordre-roll
Comment #36
bowersox CreditAttribution: bowersox commentedComment #37
mgifford#35: move-password-match-and-strength-1811240-35.patch queued for re-testing.
Comment #38
geodaniel CreditAttribution: geodaniel commentedRelated issue: #2099301: Password strength indicator position broken during install
Comment #39
dcrocks CreditAttribution: dcrocks commentedAlso #2100509: Password strength indicator for site maintenance account is aligned incorrectly on the installation screen and #2044889: 'Password Match' alignment error during install, one of which may get closed as dup.
Comment #40
dcrocks CreditAttribution: dcrocks commentedInstalled patch in #35 on a D8 cloned from git. The appearance is pretty bad(OS X and FF 24.0). It changes dramatically and tends to look better as the page is narrowed.
Comment #41
dcrocks CreditAttribution: dcrocks commentedPatch #1839318: Replace drupal.base.css library with normalize.css for all themes went in and there appears to be some overlap with this one. Might explain my results. Needs a reroll.
Comment #42
dcrocks CreditAttribution: dcrocks commentedIt's not clear to me. Is this patch still needed, does the problem still exist?
Comment #43
mgiffordYes @dcrocks, this is definitely still a problem.
When I tried the last patch it was definitely broken, but at the moment this functionality is broken in Core.
So firstly, with VoiceOver in Safari, in navigating down the http://sadaf3d61e4fa7e9.s3.simplytest.me/user/1#overlay=user/1/edit everything seems to work well until you hit the Password field. At that point it doesn't speak, but VoiceOver does display what it should be saying in VoiceOver. Not sure why.
The same thing happens when you go to confirm Confirmation field. It should announce that you are on that input form, but it doesn't. The text from VoiceOver looks like it does, but it was silent (unlike every other field on that page).
VoiceOver does announce when there is a Fair or Good password.
But doesn't say anything if it doesn't match.
Let me know if further confirmation is required.
Comment #44
mgifford35: move-password-match-and-strength-1811240-35.patch queued for re-testing.
Comment #46
mgiffordReroll
Comment #47
padma28 CreditAttribution: padma28 commentedThis patch move-password-match-and-strength-1811240-45.patch is not working.
Instead of Password strength, it is showing undefined and Passwords match is also not displaying
Comment #48
mgifford46: move-password-match-and-strength-1811240-45.patch queued for re-testing.
Comment #49
mgiffordOk, it should get set back to needs work. Also useful to produce a screenshot, but not a big deal on that.
Comment #50
Shyamala CreditAttribution: Shyamala commentedsetting it to needs work
Comment #51
jmuzz CreditAttribution: jmuzz commentedI agree with replacing the aria-live tags with Drupal.announce calls because the text in the tags themselves isn't the best way to say the information being presented. The problem is that Drupal.announce doesn't output the text it's given because its purpose is to explain changes that won't necessally be represented by text on the page. Also, the announce calls should happen every time the widget changes its info, so just putting them in the page load isn't sufficient.
Here's a version of the patch that fixes the problem described in #47 as well as announcing the changes as they happen in the widget.
Comment #52
jmuzz CreditAttribution: jmuzz commentedRe-added 'assertive' argument to announce calls and added a missing translation call.
Comment #54
mgifford52: move-password-match-and-strength-1811240-52.patch queued for re-testing.
Comment #55
mgifford52: move-password-match-and-strength-1811240-52.patch queued for re-testing.
Comment #56
mgiffordThe confirmation boxes didn't seem to line up in my test. Also I do think the confirmation should all be in one line. Even if only for formatting purposes.
Comment #57
mgifforddidn't attach it in #56 apparently.
Comment #58
mgiffordMore CSS alignment.
Comment #59
mgiffordApparently that div was needed. I gotta get my local install up again.
Comment #60
mgiffordComment #61
mgiffordHere's a screenshot with text for VoiceOver's feedback. It seems to work well.
Comment #63
martin107 CreditAttribution: martin107 commented60: move-password-match-and-strength-1811240-59.patch queued for re-testing.
Comment #65
martin107 CreditAttribution: martin107 commentedComment #66
mgiffordHope the bot likes this.
Comment #67
martin107 CreditAttribution: martin107 commentedComment #68
mgiffordno longer applies.
Comment #69
martin107 CreditAttribution: martin107 commentedComment #70
lahoosascoots CreditAttribution: lahoosascoots commentedRerolled with no conflicts =]
Comment #71
lahoosascoots CreditAttribution: lahoosascoots commentedComment #72
mgiffordThis is definitely an improvement. I now get an announcement when the passwords match.
I don't get any notice for when the passwords don't match however. That might be harder to accomplish though.
It would be possible to check when the password line has the same number of characters as the confirmation line.
I don't get a notice that I have chosen a weak password and am not promted with any ideas to make it more robust.
I just did basic testing with http://www.chromevox.com/
Comment #73
mgiffordComment #76
mgiffordRerolled the code.. No new tests..
Here's with matching passwords:
And without:
Comment #77
mgiffordForgot patch...
Comment #78
mgiffordFor TCDrupal folks wanting to test this, there's an environment up here http://s51bf82f0a91a766.s2.simplytest.me/
Login, and just start the process of changing the u/1 password. Don't hit save..
Comment #79
tim.plunkettComment #80
nod_How does this relates to #2293803: Replace confirm password element with a new password element with show/hide functionality?
Comment #81
mgiffordI do think that if the order from this issue isn't respected in #2293803 we'll get the same problem. Although I'm not certain as I haven't tested that.
Comment #84
mgiffordNeeds a re-roll.
Comment #85
mgiffordComment #86
BarisW CreditAttribution: BarisW commentedHere's a re-roll.
Comment #87
mgiffordI just tested this here http://sd4ae5bf8fd014ba.s3.simplytest.me/user/1/edit
The order (placing the confirmation after the input) wasn't changed.
I'm also puzzled that I didn't hear an announcement in VoiceOver... I need to look into that more.
Comment #88
mgiffordSadly my patch messed it up more. @BarisW was better. Two things need to happen with this patch.
1) The password matches text/content should follow the input form for the confirmation password. That still isn't happening in #86 (or #87)
2) The changes of state for both the password strength and password match values should be communicated through ARIA Live.
Comment #89
BarisW CreditAttribution: BarisW commentedHi Mike, mine was just a re-roll. I'm in the Amsterdam sprint now, will take this on.
Comment #90
BarisW CreditAttribution: BarisW commentedSo here's a new patch. Here's the changes:
I tested it with VoiceOver and that seem to work fine.
Comment #91
mgiffordAlso looking at #2293803: Replace confirm password element with a new password element with show/hide functionality - which completely re-does this pattern.
Comment #92
LewisNymanComment #93
rteijeiro CreditAttribution: rteijeiro commentedIt looks sweet! Tested in Chrome, Firefox, Safari and Opera. RTBC?
Comment #96
mgiffordsetting it back to rtbc - bad bot I believe.
Comment #97
alexpottWhy are we aligning this to right? Looking at the screenshot above password match and password strength now are not in alignment.
Comment #98
mgifford@alexpott - How do you keep looking at so many issues with fresh eyes? Great skill.
Anyways, The reason that the float right was there is because it was there in the design from a year ago. It matched at the time and this issues just been re-rolling changes to that old design.
But, actually, it's even bigger than this as I don't think the .password-confirm-text css should be added here. It's no longer consistent with .password-strength-text.
Comment #99
mgiffordBut fixing that highlights other inconsistencies between password-strength__title / password-strength__text and password-confirm-title / password-confirm-text
Which should be fixed so that:
Is followed rather than:
Comment #100
mgiffordMore changes in the JS.. There are now 2 spans. Sorry about the noise.
Comment #101
mgiffordOk, attaching a screenshot. I think this patch is good now, but think there should probably be a follow-up issue. I think that "Passwords match: yes" should probably be "Passwords match: Yes" to be consistent with "Password strength: Fair" above it.
I think that's a different issue though.
Comment #102
hadi_gsf CreditAttribution: hadi_gsf commentedI've tested the last patch, the one that Mike posted. after i'm done entering the confirmation password, i can hear "Passwords match". tested with firefox, and IE. JAWS and NVDA. I didn't hear anything about the password strength though, i had to go to the browse mode to see that. The password strength wasn't announced automatically, like the pass match did.
Comment #103
mgiffordJust needed a re-roll.
Comment #104
mgiffordI'm a bit confused as to why this all appears at the bottom of the page:
Not sure if that's why it doesn't work for @hadi_gsf
Comment #105
BarisW CreditAttribution: BarisW commentedIsn't that just what
Drupal.announce
does? It adds a hidden container with anaria-live="assertive"
attribute.Comment #106
mgiffordI'm just trying to understand why there is a difference in #102.
Why would the password strength not be announced automatically, like the pass match was?
They are using the same code and show up at the bottom of the page...
@BarisW Thanks. One of these days I might have to learn JS.
Comment #107
BarisW CreditAttribution: BarisW commentedJust tested the patch in #103 and it works just as expected. I've made a screen recording to see how Voiceover behaves and it works quite good.
Comment #108
mgiffordI tried that http://simplytest.me/project/drupal/8.0.x
I can't get the password notification to come up on the confirmation box.
I had trouble with both while testing on both NVDA & ChromeVox.
Thanks for recording the movie.. I'm not sure what is different.
Comment #109
YesCT CreditAttribution: YesCT commented@mgifford are you trying to reproduce a problem in head?
The link to simplytest.me that you have in #108 is not one that will have the patch applied.
http://simplytest.me/project/drupal/8.0.x?patch[]=https://www.drupal.org...
Comment #110
mgiffordComment #111
adci_contributor CreditAttribution: adci_contributor commentedComment #112
YesCT CreditAttribution: YesCT commented@adci_contributor Thank you for the reroll.
Are you up for adding tests? I will add instructions to the issue summary for how to add automated tests. The contributor task document is worth reading.
[edit] ps. I would recommend you also get a browser plugin from http://dreditor.org that many of us contributors use. (one of the things it has is a button insert tasks, that I used to insert the tasks template into the summary)
Comment #113
mgifford@YesCT - sorry never got back to you in #109. Yes.. On D8, I'm always trying to work against head. Thanks for the link (which will need to be updated).
They come up with DREditor pretty easily. That's how generally I get to SimplyTest.me from the issue queue.
@adci_contributor - if you need help writing tests for this, I'm sure @YesCT can help direct you to the right person.
Comment #114
BarisW CreditAttribution: BarisW commentedI would be happy to write tests (or at least, try to write them). I always find it hard to determine whát to test. Can someone describe which tests are needed?
Comment #115
mgiffordComment #116
mgiffordWell, the order doesn't seem to be something that we can test for...
We might be able to test if Drupal.announce() is fired.. To identify that change.
@YesCT - any other ideas?
Comment #117
idebr CreditAttribution: idebr commentedPatch no longer applies after #1663210: Clean up css in the User module was committed:
error: patch failed: core/modules/user/css/user.module.css:1
error: core/modules/user/css/user.module.css: patch does not apply
error: patch failed: core/modules/user/user.js:13
error: core/modules/user/user.js: patch does not apply
Comment #118
polaki_viswanath CreditAttribution: polaki_viswanath commentedRerolled
Comment #119
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #122
mgiffordComment #123
droplet CreditAttribution: droplet commentedPlease also fix following code standard issue while reroll a new patch.
Thanks
prefix with $ > $confirmInput
prefix with $
should be `!==`
Comment #124
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedDo not test this patch. I am just rerolling this patch with latest API changes. still have a doubt to want to confirm first if it passes here too.
Comment #125
edysmpMoving back to needs work to add tests. Fixing tags. Working on screenshots...
missing new line.
Comment #126
edysmpI try to apply path, there are errors in javascript(Uncaught DrupalBehaviorError: attach ; password: outerWrapper is not defined). It is not possible to screenshot.
Comment #127
nod_Need reroll because of we added JSDoc everywhere.
Comment #128
heddnNo tests yet, but here's a fix of the javascript error and some styling fixes.
Before patch:
After patch:
Comment #129
heddnNoticed an issue right off in #128. Password match was wrapping to a new line. Fixed that now. Updated screenshots attached. Sorry for the noise.
With patch:
Without:
Comment #130
heddnSince this a JS only change, are there options for creating tests here? Or should that tag be removed? Removing for now. Ready for reviews.
Comment #131
heddnComment #132
mgiffordThanks @heddin - in testing with NVDA though the confirmation doesn't actually tell me if the password matches. It just repeats the information it gave me for the quality of the initial password I am trying to match. It doesn't seem to be paired properly. This screenshot shows what a blind user would hear.
I should hear that "Passwords match: Yes", but don't.
Comment #133
heddnAfter looking at the code here a little more, things were a little more out of date in the patch then I realized. This has less code and seems to work.
Comment #134
hrezaei CreditAttribution: hrezaei commentedfeedback on the latest patch:
When typing passwords, I hear this
"Strong, no"
or
"strong, yes" or. "weak, yes".
It is awesome; But I believe it'd be cool If it said like, "strong, matches: yes". or "Strong, password matches: yes" instead of just yes or no"
edit: tested with jaws. IE11 and firefox 38. both same results which is very great.
Comment #135
mgiffordAgreed that having something that gave context would be good. When I was testing with NVDA I saw something similar. "Password Strong" & "Password Match: yes" would be much less confusing that a simple "Strong, yes".
It's getting better @heddn - think you can get this in?
Comment #136
edysmpAddress feedback #134 #135
Comment #137
edysmpGood interdiff in #136, bad patch, new patch here.
Comment #138
heddnI've done a quick review of this issue from the beginning. The idea is to speak the password strength and equality to the user. This does not require any CSS changes. Nor does it appear to need Drupal.announce. Simple ARIA tags are sufficient. Latest patch tested with NVDA on Windows 7 and Firefox 38.
Comment #141
mgiffordHmm, that failed the first time I tested it on SimplyTest.me. Seems to be working now.
Great that ARIA is sufficient. I will try to test the patch later today.
Comment #142
hrezaei CreditAttribution: hrezaei commentedTested latest patch
When I enter my passwords, Here's what I get from jaws:
"Passwords match: yes. Password strength: strong. passwords match: yes"
Seems that it is repeating the password match two times.
Comment #143
mgiffordUsing ChromeVox (ok, not a good tool, but) when adding folks here /admin/people/create I never hear the Password Strength just hear a single "dot" - The Confirm password sounds correct though and is only said once.
My testing on NVDA just seems to be all mixed up. I'm going to submit an issue to their issue queue as the logic just seems wrong.
This said, I'm not sure why, after looking at the code, there would be a duplicate of Passwords match: yes. in JAWS.
EDIT: http://community.nvda-project.org/ticket/5202
Comment #144
jessebeach CreditAttribution: jessebeach as a volunteer commentedI hear the correct updates and no duplication in VoiceOver on a Mac in Chrome. +1
Comment #145
heddnCan we commit and fix/investigate the double "Passwords match: yes/no" in a follow-up? I'm going to assume we can, and move this to RTBC. If there is disagreement, feel free to move back to needs work.
Comment #146
BarisW CreditAttribution: BarisW at LimoenGroen commentedI'm not sure if you can RTBC your own patch. If not, I'd be happy to do so. Let's get this in!
Comment #147
Anonymous (not verified) CreditAttribution: Anonymous commentedLong time listener, first time caller. Just fired up a fresh d8dev environment, patch applies nicely, movement sounds good!
Comment #148
heddnre: #146, Hopefully that makes things clearer why I marked RTBC. I should have been more explicit in #145. I wasn't RTBCing for myself, rather I was doing so based on #143 & #144. With the clear caveat that we would need to open a follow-up.
Comment #149
alexpottAccessibility improvements are prioritised changes during beta. Committed d519a21 and pushed to 8.0.x. Thanks!