Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
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.

yes

Contributor tasks needed
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

CommentFileSizeAuthor
#138 interdiff_137-138.txt2.9 KBheddn
#138 drupal-improve_password-1811240-138.patch1.81 KBheddn
#137 drupal-improve_password-1811240-137.patch2.61 KBedysmp
#136 interdiff_133-136.txt1.71 KBedysmp
#136 drupal-improve_password-1811240-136.patch1.71 KBedysmp
#133 interdiff_129-133.txt1.77 KBheddn
#133 drupal-improve_password-1811240-133.patch3.19 KBheddn
#132 nvda-screenshot-withpatch.png112.77 KBmgifford
#129 with-1811240.jpg47.04 KBheddn
#129 drupal-improve_password-1811240-129.patch3.62 KBheddn
#129 interdiff_124-129.txt2.08 KBheddn
#128 interdiff_124-128.txt2.08 KBheddn
#128 drupal-improve_password-1811240-128.patch2.62 KBheddn
#128 without-1811240.jpg50.82 KBheddn
#128 with-1811240.jpg50.9 KBheddn
#124 1811240-improve-passpord-matches-re-rolling.patch1.99 KBRavindraSingh
#118 improve-password-matches-1811240-118.patch5.26 KBpolaki_viswanath
#111 move-password-match-and-strength-1811240-111.patch5.22 KBadci_contributor
#107 change-password-voiceover.mov3.43 MBBarisW
#103 move-password-match-and-strength-1811240-103.patch5.2 KBmgifford
#101 Screen Shot 2014-10-05 at 9.29.26 AM.png204.25 KBmgifford
#100 move-password-match-and-strength-1811240-100.patch5.21 KBmgifford
#99 move-password-match-and-strength-1811240-99.patch5.19 KBmgifford
#99 Screen Shot 2014-10-05 at 8.59.32 AM.png108.53 KBmgifford
#98 move-password-match-and-strength-1811240-98.patch5.17 KBmgifford
#90 move-password-match-and-strength-1811240-90.patch5.31 KBBarisW
#90 password-improvements.png58.81 KBBarisW
#87 move-password-match-and-strength-1811240-87.patch4.54 KBmgifford
#86 move-password-match-and-strength-1811240-86.patch4.54 KBBarisW
#77 move-password-match-and-strength-1811240-76.patch4.56 KBmgifford
#76 Patch-Passwords-dont-match.png274.87 KBmgifford
#76 Patch-Paswords-match.png285.18 KBmgifford
#70 move-password-match-and-strength-1811240-70.patch4.54 KBlahoosascoots
#66 move-password-match-and-strength-1811240-66.patch4.54 KBmgifford
#61 PasswordsAndScreensavers.png437.06 KBmgifford
#60 move-password-match-and-strength-1811240-59.patch4.68 KBmgifford
#58 move-password-match-and-strength-1811240-58.patch4.75 KBmgifford
#57 move-password-match-and-strength-1811240-56.patch4.46 KBmgifford
#52 move-password-match-and-strength-1811240-52.patch4.46 KBjmuzz
#51 move-password-match-and-strength-1811240-51.patch4.43 KBjmuzz
#46 move-password-match-and-strength-1811240-45.patch2.96 KBmgifford
#43 Password_secure_edit_text.png93.89 KBmgifford
#43 Confirm_password_secure_edit_text.png100.12 KBmgifford
#43 Fair.png129.65 KBmgifford
#43 Yes_not.png70.93 KBmgifford
#40 passwd.jpg56.41 KBdcrocks
#35 move-password-match-and-strength-1811240-35.patch2.96 KBmgifford
#32 move-password-match-and-strength-1811240-32.patch2.96 KBmgifford
#29 move-password-match-and-strength-1811240-29.patch2.92 KBmgifford
#27 move-password-match-and-strength-1811240-27.patch2.9 KBmgifford
#22 move-password-match-and-strength-1811240-22b.patch2.91 KBmgifford
#19 move-password-match-and-strength-1811240-19.patch6.64 KBmgifford
#16 move-password-match-and-strength-1811240-16.patch2.81 KBmgifford
#16 PasswordStrengthAria.png194.13 KBmgifford
#10 move-password-match-and-strength-1811240-10.patch2.94 KBDevin Carlson
#8 Password_order.png189.95 KBmgifford
#7 move-password-match-and-strength-1811240-6.patch1.96 KBDevin Carlson
#4 PasswordConfirmation.png155.88 KBmgifford
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Category: bug » feature

Recategorizing as a feature, as indicated by the OP.

tim.plunkett’s picture

Title: Improve "password matches" accessibility » Improve "password matches" and "password strength" accessibility

Also, since #1811228: Improve password strength indicator accessibility is dealing with the same exact widget on the same form, let's merge the two.

mgifford’s picture

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

mgifford’s picture

FileSize
155.88 KB

Attaching screenshot.

falcon03’s picture

Hi 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).

mgifford’s picture

Issue tags: +a11ySprint

This should just be an order of operations thing. Someone needs to write the patch though.

Devin Carlson’s picture

Status: Active » Needs review
FileSize
1.96 KB

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

mgifford’s picture

FileSize
189.95 KB

We should have more cross browser testing, but from the looks of this patch we're very close to RTBC.

falcon03’s picture

Category: feature » task
Status: Needs review » Needs work

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

Devin Carlson’s picture

Status: Needs work » Needs review
FileSize
2.94 KB

Updated patch that addresses the issues described in #9.

Changes:

  • Moved password-confirm specific variables to the top of the passwordCheckMatch function as is done with the passwordCheck function.
  • Added similar <div> and <span> wrappers to the password confirm title and text as are added to the password strength title and text.
  • Added aria-live="assertive" to the password confirm text.
  • Changed the order that password strength text vs. title is printed to make the title come before the text.
YesCT’s picture

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

YesCT’s picture

Anonymous’s picture

Interesting - 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 ;)

Status: Needs review » Needs work

The last submitted patch, move-password-match-and-strength-1811240-10.patch, failed testing.

mgifford’s picture

Issue tags: +Needs tests

Tagging

mgifford’s picture

Status: Needs work » Needs review
FileSize
194.13 KB
2.81 KB

Adding 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>

Anonymous’s picture

Hi 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 ;)

mgifford’s picture

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

mgifford’s picture

Since #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.

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Accessibility, +a11ySprint

The last submitted patch, move-password-match-and-strength-1811240-19.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
2.91 KB

Let's try that again.

mgifford’s picture

falcon03’s picture

Status: Needs review » Needs work

Testing on Windows/FF/NVDA.

  1. I "password strength" indicator is still shown before the "password" field;
  2. The "password match" indicator doesn't appear at all

So to get this issue fixed, I think we need to edit the patch to that it:

  1. Moves the password strength indicator after the "password" field;
  2. (maybe) uses the new drupalAnnounce() method for ARIA live regions;
  3. creates an ARIA live region for the "password match" indicator.
mgifford’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Accessibility, -a11ySprint

Status: Needs review » Needs work
Issue tags: +Needs tests, +Accessibility, +a11ySprint

The last submitted patch, move-password-match-and-strength-1811240-22b.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
2.9 KB

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

benjifisher’s picture

mgifford’s picture

Don't think this is right either, but it's now using Drupal.announce() in this patch.

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Accessibility, +a11ySprint

The last submitted patch, move-password-match-and-strength-1811240-29.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
2.96 KB

re-roll as user.css moved to css/user.module.css

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Accessibility, +a11ySprint

The last submitted patch, move-password-match-and-strength-1811240-32.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
2.96 KB

re-roll

bowersox’s picture

Issue tags: +TwinCities
mgifford’s picture

geodaniel’s picture

dcrocks’s picture

dcrocks’s picture

FileSize
56.41 KB

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

install page

dcrocks’s picture

Status: Needs review » Needs work

Patch #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.

dcrocks’s picture

It's not clear to me. Is this patch still needed, does the problem still exist?

mgifford’s picture

Yes @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.

Password secure edit text

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

Confirm password secure edit text

VoiceOver does announce when there is a Fair or Good password.

Fair

But doesn't say anything if it doesn't match.

Yes not

Let me know if further confirmation is required.

mgifford’s picture

The last submitted patch, 35: move-password-match-and-strength-1811240-35.patch, failed testing.

mgifford’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.96 KB

Reroll

padma28’s picture

This 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

mgifford’s picture

mgifford’s picture

Ok, it should get set back to needs work. Also useful to produce a screenshot, but not a big deal on that.

Shyamala’s picture

Status: Needs review » Needs work

setting it to needs work

jmuzz’s picture

Status: Needs work » Needs review
FileSize
4.43 KB

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

jmuzz’s picture

FileSize
4.46 KB

Re-added 'assertive' argument to announce calls and added a missing translation call.

The last submitted patch, 51: move-password-match-and-strength-1811240-51.patch, failed testing.

mgifford’s picture

mgifford’s picture

mgifford’s picture

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

mgifford’s picture

didn't attach it in #56 apparently.

mgifford’s picture

More CSS alignment.

mgifford’s picture

Apparently that div was needed. I gotta get my local install up again.

mgifford’s picture

mgifford’s picture

FileSize
437.06 KB

Here's a screenshot with text for VoiceOver's feedback. It seems to work well.

The last submitted patch, 57: move-password-match-and-strength-1811240-56.patch, failed testing.

martin107’s picture

Status: Needs review » Needs work

The last submitted patch, 60: move-password-match-and-strength-1811240-59.patch, failed testing.

martin107’s picture

Issue tags: +Needs reroll
mgifford’s picture

Status: Needs work » Needs review
FileSize
4.54 KB

Hope the bot likes this.

martin107’s picture

Issue tags: -Needs reroll
mgifford’s picture

Status: Needs review » Needs work

no longer applies.

martin107’s picture

Issue tags: +Needs reroll
lahoosascoots’s picture

Status: Needs work » Needs review
FileSize
4.54 KB

Rerolled with no conflicts =]

lahoosascoots’s picture

Issue tags: -Needs reroll
mgifford’s picture

Status: Needs review » Needs work

This 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/

mgifford’s picture

Issue tags: -TwinCities +TCDrupal 2014

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 70: move-password-match-and-strength-1811240-70.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
285.18 KB
274.87 KB

Rerolled the code.. No new tests..

Here's with matching passwords:

And without:

mgifford’s picture

Forgot patch...

mgifford’s picture

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

tim.plunkett’s picture

Issue tags: +JavaScript
nod_’s picture

mgifford’s picture

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

Status: Needs review » Needs work

The last submitted patch, 77: move-password-match-and-strength-1811240-76.patch, failed testing.

mgifford’s picture

Issue tags: +Needs reroll

Needs a re-roll.

mgifford’s picture

Issue tags: +dcamsa11y
BarisW’s picture

Status: Needs work » Needs review
FileSize
4.54 KB

Here's a re-roll.

mgifford’s picture

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

mgifford’s picture

Status: Needs review » Needs work

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

BarisW’s picture

Assigned: Unassigned » BarisW

Hi Mike, mine was just a re-roll. I'm in the Amsterdam sprint now, will take this on.

BarisW’s picture

Assigned: BarisW » Unassigned
Status: Needs work » Needs review
FileSize
58.81 KB
5.31 KB

So here's a new patch. Here's the changes:

  • Both fields (password strength and password match) are now send to Drupal.announce
  • While on it, I changed two div's to span's and removed the display: inline from the css
  • I fixed the position of the confirm password
  • I set the height of the element to 0 by default to remove unneeded whitespace in the output.

I tested it with VoiceOver and that seem to work fine.

Screenshot of the changes

mgifford’s picture

LewisNyman’s picture

Issue tags: +frontend, +CSS
rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

It looks sweet! Tested in Chrome, Firefox, Safari and Opera. RTBC?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 90: move-password-match-and-strength-1811240-90.patch, failed testing.

Status: Needs work » Needs review
mgifford’s picture

Status: Needs review » Reviewed & tested by the community

setting it back to rtbc - bad bot I believe.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/user/css/user.module.css
@@ -41,9 +37,20 @@ input.password-field,
+  float: right; /* LTR */

Why are we aligning this to right? Looking at the screenshot above password match and password strength now are not in alignment.

mgifford’s picture

Status: Needs work » Needs review
FileSize
5.17 KB

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

mgifford’s picture

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

<span class="password-strength__title">Password strength: </span>
<span class="password-strength__text">Fair</span>

Is followed rather than:

<div class="password-confirm-title">
Passwords match:
<span class="password-confirm-text ok">yes</span>
</div>
mgifford’s picture

More changes in the JS.. There are now 2 spans. Sorry about the noise.

mgifford’s picture

Ok, 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.

hadi_gsf’s picture

I'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.

mgifford’s picture

Just needed a re-roll.

mgifford’s picture

I'm a bit confused as to why this all appears at the bottom of the page:

<div id="drupal-live-announce" class="visually-hidden" aria-live="assertive" aria-busy="false">Weak password</div>
</body>

Not sure if that's why it doesn't work for @hadi_gsf

BarisW’s picture

Isn't that just what Drupal.announce does? It adds a hidden container with an aria-live="assertive" attribute.

  /**
   * Builds a div element with the aria-live attribute and attaches it
   * to the DOM.
   */
  Drupal.behaviors.drupalAnnounce = {
    attach: function (context) {
      // Create only one aria-live element.
      if (!liveElement) {
        liveElement = document.createElement('div');
        liveElement.id = 'drupal-live-announce';
        liveElement.className = 'visually-hidden';
        liveElement.setAttribute('aria-live', 'polite');
        liveElement.setAttribute('aria-busy', 'false');
        document.body.appendChild(liveElement);
      }
    }
  };
mgifford’s picture

I'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.

BarisW’s picture

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

mgifford’s picture

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

YesCT’s picture

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

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
adci_contributor’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.22 KB
YesCT’s picture

Issue summary: View changes

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

mgifford’s picture

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

BarisW’s picture

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

mgifford’s picture

Issue summary: View changes
mgifford’s picture

Well, 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?

idebr’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch 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

polaki_viswanath’s picture

Status: Needs work » Needs review
FileSize
5.26 KB

Rerolled

Anonymous’s picture

Issue tags: -Needs reroll

Status: Needs review » Needs work

The last submitted patch, 118: improve-password-matches-1811240-118.patch, failed testing.

mgifford’s picture

droplet’s picture

Please also fix following code standard issue while reroll a new patch.

Thanks

  1. +++ b/core/modules/user/user.js
    @@ -13,33 +13,37 @@
    +        var confirmInput = outerWrapper.find('input.password-confirm');
    

    prefix with $ > $confirmInput

  2. +++ b/core/modules/user/user.js
    @@ -13,33 +13,37 @@
    +        var confirmResult = outerWrapper.find('div.password-confirm');
    +        var confirmChild = confirmResult.find('span.password-confirm__text');
    

    prefix with $

  3. +++ b/core/modules/user/user.js
    @@ -13,33 +13,37 @@
    +        if (confirmChild.html() != translate['confirm' + (success ? 'Success' : 'Failure')]) {
    

    should be `!==`

RavindraSingh’s picture

Status: Needs work » Needs review
FileSize
1.99 KB

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

edysmp’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll

Moving back to needs work to add tests. Fixing tags. Working on screenshots...

+++ b/core/modules/user/css/user.module.css
@@ -19,3 +15,12 @@
\ No newline at end of file

missing new line.

edysmp’s picture

I try to apply path, there are errors in javascript(Uncaught DrupalBehaviorError: attach ; password: outerWrapper is not defined). It is not possible to screenshot.

nod_’s picture

Need reroll because of we added JSDoc everywhere.

heddn’s picture

Issue summary: View changes
FileSize
50.9 KB
50.82 KB
2.62 KB
2.08 KB

No tests yet, but here's a fix of the javascript error and some styling fixes.

Before patch:

After patch:

heddn’s picture

Issue summary: View changes
FileSize
2.08 KB
3.62 KB
47.04 KB

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

heddn’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

Since this a JS only change, are there options for creating tests here? Or should that tag be removed? Removing for now. Ready for reviews.

heddn’s picture

Issue summary: View changes
mgifford’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
112.77 KB

Thanks @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.

heddn’s picture

Status: Needs work » Needs review
FileSize
3.19 KB
1.77 KB

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

hrezaei’s picture

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

mgifford’s picture

Status: Needs review » Needs work

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

edysmp’s picture

Status: Needs work » Needs review
FileSize
1.71 KB
1.71 KB

Address feedback #134 #135

edysmp’s picture

FileSize
2.61 KB

Good interdiff in #136, bad patch, new patch here.

heddn’s picture

Issue tags: -frontend, -CSS
FileSize
1.81 KB
2.9 KB

I'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.

The last submitted patch, 136: drupal-improve_password-1811240-136.patch, failed testing.

mgifford’s picture

Hmm, 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.

hrezaei’s picture

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

mgifford’s picture

Using 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

jessebeach’s picture

I hear the correct updates and no duplication in VoiceOver on a Mac in Chrome. +1

heddn’s picture

Status: Needs review » Reviewed & tested by the community

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

BarisW’s picture

I'm not sure if you can RTBC your own patch. If not, I'd be happy to do so. Let's get this in!

Anonymous’s picture

Long time listener, first time caller. Just fired up a fresh d8dev environment, patch applies nicely, movement sounds good!

heddn’s picture

re: #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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Accessibility improvements are prioritised changes during beta. Committed d519a21 and pushed to 8.0.x. Thanks!

  • alexpott committed d519a21 on
    Issue #1811240 by mgifford, heddn, BarisW, edysmp, Devin Carlson, jmuzz...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.