665319 - Add a checkbox to activate network.proxy.socks_remote_dns

Status: VERIFIED FIXED

(Firefox :: Settings UI, enhancement)

Description

[rpointel]

User-Agent:       Mozilla/5.0 (X11; OpenBSD) AppleWebKit/534.26+ Midori/0.3
Build Identifier: Mozilla/5.0 (X11; OpenBSD amd64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

Hi,
It's my first ticket so if something was wrong don't hesitate to indicates me.

I think it could be usefull to have a checkbox to activate network.proxy.socks_remote_dns into settings.

This is the patch to activate this. I work on it with Emeric Boit.
I tested it on OpenBSD (amd64).

Thanks,

Remi.

Reproducible: Always

Comment 1

[rpointel]

Attachment: diff to add socks_remote_dns_checkbox

diff to add socks_remote_dns_checkbox.

Comment 2

[Mike Hommey [:glandium]]

I'd got even further and suggest that the default should be to turn it on...
When you go through a HTTP proxy, the proxy is going to do dns resolution, why should it be different for SOCKS proxies?
(I'm even tempted to say the pref default should be changed, and no UI provided)

Comment 3

[rpointel]

I'm ok with your idea, I'm annoyed that it's not true by default.

So if it's true by default when we use SOCKS proxies, I'm ok to not add checkbox in UI.

Comment 4

[Gervase Markham [:gerv]]

Defaulting it on is bug 610896. 

However, this preference currently only works with hard-coded SOCKS proxies, not when the proxy auto-config file returns a value that says you should use a SOCKS proxy. That bug is bug 468868.

I suggest this UI might be confusing if the checkbox only works part of the time, so bug 468868 would need to be fixed first.

Gerv

Comment 5

[Malcolm Smith]

Personally I'd suggest that adding UI is beneficial regardless of the default.  Whatever the default is, there will be a decent section of SOCKS users who will want to change it.  See bug 610896 comment 3.  It's really a historical accident that no UI exists in Firefox for this; I was working on SeaMonkey at the time, right when things were changing over, and SeaMonkey did get UI for a pref.

Thanks rpointel for building the patch for this.

Comment 6

[Manish Goregaokar [:manishearth]]

Why not have it enabled only when the proxy setting is manual and when the SOCKS field is non empty?

I'll try to add a patch that accomplishes this later.

Comment 7

[Manish Goregaokar [:manishearth]]

taking, this seems like an easy fix

Comment 8

[Manish Goregaokar [:manishearth]]

I have a preliminary patch, but I'm confused about the UI.

http://i.stack.imgur.com/vPNWI.png

Currently, I have placed the pref next to the SOCKS version prefs and I disable it when:

 - The "Manual Proxy configuration" option is on, but the proxy type is SOCKS 4
 - The "No proxy" configuration option is on

Because this pref is something that can be used whenever FF is using a SOCKS proxy, which can happen when it is configured to that the proxy details from elsewhere.

However, it looks a bit ugly to have a preference that is enabled when the whole area around it is grey (http://i.stack.imgur.com/FTEv6.png)

Any idea how this can be improved? (needinfoing a UX team member for some input, please let me know if this is inappropriate)

Comment 9

[Manish Goregaokar [:manishearth]]

Attachment: Patch 1: preference+disable/enable magic

This patch has a working "Remote DNS" option as well as JS magic to enable/disable it at the right times

(clearing needinfo because I can just ui-review)

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8364601 [details] [diff] [review]
Patch 1: preference+disable/enable magic

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

The patch looks good but I don't know whether leaving the checkbox enabled with automatic proxy settings is fine to do. I know that automatic proxy settings *could* set socks5 and the pref would then be useful but it looks very weird from a UX point of view to have a whole prefpane disabled except a tiny checkbox at the bottom. We might want to wait on some UX input for that.

::: browser/components/preferences/connection.js
@@ +66,5 @@
>      autoconfigURLPref.disabled = proxyTypePref.value != 2;
>  
>      this.updateReloadButton();
>    },
> +  socksTypeChanged: function ()

Nit: please add a blank line above here.

@@ +69,5 @@
>    },
> +  socksTypeChanged: function ()
> +  {
> +    var socksTypePref = document.getElementById("network.proxy.socks_version");
> +    if (socksTypePref.value == 5 && !socksTypePref.disabled) {

This condition doesn't play well with the fact that you allow changing the checkbox with e.g. automatic proxy settings. Try changing it with automatic settings and it will get disabled.

@@ +73,5 @@
> +    if (socksTypePref.value == 5 && !socksTypePref.disabled) {
> +      document.getElementById("network.proxy.socks_remote_dns").disabled=false;
> +    } else {
> +      document.getElementById("network.proxy.socks_remote_dns").disabled=true;
> +    }

This should be:

document.getElementById("network.proxy.socks_remote_dns").disabled = myVariable;

without the two branches.

@@ +80,1 @@
>    updateReloadButton: function ()

Nit: please add a blank line above here.

@@ +138,2 @@
>      socksVersionPref.disabled = proxyTypePref.value != 1 || shareProxiesPref.value;
> +    socksDNSPref.disabled = !(proxyTypePref.value == 1 && socksVersionPref.value == 5 || proxyTypePref.value != 0);

!((proxyTypePref.value == 1 && socksVersionPref.value == 5) || proxyTypePref.value != 0);

I hate having to think about operator precedence when reading code :) We could also clean this up further like:

var isSocks5 = proxyTypePref.value == 1 && socksVersionPref.value == 5;
socksDNSPref.disabled = !(isSocks5 || proxyTypePref.value != 0);

::: browser/components/preferences/connection.xul
@@ +36,5 @@
>        <preference id="network.proxy.ssl"          name="network.proxy.ssl"          type="string"/>
>        <preference id="network.proxy.ssl_port"     name="network.proxy.ssl_port"     type="int"/>
>        <preference id="network.proxy.socks"        name="network.proxy.socks"        type="string"/>
>        <preference id="network.proxy.socks_port"   name="network.proxy.socks_port"   type="int"/>
> +      <preference id="network.proxy.socks_version"  name="network.proxy.socks_version"  type="int" onchange="gConnectionsDialog.socksTypeChanged()"/>

socksTypeChanged() is a quite inaccurate function name. Maybe socksVersionChanged?

@@ +133,5 @@
>                </hbox>
>              </row>
>              <row>
>                <spacer/>
> +              <box pack="start">

Just using <vbox> here should do.

@@ +139,5 @@
>                            preference="network.proxy.socks_version">
>                  <radio id="networkProxySOCKSVersion4" value="4" label="&socks4.label;" accesskey="&socks4.accesskey;" />
>                  <radio id="networkProxySOCKSVersion5" value="5" label="&socks5.label;" accesskey="&socks5.accesskey;" />
>                </radiogroup>
> +              <checkbox id="networkProxySOCKSRemoteDNS" onsyncfrompreference="return gConnectionsDialog.socksTypeChanged()" preference="network.proxy.socks_remote_dns" label="&socksRemoteDNS.label;" accesskey="&socksRemoteDNS.accesskey;" />

Why do we call socksTypedChanged() here when the checkbox is synced with the pref? socksTypedChanged() only cares about the checkbox's disabled state and that doesn't change with the value for network.proxy.socks_remote_dns.

::: browser/locales/en-US/chrome/browser/preferences/connection.dtd
@@ +32,5 @@
>  <!ENTITY  socks4.accesskey              "K">
>  <!ENTITY  socks5.label                  "SOCKS v5">
>  <!ENTITY  socks5.accesskey              "v">
> +<!ENTITY  socksRemoteDNS.label                  "Remote DNS">
> +<!ENTITY  socksRemoteDNS.accesskey              "d">

Nit: The indentation here is a little off.

Comment 11

[Manish Goregaokar [:manishearth]]

Attachment: Patch 1: Improve code; make consistent

I consolidated all DNS-updating code into `updateDNSPref()`

I tried the vbox thing, it made the DNS checkbox come on a new line. What exactly is to be replaced by a vbox?

Currently, it gets disabled if it knows for sure that the proxy pref is SOCKS 4 (i.e., when the SOCKS 4 pref is enabled and selected), or if the setting is in no proxy mode.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8371209 [details] [diff] [review]
Patch 1: Improve code; make consistent

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

Thanks! This looks much better. Now let's wait for some UX feedback on the enabled checkbox at the bottom of a disabled prefpane. I'll hold off my r+ until then.

::: browser/components/preferences/connection.xul
@@ +133,5 @@
>                </hbox>
>              </row>
>              <row>
>                <spacer/>
> +              <box pack="start">

Using <vbox> ... </vbox> did work for me without any wrapping, hm. But that's not too important.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Can you please push your patch to try without any tests and only opt builds to provide something for UX people to click around? That might help with getting UX feedback a lot faster.

Comment 14

[Manish Goregaokar [:manishearth]]

Can't push to try yet, the process is currently ongoing (bug 964409 - the form has been received and I've been vouched, just need to wait on it moving forward)

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Pushed the patch to try:

https://tbpl.mozilla.org/?tree=Try&rev=2c129ba54237

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Try builds are here:

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2c129ba54237/

Comment 17

[Manish Goregaokar [:manishearth]]

Attachment: Animation of disabling/enabling of pref

Philipp, any comments on the location or behavior of the pref? Currently there are situations when it's enabled but surrounded by disabled options. Which looks strange.

Attached an animation to make it clearer.

Comment 18

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Manish Goregaokar [:manishearth] from comment #17)
> Created attachment 8371767 [details]
> Animation of disabling/enabling of pref
> 
> Philipp, any comments on the location or behavior of the pref? Currently
> there are situations when it's enabled but surrounded by disabled options.
> Which looks strange.
> 
> Attached an animation to make it clearer.

Thanks for the attachment!
The situation is odd, but after going back and forth a bit, I think the current solution is actually the best one.
The thing is that the box is enabled in three out of four cases. That means that from an information hierarchy point of view, it wouldn't make it better to move it e.g. to the top of this section. It also has to do with SOCKS, so the general location makes sense.
One option would be to only show the text fields when custom proxy is selected (and just the checkbox otherwise), but that would be a more significant effort.
Since there will be some changes to prefs in general when we move them in-content (hopefully soon), that would probably be a good time to look at issues like this one in a more holistic way.
Until then, I think the current solution works good enough™

Comment 19

[Manish Goregaokar [:manishearth]]

While testing this, I noticed something:

When we use the "use this proxy for all protocols" option, the SOCKS option is disabled even though the HTTP proxy text is copied to the SOCKS field.

I guess that is a separate bug, but is the SOCKS proxy actually used when you "use this proxy for all protocols"? If so, shouldn't the 4 vs 5 option be enabled?

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Looks like it should be enabled but I don't know enough about our proxy settings. Best would be to file a new bug, I think.

Comment 21

[Manish Goregaokar [:manishearth]]

Turns out that the issue was a bit different, filed bug 969282.

Comment 22

[Manish Goregaokar [:manishearth]]

Comment on attachment 8371209 [details] [diff] [review]
Patch 1: Improve code; make consistent

Re-requesting review as per comment 12

As for the vbox, isn't it supposed to put child elements vertically anyway? So I guess the wrapping was by design -- when I put it in the pref comes one line below the SOCKS version pref. Which isn't necessary, IMO.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Manish Goregaokar [:manishearth] from comment #22)
> As for the vbox, isn't it supposed to put child elements vertically anyway?
> So I guess the wrapping was by design -- when I put it in the pref comes one
> line below the SOCKS version pref. Which isn't necessary, IMO.

Yes, stupid me, sorry. I meant <hbox>.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8371767 [details]
Animation of disabling/enabling of pref

Setting to ui-r+ based on comment #18.

Comment 25

[Manish Goregaokar [:manishearth]]

Marking as checkin-needed, looks like it's done with the review.

(Let me know if it isn't)

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/1b0c95032973

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/1b0c95032973

Comment 28

[Tracy Walker [:tracy]]

Verified with latest Nightly build