Page MenuHomePhabricator

Convert MW core login/create account pages to OOUI (Special:UserLogin / Special:CreateAccount)
Open, HighPublic

Description

Let's convert MW core login/create account templates to OOUI. Splitting this off from T74715. Let's do this after T12317 is fixed.

Open issues (Feb 2019):

  • Error message not properly styled clear enough T145674
  • Captcha's misaligned, especially on mobile
  • Labels added on mobile
  • Required indicators probably better off hidden on this specific forms

Related Objects

StatusAssignedTask
ResolvedEsanders
OpenNone
OpenNone
OpenNone
ResolvedMarkTraceur
Resolvedmatmarex
Resolvedmatmarex
Resolvedmatmarex
OpenNone
OpenNone
OpenNone
ResolvedJdlrobson
Resolvedmatmarex
Resolvedmatmarex
OpenNone
OpenNone
ResolvedEsanders
DuplicateNone
ResolvedTTO
ResolvedJayprakash12345
DuplicateNone
OpenNone
ResolvedNone
DuplicateNone
ResolvedAnomie
ResolvedAnomie
ResolvedTgr
ResolvedAnomie
ResolvedJoe
ResolvedJoe
Resolvedhashar
Resolvedbd808
ResolvedAnomie
ResolvedKrinkle
OpenNone
ResolvedJanZerebecki
ResolvedKrinkle
ResolvedTgr
Resolvedmatmarex
ResolvedVolker_E
ResolvedVolker_E
ResolvedVolker_E
DuplicateVolker_E

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
matmarex updated the task description. (Show Details)Jan 14 2015, 8:36 PM
matmarex renamed this task from Convert MW core login/create account pages to OOUI to Convert MW core login/create account pages to OOUI (Special:UserLogin / Special:CreateAccount).Nov 4 2015, 6:47 PM

Change 183390 restored by Bartosz Dziewoński:
[WIP] Convert login form to OOUI

Reason:
Eeeeehh

https://gerrit.wikimedia.org/r/183390

Change 183390 abandoned by Bartosz Dziewoński:
[WIP] Convert login form to OOUI

Reason:
OK, no, finishing this properly is still not worth it.

https://gerrit.wikimedia.org/r/183390

@matmarex What are the blockers here, still the same as laid out above?

Yes. There are two ways we could do this:

  • Spend a ton of effort to change this now, which will all have been for naught when T12317 happens. (My patch above tried this, but it only changes the login form, not touching the signup form at all.)
  • Wait for somebody else to spend a ton of effort doing T12317, and then ride on top of their work and change this easily.

There is actually work happening on T12317 now (as a side-effect of T110277) and it might be done before the end of March, yay (per comments on https://gerrit.wikimedia.org/r/#/c/240052/). So I say let's wait. :)

Tgr added a subscriber: Tgr.Apr 12 2016, 10:48 PM

There is actually work happening on T12317 now (as a side-effect of T110277) and it might be done before the end of March, yay (per comments on https://gerrit.wikimedia.org/r/#/c/240052/). So I say let's wait. :)

The relevant part is LoginSignupSpecialPage::mainLoginForm. It uses a vform because it looks very broken in ooui; anyone willing to tweak that is welcome :) ($wgDisableAuthManager = false; needs to be set to test it.)

He7d3r added a subscriber: He7d3r.Apr 18 2016, 11:29 AM

Change 284902 had a related patch set uploaded (by Bartosz Dziewoński):
[WIP] LoginSignupSpecialPage: Convert form to OOUI

https://gerrit.wikimedia.org/r/284902

In T85853#2201537, @Tgr wrote:

The relevant part is LoginSignupSpecialPage::mainLoginForm. It uses a vform because it looks very broken in ooui; anyone willing to tweak that is welcome :) ($wgDisableAuthManager = false; needs to be set to test it.)

I gave it a try (see patch above) and it was pretty easy to make it reasonable. Some things still look broken due to the weird mix of old mediawiki.ui and new OOjs UI styles, and some seems to be bugs in OOUIHTMLForm, and I didn't really feel like looking into those this morning, but I will at some point. (Unless perhaps @Volker_E does it first ;) )

Some quick screenshots:

Before
After

Notable changes:

  • The forms fields are going to be a bit wider (to match the spacier style of OOjs UI, and to accomodate wide captchas which are now sometimes "sticking out" of the form, there should be a bug about this somewhere).
  • Buttons are no longer full-width (it would look even sillier with the wider form).
  • Some text is now hidden behind icons.
Volker_E added a comment.EditedApr 22 2016, 11:21 PM

Thanks for that effort, a small question to start with, why doesn't the password field have a required indicator on “Create account” screen?

Hmm, no idea. I presume it is a bug. The password can be optional in some workflows (when the send-temporary-password-via-email checkbox as seen in the last screenshot is checked, or perhaps in some different authentication methods), but I don't think it is optional in that one.

Tgr added a comment.EditedApr 23 2016, 11:41 PM

Thanks for looking into this!

Required field handling is a weakness of the new system. There are two kinds of authentication plugins: primary providers tell who the user is (or create it), other providers perform extra checks (captcha, password reset etc). For an authentication success to succeed, all non-primary providers need to accept and one primary needs to accept (since primaries are competing authentication methods, like password login and Google OAuth login).

Providers can tell what fields they need and whether they are required; multiple providers can share the same field (e.g. local username/password, CentralAuth and LDAP login all use the same fields). A field will be marked as required is it is marked so by some non-primary provider, or by all primary providers.

Since the "create a random password and send it to the given email address" workflow does not require a password, the corresponding primary provider does not mark the field and it ends up as optional. (Toggling the "send in email" option will remove the field so it could be left required, but there is no way to express that.)

There is a hook for messing with form settings, so it's easy to re-add the required flag for a specific installation, but I'm not sure if it can be done in a generic way.

Right. However, the send-temporary-password-via-email provider (TemporaryPasswordPrimaryAuthenticationProvider, apparently) is not available to be used by anonymous users for account creation. So, when an anonymous user is creating an account, the only provider that should matter is LocalPasswordPrimaryAuthenticationProvider, and that one obviously requires a password.

I poked around a little and it seems like there are at least two reasonable ways we could do this. Can you say why they can't work? :) Both seem a bit hacky, but acceptable to me. (These patches apply on top of https://gerrit.wikimedia.org/r/#/c/284902/ / 1891ca9b027b9c9b3bf87e51708e5cd34d980ee1.)

Tgr added a comment.Apr 25 2016, 7:16 PM

Right. However, the send-temporary-password-via-email provider (TemporaryPasswordPrimaryAuthenticationProvider, apparently) is not available to be used by anonymous users for account creation. So, when an anonymous user is creating an account, the only provider that should matter is LocalPasswordPrimaryAuthenticationProvider, and that one obviously requires a password.

You are right, that should work. I'll look into it.

I poked around a little and it seems like there are at least two reasonable ways we could do this. Can you say why they can't work? :) Both seem a bit hacky, but acceptable to me. (These patches apply on top of https://gerrit.wikimedia.org/r/#/c/284902/ / 1891ca9b027b9c9b3bf87e51708e5cd34d980ee1.)

The hardcoding the required flag would break registration methods which do not need a password (e.g. registering with a Facebook account). AuthManager is too generic to special-case for this, plus I don't think that part needs fixing - TemporaryPasswordPrimaryAuthenticationProvider does not return any request when the user is not logged in, so the PasswordAuthenticationRequest is the only one that contains the password field, so AuthManager::getAuthenticationRequestsInternal should flip its required flag to REQUIRED and then AuthenticationRequest::mergeFieldInfo and AuthManagerSpecialPage::fieldInfoToFormDescriptor should change that into a form field definition with required => true. There is probably a bug in that somewhere.

AuthManager is merged and no longer a blocker, woo!

@Volker_E Would you be able/willing to take over https://gerrit.wikimedia.org/r/#/c/284902/ and finish it? (Get rid of all the CSS rules that no longer apply, and add new ones to make the forms pretty.)

@Tgr The required flag for password doesn't seem to be fixed on master :(. Do you want me to file a task for it?

Tgr added a comment.May 17 2016, 9:09 AM

@Tgr The required flag for password doesn't seem to be fixed on master :(. Do you want me to file a task for it?

Sorry! I will look into it for reals once we are over the merge/backport craziness.

AuthManager is merged and no longer a blocker, woo!
@Volker_E Would you be able/willing to take over https://gerrit.wikimedia.org/r/#/c/284902/ and finish it? (Get rid of all the CSS rules that no longer apply, and add new ones to make the forms pretty.)

Yes, I will. :) It's not my highest priority right now, but will do try to get it further in the next couple of weeks.

Change 291678 had a related patch set uploaded (by Gergő Tisza):
Fix required field calculation in AuthenticationRequest

https://gerrit.wikimedia.org/r/291678

Change 291678 merged by jenkins-bot:
Fix required field calculation in AuthenticationRequest

https://gerrit.wikimedia.org/r/291678

Change 291727 had a related patch set uploaded (by Gergő Tisza):
Fix required field calculation in AuthenticationRequest

https://gerrit.wikimedia.org/r/291727

Change 291727 merged by jenkins-bot:
Fix required field calculation in AuthenticationRequest

https://gerrit.wikimedia.org/r/291727

Anomie added a subscriber: Anomie.Jun 16 2016, 6:49 PM

Regarding the field requirement determination, AuthManager does give you some field cross-requirement information. For example, enwiki as a logged-in user right now tells me (among other things):

  • "MediaWiki\Auth\UsernameAuthenticationRequest" is required. It has one required field, "username".
  • "MediaWiki\Auth\PasswordAuthenticationRequest" is primary-required. It has three required fields, "username", "password", and "retype".
  • "MediaWiki\Auth\TemporaryPasswordAuthenticationRequest" is primary-required. It has one required field, "mailpassword" (a checkbox).

So from this you could deduce that a "username" field is always required, and you have to specify either "mailpassword" or both "password" and "retype". Actually making use of this information generically (instead of hard-coding that checking "mailpassword" hides "password" and "retype") seems like a difficult problem, though.

My local testing wiki is even more complicated, since I've recently been reviewing https://gerrit.wikimedia.org/r/#/c/289099/:

  • "MediaWiki\Auth\UsernameAuthenticationRequest" is required. It has one required field, "username".
  • "MediaWiki\Auth\PasswordAuthenticationRequest" is primary-required. It has three required fields, "username", "password", and "retype".
  • "MediaWiki\Auth\TemporaryPasswordAuthenticationRequest" is primary-required. It has one required field, "mailpassword" (a checkbox).
  • "GoogleLogin\Auth\GoogleAuthenticationRequest:googlelogin" is primary-required. It has one required field, "googlelogin" (a submit button).

Again, you could deduce that a "username" field is always required, and you have to specify either "mailpassword", or both "password" and "retype", or click the "googlelogin" button instead of the usual submit button.

Tgr added a comment.Jul 27 2016, 2:48 AM

Re: the current version of https://gerrit.wikimedia.org/r/#/c/284902/:

  • messages don't look too good with OOUI IMO:

vform:


ooui:

  • help texts are not handled properly:

vform:


ooui:

  • ConfirmEdit looks broken, partly because it is hand-tailored to the old sizes, but some text does not appear at all:

vform:


ooui:

Tgr added a comment.Jul 27 2016, 7:14 AM

Also, mobile:
vform:


ooui:

vform, tablet:

ooui, tablet:

@Tgr Thanks for these comparisons. Very helpful!

For the record: Mobile does some styling on the current vform-based login form, so the direct comparsion looks more scaried as it should, simply because the styles doesn't apply to mostly all of the OOUI form elements. fixing this should probably result in a more accurate form :)

Tgr added a comment.Aug 13 2016, 4:45 AM

OOUI error messages seem awkward to me, even apart from the issue fixed in T142639: there is just no visual sign I would recognize as error (such as red color or background). I feels closer to the explanatory text created with the 'help' keys on VForm.

Tgr added a comment.Aug 31 2016, 12:55 AM

Text fields just don't look nice in OOUI:

vformooui
  • there is more distance between the label and the field, so they don't form an obvious unit, the form is hard to parse
  • the placeholder text is way too aggressive, barely different from the label. Again, hard to scan.
Jdforrester-WMF raised the priority of this task from Low to High.May 3 2018, 3:28 PM

This is the last remaining use of $wgUseMediaWikiUIEverywhere other than in MobileFrontend. Let's try to get it done.

Volker_E updated the task description. (Show Details)Feb 21 2019, 6:42 AM

Time for picking this up again.
Due to the amount of work and different patches and comments it's harder to gather overview of what remains, therefore added open points on first look to the description.

Volker_E updated the task description. (Show Details)Mar 7 2019, 12:28 AM