910670 - Preference UI for proxy autologin

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Manish Goregaokar [:manishearth]]

Related: Bug 646452 , Bug 223636

Bug 521467 gives us the automatic signon preference "signon.autologin.proxy" for proxies. This essentially tells Firefox to use the saved passwords from the password manager for network proxies.

Most people who use proxies are behind a single network proxy, and there really isn't any need to prompt them for the password every time we open Firefox. While enabling autologin by default (Bug 646452) may be an option, it can get confusing for some.

Thus, I suggest that this option (disabled by default) be added to the proxy preferences window.

Comment 1

[Manish Goregaokar [:manishearth]]

Attachment: Patch 0.1

Added option at bottom of proxy pref window. There probably is a better place to keep this, but I can't immediately think of one. I don't think it should be nested underneath any one proxy type radiobutton as it has an effect in all modes except the "no proxy" mode.

Comment 2

[Mike de Boer [:mikedeboer]]

Comment on attachment 797884 [details] [diff] [review]
Patch 0.1

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

At first glance, this looks good to me.

I think it's best that you first fix the issues I mentioned and then ask gavin for review. How does that sound?

::: browser/components/preferences/connection.js
@@ +58,5 @@
>  
>      var shareProxiesPref = document.getElementById("network.proxy.share_proxy_settings");
>      shareProxiesPref.disabled = proxyTypePref.value != 1;
> +    var autologinProxyPref = document.getElementById("signon.autologin.proxy");
> +    autologinProxyPref.disabled = (proxyTypePref.value==0);

please keep the style consistent: ` = proxyTypePref.value == 0;`

@@ +64,1 @@
>      

Nit: no extra newline needed. You can remove the superfluous indentation while you're at it though.

::: browser/components/preferences/connection.xul
@@ +49,1 @@
>        <preference id="pref.advanced.proxies.disable_button.reload"

nit: please keep the newline before the 'pref.advanced.proxies.disable_button.reload'. You can remove the superfluous indentation while you're at it.

::: browser/locales/en-US/chrome/browser/preferences/connection.dtd
@@ +43,5 @@
>  <!ENTITY  shareproxy.label              "Use this proxy server for all protocols">
>  <!ENTITY  shareproxy.accesskey          "s">
> +<!ENTITY  autologinproxy.label              "Do not prompt for authentication if password is saved">
> +<!ENTITY  autologinproxy.accesskey          "a">
> +<!ENTITY  autologinproxy.tooltip         "This option silently authenticates you to proxies when you have saved credentials for them. You will be prompted if authentication fails.">

nit: it looks like you aligned the entity value rather randomly. I'd suggest you align them with the other entity values.

Comment 3

[Manish Goregaokar [:manishearth]]

Attachment: Patch 0.2

Comment 4

[Manish Goregaokar [:manishearth]]

Attachment: Patch 0.3

Comment 5

[:Gavin Sharp [email: [e-mail]]]

I would really much rather fix bug 646452. Can we chase down any doubts there, perhaps by posting to firefox-dev and/or mozilla.dev.security?

It would also be helpful if you attached a screenshot of the patch applied and requested UX review (you can mention it in the firefox-dev post, too).

Comment 6

[Manish Goregaokar [:manishearth]]

(In reply to :Gavin Sharp (mostly away until Sep 13; use [e-mail] for email) from comment #5)
> I would really much rather fix bug 646452. Can we chase down any doubts
> there, perhaps by posting to firefox-dev and/or mozilla.dev.security?

Yeah, me too, but I'm not entirely sure if it's desirable, as explained in comment 2 bug 646452. For my own use autologin-by-default would be perfect, but I'm not so sure that it applies to others. I'll investigate a bit on it, though.

(I'll get the screenshot after rebooting)

Comment 7

[Manish Goregaokar [:manishearth]]

Attachment: Proposed UI, with checkbox at bottom

This is what the current preference window looks like; it has the checkbox at the bottom.

There probably is a better place to put this, but I can't think of one that makes sense. It can't be nested in any one option as it works in all modes except "no proxy".

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 799403 [details] [diff] [review]
Patch 0.3

Madhava, any feedback about the wording or location of this pref? Seems edge-casey so not worth over-thinking too much, but I welcome any feedback from you or your delegate!

Comment 9

[:Gavin Sharp [email: [e-mail]]]

To be clear, I would like to default this pref to on (i.e. also fix bug 646452), but think there's value in letting people disable the behavior in our prefs UI should it prove problematic for them.

Comment 10

[Manish Goregaokar [:manishearth]]

Comment on attachment 801373 [details]
Proposed UI, with checkbox at bottom

Ui-reviewing on the screenshot instead of the patch for convenience.

Also, bump.

Comment 11

[Manish Goregaokar [:manishearth]]

Attachment: Patch 4: added some space

Improved UI as per vt's suggestion in IRC.

Comment 12

[Manish Goregaokar [:manishearth]]

Attachment: UI Screenshot

Comment 13

[Valentin Tsatskin [:vt]]

My suggestion on IRC:

The checkbox seems logically grouped with the bullet list. Can you try putting in a space between the option and the radio controls? The rationale here is to visually separate the new checkbox from the radio buttons so that they do not look like one grouping.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8374376 [details] [diff] [review]
Patch 4: added some space

>diff --git a/browser/components/preferences/connection.xul b/browser/components/preferences/connection.xul

>-      <preference id="network.proxy.http_port"    name="network.proxy.http_port"    type="int"/>
>+-      <preference id="network.proxy.http_port"    name="network.proxy.http_port"    type="int"/>

o_O

Looks good otherwise, but handing this off to Drew to test this and take a closer look.

Comment 15

[Manish Goregaokar [:manishearth]]

Attachment: Patch 5: Fix the - sign

(In reply to :Gavin Sharp (email [e-mail]) from comment #14)

> >-      <preference id="network.proxy.http_port"    name="network.proxy.http_port"    type="int"/>
> >+-      <preference id="network.proxy.http_port"    name="network.proxy.http_port"    type="int"/>
> 
> o_O


I have no clue how that got there :p. Stray keystroke most probably. Fixed now, anyway.

Comment 16

[Drew Willcoxon :adw]

Comment on attachment 8375258 [details] [diff] [review]
Patch 5: Fix the - sign

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

To me, the checkbox still looks grouped with the radio buttons, and maybe even the final radio button, because it's inside the groupbox.  They're all still visually grouped.  So how about this:

    ...
    </groupbox>
    <checkbox id="autologinProxy" ... />
    <separator/>
  </prefpane>
</prefwindow>

Put it below the groupbox.

If you disagree and want to stick with what you've got, that's OK (except for what I comment on below).  Either way, please post a new patch and flag me again.

::: browser/components/preferences/connection.xul
@@ +159,5 @@
>        </radiogroup>
> +      <separator/>
> +      <row>
> +        <hbox/>
> +        <hbox>

A couple of things: (1) There's a stray, empty <hbox/> that should be removed, and (2) <row> is only for <rows> and <grid>.  In fact I don't think you need anything here except the separator and checkbox, so please remove everything else.  Like:

  </radiogroup>
  <separator/>
  <checkbox ... />
</groupbox>

@@ +161,5 @@
> +      <row>
> +        <hbox/>
> +        <hbox>
> +          <checkbox id="autologinProxy"
> +                    label="&autologinproxy.label;" 

Nit: Please remove the trailing whitespace on this line.

@@ +163,5 @@
> +        <hbox>
> +          <checkbox id="autologinProxy"
> +                    label="&autologinproxy.label;" 
> +                    accesskey="&autologinproxy.accesskey;"
> +                    preference="signon.autologin.proxy" 

Nit: Please remove the trailing whitespace on this line.

Comment 17

[Manish Goregaokar [:manishearth]]

Attachment: UI screenshot, no checkbox alignment

Comment 18

[Manish Goregaokar [:manishearth]]

Attachment: Patch 5: Remove checkbox alignment

Done.

I had initially thought that aligning the checkbox with the radiobuttons would look aesthetically better, however it does give a false sense of them being grouped together. I've put a separator and checkbox outside the groupbox to remove the alignment.

Comment 19

[Drew Willcoxon :adw]

Comment on attachment 8375913 [details] [diff] [review]
Patch 5: Remove checkbox alignment

Oh, I see.  On OS X, there's actually a box drawn for the groupbox with a different fill color from the window's, so they all really do appear as one group.  That's also why I didn't suggest adding a separator between the groupbox and checkbox.  On OS X, adding a separator there doesn't look great, and without a separator between the checkbox and buttons, they're too close together.

Does it look bad on Linux to have the separator between the checkbox and buttons, and no separator between the groupbox and buttons, like I suggested?  If so, and you need more space between the groupbox and checkbox, how about using a <separator class="thin"/>?  If that still looks bad, let's just go with your original patch with my comments addressed, since the spacing with it is OK.

Comment 20

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #19)
> buttons, and no separator between the groupbox and buttons,

Between the groupbox and checkbox, I meant.

Comment 21

[Manish Goregaokar [:manishearth]]

(In reply to Drew Willcoxon :adw from comment #19)

> Does it look bad on Linux to have the separator between the checkbox and
> buttons, and no separator between the groupbox and buttons, like I
> suggested?  If so, and you need more space between the groupbox and
> checkbox, how about using a <separator class="thin"/>?  If that still looks
> bad, let's just go with your original patch with my comments addressed,
> since the spacing with it is OK.

Nah, it looks fine on Linux (see the new screenshot). Without a separator it sticks to the top, though.

Comment 22

[Drew Willcoxon :adw]

Sorry, my fault for not being clear.  By "buttons" alone I mean the Help, Cancel, and OK buttons.  I used "radio buttons" everywhere else I was talking about the radio buttons.

Comment 23

[Manish Goregaokar [:manishearth]]

Attachment: Patch 6: Two spacers

How's this? It looks nice on Linux.

Comment 24

[Drew Willcoxon :adw]

Comment on attachment 8376143 [details] [diff] [review]
Patch 6: Two spacers

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

Looks good, thanks.

Comment 25

[Manish Goregaokar [:manishearth]]

Setting [checkin-needed].

Or are there any residual issues?

Comment 26

[Manish Goregaokar [:manishearth]]

Removing checkin-needed, there seems to be an issue here.

Comment 27

[Manish Goregaokar [:manishearth]]

Attachment: Final patch

Ugh, had accidentally removed the "p" of "preference" whilst fixing the spacing.

Fixed now; I think this patch is more or less final.

(Tested and it seems to work)

Comment 28

[Ben Bucksch (:BenB)]

I don't think we need this UI.

In the login prompt that asks the user for the password, he has a checkbox "[x] remember this login". If he checks that, Firefox will store the password. My patch merely made Firefox *use* that password.

We added the pref merely as precautionary measure in case we find situations where this new feature is a problem, so that we (or more likely the admin) can quickly turn it off. It's been intended to be true by default. Then somebody unilaterally decided to switch the default to false, without much justification other than "I feel better this way".

It seems rather silly to ask *twice* whether we should save and use this password. Once here and once in the dialog that asks the password.

We should just change the default to true and be done with it. No UI needed here. It's always supposed to be always-on.

Comment 29

[Ben Bucksch (:BenB)]

Comment 28 is bug 646452 (thanks for cross-linking it! I forgot about that one.)

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/3973c3329a9a

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3973c3329a9a

Comment 32

[Francesco Lodolo [:flod]]

With this patch (it doesn't seem to be in today's nightly), aren't you creating an access key conflict? "A" is already used for "Automatic proxy configuration URL:".

Also note that support for tooltips this long is far from optimal across different operative systems (talking from direct experience).

Comment 33

[Bogdan Maris, Desktop Test Engineering]

I did some exploratory around this feature and did not find any major issues. I used a Squid server and latest Nightly for my testing. Dropping verifyme and setting the status of the bug to Verified.

Comment 34

[Bogdan Maris, Desktop Test Engineering]

Forgot to mention that I verified the feature on Ubuntu 13.04 x64, Mac OS X 10.9.1 and Windows 7 x64.