This patch cleans up the nifty "new" login form that lets users switch back and forth between CAS, standard, and OpenID logins. It improves the appearance of the form with most themes, GREATLY improves the appearance with certain themes (i.e. Open Atrium), and removes some hardcoded text that shouldn't have been there in the first place.
If you don't believe me, I can show you "before" and "after" screenshots :)
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | cas-889396-8.patch | 4.69 KB | danepowell |
| #6 | 0001-Issue-889396-by-Dane-Powell-bfroehle-Login-form-comp.patch | 4.98 KB | bfroehle |
| cas-improvelogin.patch | 2.07 KB | danepowell |
Comments
Comment #1
danepowell commentedActually it looks like this might interfere a bit with OpenID login components, but I haven't had a chance to investigate why - it works for my needs and doesn't seem to affect functionality...
Comment #2
danepowell commentedComment #3
bfroehle commentedSubscribing, so I can take a look at this eventually.
Comment #4
bfroehle commentedHi Dane,
I'm a bit confused about your suggestion. The JS changes will impact OpenID, as you've discovered. The changes to $form['cas_identifier'] are generally hidden except if Javascript is turned off, and the other suggestion I don't quite understand.
Perhaps you can post a screenshot of this so I can better know what you are going for.
Comment #5
danepowell commentedOkay, sorry- it's been so long since I wrote this, I forgot what my motivation was. I had to reverse-engineer my own work :)
Some themes (i.e. Rubik / Open Atrium) hide the description of fields and instead provide the description in a pop-up when you hover over the field. Unfortunately, the redirection notification message is in the description of the cas-identifier checkbox, meaning it won't be shown with these themes. That's why I made the message part of the uncas-link.
Furthermore, this makes it possible to completely hide the edit-cas-identifier-wrapper, which is really better (and again necessary on some themes like Rubik, which will show it as an ugly empty fieldset).
Unfortunately, this sort of breaks the OpenID login. But in fact, OpenID login is already broken with this form. It might be possible to hardcode support for OpenID into cas.js, but it would take a bit of code, and I think using OpenID in combination with CAS is a bit strange and probably very rare- it might be easier to just add a warning message to the "Add CAS link to login forms:" configuration field that OpenID is not supported when this option is enabled.
In summary, I think the previous patch should be committed in order to provide compatibility with Open Atrium, and that a warning should be added to the CAS settings page that OpenID is not compatible with the new login form. Please let me know what you think.
Comment #6
bfroehle commentedI've attached a patch in the same vein, but rolled against 6.x-3.x with the following changes:
(1): Default wording was changed to "Log in using CAS" / "Cancel CAS login" [before it was "Log in via single sign-on (CAS)" / "Log in locally (no single sign-on)"] -- I'm indifferent to this change, however.
(2): Added a new text element which just displays 'You will be redirected to the secure CAS login page.' which we selectively hide/show along with the CAS links.
(3): Rework the JS to support OpenID as well.
I've tested it in Rubik (Open Atrium) and Garland in browsers both with an without JS. However, I'm hesitant do commit anything here without some more testing.
Comment #7
danepowell commentedLooks pretty slick. Tested on Open Atrium and stock Drupal, no problems. I might create a corresponding patch for 2.x when I get a chance, that's the only way I'll be able to test it in a "live" environment.
Comment #8
danepowell commentedHere's the corresponding patch for 2.3, still need to test it. As an aside, do you know why the testbot is ignoring these patches?
Comment #9
bfroehle commentedYeah, the status has to be set to Needs Review for the testbot to trigger.
Comment #11
danepowell commentedDuh... :) Well, apart from the testbot's complaint (apparently I don't know how to patch "properly" with git), the patch does in fact work fine for 2.3.
Comment #12
bfroehle commentedSo the needs work is because the patch couldn't be applied to the 6.x-3.x branch (as you would expect).
One last thing to consider --- previously in the user login BLOCK, the "register new user" / "change password" links were hidden when the user was on the CAS login phase. The proposed patch does _not_ hide those. Is that an issue?
Comment #13
danepowell commentedHmm, I suppose I would prefer that they were not present, and it would make for a better UX for my uses. For instance, it doesn't make any sense to retrieve a forgotten password if you are using CAS to log in. And I prefer that new users with CAS accounts simply log in with CAS to create their account, rather than creating it manually.
That's the case for my workflow on 6.x-2.x. I don't know if that reasoning would apply for other use cases / 6.x-3.x.
Comment #14
bfroehle commentedCommitted to all branches:
7.x-1.x and 6.x-3.x: Committed the patch in #6.
6.x-2.x: Committed a revised version of the patch in #8, which doesn't change any text strings and maintains the current behavior with respect to hiding the user register / change password links when you click on "Log in via single sign-on (CAS)"
Thanks for your comments, ideas, and testing!
Comment #15
danepowell commentedNo problem, thanks for being such a responsive maintainer!