Closed Bug 910670 Opened 6 years ago Closed 6 years ago

Preference UI for proxy autologin

Categories

(Firefox :: Preferences, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

(Keywords: feature)

Attachments

(2 files, 9 obsolete files)

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.
Assignee: nobody → manishsmail
Attached patch Patch 0.1 (obsolete) — Splinter Review
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.
Attachment #797884 - Flags: review?(gavin.sharp)
QA Contact: preferences
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Attachment #797884 - Flags: review?(gavin.sharp)
Attached patch Patch 0.2 (obsolete) — Splinter Review
Attachment #797884 - Attachment is obsolete: true
Attachment #799385 - Flags: review?(gavin.sharp)
Attachment #799385 - Attachment description: prox.patch → Patch 0.2
Attached patch Patch 0.3 (obsolete) — Splinter Review
Attachment #799385 - Attachment is obsolete: true
Attachment #799385 - Flags: review?(gavin.sharp)
Attachment #799403 - Flags: review?(gavin.sharp)
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).
(In reply to :Gavin Sharp (mostly away until Sep 13; use gavin@gavinsharp.com 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)
Attached image Proposed UI, with checkbox at bottom (obsolete) —
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 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!
Attachment #799403 - Flags: ui-review?(madhava)
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 on attachment 801373 [details]
Proposed UI, with checkbox at bottom

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

Also, bump.
Attachment #801373 - Flags: ui-review?(madhava)
Attached patch Patch 4: added some space (obsolete) — Splinter Review
Improved UI as per vt's suggestion in IRC.
Attachment #799403 - Attachment is obsolete: true
Attachment #801373 - Attachment is obsolete: true
Attachment #799403 - Flags: ui-review?(madhava)
Attachment #799403 - Flags: review?(gavin.sharp)
Attachment #801373 - Flags: ui-review?(madhava)
Attachment #8374376 - Flags: ui-review?(vtsatskin)
Attachment #8374376 - Flags: review?(gavin.sharp)
Attached image UI Screenshot (obsolete) —
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 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.
Attachment #8374376 - Flags: review?(gavin.sharp) → review?(adw)
Attached patch Patch 5: Fix the - sign (obsolete) — Splinter Review
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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.
Attachment #8374376 - Attachment is obsolete: true
Attachment #8374376 - Flags: ui-review?(vtsatskin)
Attachment #8374376 - Flags: review?(adw)
Attachment #8375258 - Flags: review?(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.
Attachment #8375258 - Flags: review?(adw)
Attachment #8374377 - Attachment is obsolete: true
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.
Attachment #8375258 - Attachment is obsolete: true
Attachment #8375913 - Flags: review?(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.
(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.
(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.
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.
Attached patch Patch 6: Two spacers (obsolete) — Splinter Review
How's this? It looks nice on Linux.
Attachment #8375913 - Attachment is obsolete: true
Attachment #8375913 - Flags: review?(adw)
Attachment #8376143 - Flags: review?(adw)
Comment on attachment 8376143 [details] [diff] [review]
Patch 6: Two spacers

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

Looks good, thanks.
Attachment #8376143 - Flags: review?(adw) → review+
Depends on: 521467
Keywords: feature
Summary: Preference for proxy autologin → Preference UI for proxy autologin
Setting [checkin-needed].

Or are there any residual issues?
Whiteboard: [checkin-needed]
Removing checkin-needed, there seems to be an issue here.
Whiteboard: [checkin-needed]
Attached patch Final patchSplinter Review
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)
Attachment #8376143 - Attachment is obsolete: true
Attachment #8376494 - Flags: review?(adw)
Attachment #8376494 - Flags: review?(adw) → review+
Keywords: checkin-needed
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.
Attachment #8376494 - Flags: feedback-
See Also: → 646452
Comment 28 is bug 646452 (thanks for cross-linking it! I forgot about that one.)
https://hg.mozilla.org/mozilla-central/rev/3973c3329a9a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
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).
Depends on: 974394
QA Contact: preferences → bogdan.maris
Keywords: verifyme
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Forgot to mention that I verified the feature on Ubuntu 13.04 x64, Mac OS X 10.9.1 and Windows 7 x64.
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.