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

In T85853#2101523, @matmarex wrote:

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

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.

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.

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

• approach-1.patch975 BDownload
• approach-2.patch1 KBDownload

In T85853#2236266, @matmarex wrote:

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

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?

In T85853#2299095, @matmarex wrote:

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

In T85853#2299095, @matmarex wrote:

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

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.

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

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

vform:

• help texts are not handled properly:

vform:

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

vform:

Also, mobile:
vform:

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

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.

Text fields just don't look nice in OOUI:

vform	ooui

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

Another related UI task is T146551: ButtonElement: Fix focus <button> outline in Firefox

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.

Change 284902 had a related patch set uploaded (by VolkerE; owner: Bartosz Dziewoński):
[mediawiki/core@master] LoginSignupSpecialPage: Convert form to OOUI

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

Change 284902 abandoned by Bartosz Dziewoński:

[mediawiki/core@master] LoginSignupSpecialPage: Convert form to OOUI

Reason:

This has diverged too far, the work will probably need to be redone. The last few comments have advice on what else is missing.

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