1200765 - Make login UX mobile friendly to assist mobile authentication workflow

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, defect)

Description

[Dylan Hardison [:dylan] (he/him)]

Need to do this for BMO first. The changes to upstream bugzilla will be a bit different due to divergence, and we need this in BMO soon for the 2fa change that makes api keys more of a requirement.

Comment 1

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1200765_1.patch

Tested in Firefox for Android, chrome for android, and the Responsive view in firefox desktop. The solution is generalized a bit because we might want the same treatment for error pages. "Request Desktop Site" is honored.

Comment 2

[:glob ✱]

Comment on attachment 8656205 [details] [diff] [review]
1200765_1.patch

Review of attachment 8656205 [details] [diff] [review]:
-----------------------------------------------------------------

the screen is too wide for iphone 5 and earlier (320 x 568).
we should also increase the font size on mobile.

::: .htaccess
@@ +91,4 @@
>  RewriteRule ^(?:latest|1\.2|1\.3)/(.*)$ extensions/BzAPI/bin/rest.cgi/$1 [NE]
>  RewriteRule ^bzapi/(.*)$ extensions/BzAPI/bin/rest.cgi/$1 [NE]
>  RewriteRule ^data/assets/ZeroClipboard.swf(.*)$ extensions/BugModal/web/ZeroClipboard/ZeroClipboard.swf$1 [NE]
> +RewriteRule ^login$ index.cgi?GoAheadAndLogIn=1 [NE]

this doesn't appear to be used.

::: Bugzilla/Template.pm
@@ +1124,5 @@
> +            'is_mobile_browser' => sub {
> +                my $cgi = Bugzilla->cgi;
> +
> +                return 1;
> +                return $cgi->user_agent =~ /Mobi/;

this will always return true (debugging i guess).

nit: no need to create the $cgi var here

::: skins/custom/mobile.css
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

as this is part of the core header move this from /custom/ to /standard/

Comment 3

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1200765_3.patch

This looks pretty reasonable on an assortment of phones (and emulations of phones).

Comment 4

[:glob ✱]

Comment on attachment 8657989 [details] [diff] [review]
1200765_3.patch

Review of attachment 8657989 [details] [diff] [review]:
-----------------------------------------------------------------

looks good on mobile, but there's some regressions on desktop that should be fixed.

::: skins/custom/mobile.css
@@ +25,5 @@
> +
> +    body {
> +         font-size: normal;
> +    }
> +}
\ No newline at end of file

this file appears to be redundant and should be deleted.

::: template/en/default/account/auth/login.html.tmpl
@@ +44,5 @@
> +<div id="login" class="login-form">
> +  <form name="login" action="[% target FILTER html %]" method="POST"
> +        [%- IF Bugzilla.cgi.param("data") %] enctype="multipart/form-data"[% END %]>
> +    <div class="field-login">
> +      <label for="Bugzilla_login">Email Address:</label>

the labels should be bold, and vertically centered

@@ +46,5 @@
> +        [%- IF Bugzilla.cgi.param("data") %] enctype="multipart/form-data"[% END %]>
> +    <div class="field-login">
> +      <label for="Bugzilla_login">Email Address:</label>
> +      <input id="Bugzilla_login" name="Bugzilla_login"
> +             [%- ' type="email"' UNLESS Param('emailsuffix') %]>

the _login and _password fields are touching - add a gap to avoid this

@@ +52,5 @@
> +    </div>
> +
> +    <div class="field-password">
> +      <label for="Bugzilla_password">Password:</label>
> +      <input type="password" id="Bugzilla_password" name="Bugzilla_password">

on desktop the width of the fields is now smaller.  we have quite a few long email addresses so this now looks weird.  i suggest a width of 20em for both fields on non-mobile displays.

@@ +62,5 @@
> +        <input type="checkbox" id="Bugzilla_remember" name="Bugzilla_remember" value="on"
> +               [%+ "checked" IF Param('rememberlogin') == "defaulton" %]>
> +        <label for="Bugzilla_remember" class="checkbox-note">
> +          Remember my email address
> +        </label>

on desktop -remember and -restrict should have a 7em left margin to match the current layout

@@ +80,5 @@
>  
> +    <div class="field-submit">
> +      <input type="hidden" name="Bugzilla_login_token"
> +             value="[% get_login_request_token() FILTER html %]">
> +      <input type="submit" name="GoAheadAndLogIn" value="Sign in" id="log_in">

you've changed "Log in" to "Sign in" - revert this change (we use "Log in" consistently).

Comment 5

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1200765_4.patch

aligned the fields, added padding, etc.

Comment 6

[:glob ✱]

Comment on attachment 8669744 [details] [diff] [review]
1200765_4.patch

Review of attachment 8669744 [details] [diff] [review]:
-----------------------------------------------------------------

looks much better on desktop - full review later this week.

::: skins/standard/mobile.css
@@ +21,5 @@
> +
> +    #login .field-restrict, #login .field-remember {
> +    }
> +
> +    

remove trailing whitespace

Comment 7

[:glob ✱]

Comment on attachment 8669744 [details] [diff] [review]
1200765_4.patch

Review of attachment 8669744 [details] [diff] [review]:
-----------------------------------------------------------------

this looks excellent, except the 2fa verification pages also need mobile love (totp just needs larger input elements, see https://www.duosecurity.com/docs/duoweb for duo).

::: skins/custom/mobile.css
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

as per irc this file should be deleted

Comment 8

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1200765_5.patch

The duo CSS for the iframe looks simple but as duo hasn't been reviewed yet, I have not included it in this patch. This changes the totp elements to be bigger and adds allow_mobile to the verify-totp template.

Comment 9

[:glob ✱]

(In reply to Dylan William Hardison [:dylan] from comment #8)
> The duo CSS for the iframe looks simple but as duo hasn't been reviewed yet,
> I have not included it in this patch.

oh yeah .. i've been in duo land for so long i forgot it isn't in the tree yet :)

Comment 10

[:glob ✱]

Comment on attachment 8670880 [details] [diff] [review]
1200765_5.patch

Review of attachment 8670880 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

Comment 11

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   4ce3037..d6f47aa  master -> master