Improve password checker in the Stark theme.

Good catch! I suspect garland-oriented CSS. Subscribing for now; no time before DCDC.

The mis-alignment of the password checker seems to be due to some weird interaction with the width set on .password-parent

This looks like a good one for the novice queue.

It's a combination of the floats on the password checker and the widths of the password elements. The least intrusive fix is to clear the floats in the password description. Patch attached.

No time to test the patch here, but you must be sure the CSS for the password checker makes it look good without any theme-specific CSS (disable your theme's CSS files while theming the checker).

Applies perfectly. Attached screenshot in Stark theme. Notice the box that shows the password strength is a little mis-centered vertically, that's why I'm not settings this to RTBC. Since this is way better than without the patch, though, it might as well be committed.

Dove into the CSS (system.css) a little and saw some weird things. Now I am no CSS guru but the markup just a little over-complicated to me. I've attached a patch which fixes the misplaced password checker on the outside and makes things more logical (to me) in the code.
Since this is really my first core patch, I really appreciate any help or comments whether I'm heading in the wrong direction, etc.
Oh, and bot: please don't fail me!

edit: only tested in FF3!

This begs the question... Is the issue with Drupal core or with the Stark theme? I hesitated to add CSS to Stark because it seems that it should have as little CSS as possible. But a change to the core css could trigger issues with other themes as well.

Well Drupal's core css should be as clean as possible, which is easily tested with the Stark theme. The password selector to my mind didn't have a very clean markup at all, that's why I think it correct to change that (see my patch).
You're right, though, that I should have checked the other core themes, whether the patch breaks them.
I'll do that in a minute...

Re-added the margin that was superfluous in Stark back to Garland. Now works perfectly in all 3 core themes.
Again only tested in FF3.

Code in patch looks good on first inspection

Patch applies, but with offset.

patching file modules/system/system.css
Hunk #1 succeeded at 521 (offset 5 lines).
Hunk #2 succeeded at 541 (offset 5 lines).
Hunk #3 succeeded at 564 (offset 5 lines).
patching file themes/garland/style.css
T2:d7head_testing joe$ 

Tested Stark theme, and working in Safari 4, FF3, IE7 and IE6. Also tested in IE5.5, which didn't work at all (password checker is broken completely), but after some discussion in IRC decided that that doesn't really matter to much.

See before and after screenshots.

I think this section that gets added to garland/style.css

#password-indicator {
  margin: 0.3em 1.3em 0 0;
}

should be

#password-indicator {
  margin: 0.3em 0.8em 0 0.3em;
}

Which is what was removed from system.css. The CSS in the patch causes the password strength widget to not line up with right edge of the password input field when using Garland, and in IE6 causes the widget to bump up right next to the text "Password strength:" which looks a little awkward.

The attached patch fixes this.

Not sure if it's a deal breaker or not, but resizing your browser to be really narrow causes some weirdness with the confirm password input field wrapping below the password strength checker while the label for the field stays above. This only happens in Stark. Screenshot attached.

Sorry to be annoying, but this issues is going to be hurt by #325877: Password checker breaks if no description. The markup has to change in order for the suggestions box to appear on the installer, which means the CSS needs to change too. Hopefully the CSS changes in the other issue will also correct some of the problems with the stark theme.

I have spent hours wrestling with the CSS to get it looking good across browsers, themes and browser-defined-text-sizes and finally managed to get it so it doesn't break, but can't make it look perfectly aligned in every browser. I'm hoping we can fix that.

I apologise for not bringing that other issue up earlier, but I took a hiatus from the issue queue for a few months and didn't know this issue existed.

IMO, floats should be avoided for the default styles attached the module. Let the indicator and label for the password strength be on their own line. I'd take this further to all module styles. Just give a consistent set of classes and make it completely bare leaving it up to the theme to style it in specific ways.

I'm tired of core or any module making one off design decisions. Although this will make more work for themers with less experience, it'll make it easier for those who need to make everything consistent in their themes.

I'd have less issue with styling these forms if they were consistent to begin with. In some situations, it really can't be done but it should be avoided when possible.

@dvessel: Could you post a screenshot of what you propose?

Something like this. It's not pretty but it doesn't need to be. As long as it doesn't look like it's obviously broke.

It always breaks my heart to intentionally make Drupal less beautiful...

...but you really do have a point. Stark is Stark after all.

+1 on #15 (will roll a patch once I can get my dev-server to work, if someone doesn't beat me to it)

Here we go.

Attached some screenshots.
Notes:
pass-check-nojs.png: The space above the description of the password fields is inconsistent to the other description fields.
This is because the password form is really two forms. To change this would either include a dirty hack or include the description in one of the forms which would kill flexibility for custom themes.

pass-check-js-empty.png: As you can see in pass-check-js-filled.png the empty space above the suggestions is caused by the hidden match validator. To remove this space would mean the whole page shifts once you input something in the second form. An option would be to show the "Passwords match:" part from the beginning and then just append "Yes" or "No" once you enter something.

This patch alters Garland's output. If this patch is approved I will add the styles back to Garland's CSS.

I fail to see how this is postponed.
This is about Stark.
#325877: Password checker breaks if no description is about Garland.

I'm delegating this patch to dvessel. I'll review and commit it when dvessel thinks it is ready to be committed.

tstoeckler, I think we can improve this further. I'd have to examine it further to give any meaningful input and I have little time at the moment.

One thing we must ensure is that it looks good in Garland.

Since, apart from 1 line, I only deleted lines, it's quite trivial to make it look good (= like it looked before) in Garland, by simply copying them to themes/garland/style.css

Awaiting your review, then ;)

This may seem out of scope but since we are focused on the password checker I think we should do this.

The notes on what makes a good password should not pop in and out. (It currently reveals itself when the relevant field is in focus.) It should always be present and it should be part of the html markup. Not passed through javascript. Same with the "Passwords match: *" text under the confirm input element.

What's even worse is how each item list just disappears. They should inform the user on what they did right. It give reassurance too. As long as it's worded right, it shouldn't matter whether javascript is enabled or disabled.

What can be done with the javascript is to add classes to the description text so it can stand out. For the confirm input, the text can be swapped in addition to adding classes.

I attached a screen shot with notes in how it might look in both Stark and Garland. See the Garland image which illustrates how it would function. Stark would benefit too by preventing the odd gaps caused by these boxes from jumping in and out of view.

Since this is your issue, I guess you get to decide what's out of scope?!

I very much like your proposal. As you can see from my #17 I was also unhappy with the current approach of things dis/appearing magically and randomly. The weirdest thing, about the magically appearing notes for password strength is, is that once you focused the password field, it won't go away, even if you focus something else...

Anyway, I'll have a go at this in the next few days, I really liked those screenshots...

Since this is your issue, I guess you get to decide what's out of scope?!

Not really and it's not my issue. :) But placing a band-aid on what looks to be more than a cosmetic problem didn't sit right.

I very much like your proposal. As you can see from my #17 I was also unhappy with the current approach of things dis/appearing magically and randomly. The weirdest thing, about the magically appearing notes for password strength is, is that once you focused the password field, it won't go away, even if you focus something else...

We have things popping into view in other forms which is perfectly acceptable (date formatter and hierarchy select in book outlines for example) but there's no other place in core where the descriptive text starts playing hide and seek. It's jarring and not needed. In this case, like you mentioned #17 creates some weird spacing.

Semi-related to #432962: Add option to disable password strength checking

I fail to see how this is postponed.
This is about Stark.
#325877: Password checker breaks if no description is about Garland.

Not true. That issue concerns every theme, and more than likely conflicts with this issue (the most recent patch certainly does).

In fact, that patch is so closely related that it indirectly fixes the original concern brought up in this issue (or it did last I tested anyway). The only reason I didn't mark this issue a duplicate instead of postponed is because it looks like there is room to improve the CSS further here. The other issue is just trying to do the minimum to fix a bug; I couldn't help but touch the CSS that fixes this bug as well.

Now, based on the last few comments here, it looks like this issue is going to rewrite enough code that it may accidentally fix my original bug too. If it does, then good. At least I could then stop feeling embarrassed about the bug being there.

@alpritt: Well then lets just see which issue gets in first and adapt the other one to the changes. I would say that's the best approach.
I have an intermediate patch I'll upload later, which has only the CSS changes. On top of that I am doing the jQuery stuff currently, but as it's the first time I see jQuery code it'll take a while :) You can try that patch, if it solves your issue.

Here's the patch, hope the upload works...

tstoeckler, I haven't forgotten this. I'll review this at a later date. Been backed up with other work.

Well, here's something you can review =): I actually got it all to work!
I tested this with Stark and Garland, each with and without JavaScript, and it works as it should. Tested ONLY in FF3/Linux.

It now looks a bit different than the screenshots by dvessel, especially that I added form descriptions (with JS off) and placed the suggestion box in between the two password fields.

One thing that isn't quite perfect: When you type something in the first password box, the second one's description "Enter a matching password." still shows innocently. When you enter something in the second box the description disappears and the "Passwords match: yes/no" thing appears, except that it isn't styled as a description. It doesn't look weird on its own, just the change from one two the other. Probably makes more sense, when you just try it out...

I noticed one bug (as in functionality-wise): When the character count in the confirmation box returns to zero (that means when you've pressed DELETE when there was only one character or you've selected multiple characters and then pressed DELETE, the status of the confimation message doesn't change. That can lead to the information being false, as you can see in the screenshots. Those passwords evidently don't match.

Also, as I've noted before, this is the first time, my hands have touched jQuery code, so what I left of it is probably a mess in terms of Code Style. I apologize in advance, and I would be very happy to help clean up the mess if you tell me how.

That's it. Oh, and sorry for not making much sense, it's late over here.

It's a good start but a few notes:

1.) The tips for a good password should never turn red. When the suggestions for stronger passwords are not met, it should not indicate an error since they are suggestions. Not a requirement.

2.) Having the suggestions between the two fields spreads them way too far apart. Proximity gives them a stronger connection. This might be okay in Stark but it looks too disconnected for something like Garland. It could use some CSS tweaks so the two password fields are next to each other. I don't think that border around the suggestions box is necessary either.

3.) The patch spills over into non-related areas. It hits on vertical tabs.

I'm a bit rusty with jQuery myself but I'll try and help out with that. I think we're working in the right direction.

1.) Agreed

2.) I agree that the box belongs below the second password field, and that the styling is unnecessary. Placing them next to each other, though is Garland's job, not Stark's.

3.) I guess a different patch got mixed in there, sorry

Also: I looked at your screenshots again, and I really like how you did the confirmation check. I'll build on that in the next version of the patch.

I'll look into all this in the next few minutes, but I don't know, if I make it to a new patch today. Maybe in the next few days.

This solves almost all the issues from #32.

Except:

1. The password confirm still has a bug, as described in #31 (See screenshot of #31 or #34).

2. I still am no JS expert, and as I have a bunch of duplicated code, I strongly suspect that people that know JS a little better will want to punch me when looking at user.js after applying this patch.

Other than that, I think it's pretty nice. I spaced all the different elements consistently with a 1em margin, as it is usual for form-items. See screenshot. I know that that is not the most beautiful way, but for Stark I think consistency should be the way to go.

DISCLAIMER: I have not looked at Garland with this patch applied!!!

All right. Garland looks pretty now, too.
Also, to achieve that, the suggestion box now has the description class, so that it doesn't take away that much space.
Unless someone has any specific objections or points me in any direction concerning 1. and 2. from #34, I'm pretty much done here, because I won't be able to figure those out on my
Also, as this reaches an end-reviewable state: This hasn't been cross-browser tested! Only FF3/Linux.

I'm cleaning up this patch. This is very close but I'm cleaning up the code. And there's still some dependancies on js which I'll be changing.

@#36: Un-commandeering the issue (2 week rule).

With just a cursory look, I'm concerned about the desire to leave so much of the styling on a theme-by-theme basis. It makes sense for individual themes to be responsible for skinning a UI widget, but there are definite reasons for choices in the layout. In my opinion, if you remove all the layout CSS, you've not made a 'stark' version; you've broken the widget.

The nature of the password checker means it's not horribly broken with the stark-ified layout, but I still believe it _is_ broken. The password suggestions are purposely placed directly under the first password box in order to provide context to what the user is typing via proximity. Removing the float separates it with the password confirm box, breaking this purposeful design choice.

Defaults are important, and the default we are providing with this patch is the bad version. I fear seeing thousands of Drupal sites with a broken widget for the sake of what appears to be a philosophical desire for purity. It's just not worth the cost of trying to reimplement this in every theme and so people won't.

I do, however, believe it appropriate that we provide a mechanism for overriding this. And that mechanism would be to wrap it up in a cosy theme function. Then pull the password specific CSS out of system.css and into a password.css file and add the css from the theme function.

Any UI folks interested only in reviewing the design changes will probably find the images in #22 and #35 most instructive.

[edit: s/#15/#22]

Don't agree with the rant, but with

The password suggestions are purposely placed directly under the first password box in order to provide context to what the user is typing via proximity. Removing the float separates it with the password confirm box, breaking this purposeful design choice.

Still waiting on dvessel's patch though, and on what he says to that.

Didn't mean my comment to sound like a rant.

It would be helpful to know why and exactly what you disagree with, please. :)

The reasoning that the password suggestions should appear below the first password box and that that can only happen if the two boxes float next to each other. Actually that was bugging me too, in Stark and so originally I had moved the password suggestions between the two boxes, but that wasn't very cool either, your absolutely right in your argument.

That should be a trivial fix though, once dvessel has posted something.

Err, I've been stuck with other work so this fell off my radar.

I mentioned before that having it styled by default would be fine if it was consistent to begin with but doing so would require quite a bit of rewriting. I have been looking to do that but I just haven't found enough time.

@alprit, as you mentioned, the styling is for this "widget". My concern was that there are many widgets completely isolated from one another when instead, they can overlap with a common default set of styles describing their flow. FAPI does this already and it's beautifully consistent. Well, until we have these one-off's since there are no existing styling rules that can describe their flow.

The reason I think this would be a worthwhile change is that the CSS will shrink. Overriding the styles will apply it to many areas with less effort. Contextual classes and id's would allow theming for very specific cases but the defaults being consistent would make styling a lot simpler. And your suggestion to have it wrapped in a theme function would be a big improvement. The suggestions for stronger passwords are currently wrapped within a script. Yikes! That needs to change.

@tstoeckler, I don't have a patch. Sorry for the wasted time. If no one jumps in by the time I get back on this, I'll get back on it myself. There were a few implementation details I wasn't liking but I couldn't find a alternative that made any more sense.

dvessel: Sorry, but I could not read an action plan from your post. I'd be willing to jump in, but I don't know how.

As it stands now, there is a bug in Stark's output. With my patch there is a JS bug in the password checker.
Maybe we should go and get the CSS part commited first and then see what to do with the JS-magic.
I would be willing to roll a patch that is like my last patch, except with no JS changes, and keeps the float in the default CSS, not in Garland, as per #37.

On a related note: If we're already doing CSS only changes, would it be worthwhile to put all the CSS from system.css to user.css or would that belong in a different patch. I thought about it for along time and could not think of any possibility where there is a password field with something unrelated to user.module. And since it is currently only called by user.module...

This bug revealed by Stark looks like a trivial fix but could we hold off on this for a bit? I'd really like to see both sides of this remedied.

I'll get back into this as soon as I can.

Please dont, go out and change colours of Garland. Please just keep it as it is, I don't want to refer to a 150 comment discussion over it.

Can someone please summarize what the remaining issues are and where there's consensus and disagreement? Thanks.

The patch needs to be rerolled against HEAD, it is currently returning a bunch of FAILED messages and some of the CSS has changed, e.g. the password fields are not styled via system-behavior.css rather than system.css.

At the very least we need to have the strength meter coming after the password field, not before, so we can do away with the horror of using a float right for it and a float left for the password confirm message - which is causing many issues for themers at the moment and virtually demands we use absolute positioning override to get some sanity back. Be nice if we could just use a float:left;