Closed Bug 1249494 Opened 4 years ago Closed 3 years ago

Move networkProxySOCKSRemoteDNS checkbox to clarify when it can be set

Categories

(Firefox :: Preferences, defect, minor)

45 Branch
defect
Not set
minor

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: yfdyh000, Assigned: djmdeveloper060796)

References

Details

(Keywords: ux-consistency, Whiteboard: [good first bug])

Attachments

(2 files, 1 obsolete file)

It creates a confusion, like other forms of proxy support this option.
I would like to be assigned to this bug
hi @YF, so I was looked at the corresponding. So I found this function in proxyTypeChanged() in browser/components/preferences/connection.js. Do you mean this one should have another statement similar to shareProxiesPref.disabled = proxyTypePref.value != 1; where shareProxiesPref.disabled is replaced by corresponding id? @Harshal I spent sometime looking at the bug so it would be awesome if leave me to do it?
(In reply to shailrishabh from comment #2)
> hi @YF, so I was looked at the corresponding. So I found this function in
> proxyTypeChanged() in browser/components/preferences/connection.js. Do you
> mean this one should have another statement similar to
> shareProxiesPref.disabled = proxyTypePref.value != 1; where
> shareProxiesPref.disabled is replaced by corresponding id? @Harshal I spent
> sometime looking at the bug so it would be awesome if leave me to do it?

Sure you can take the bug.
There was no comment on the bug so i thought nobody had taken it.
Assigned to him, since he already has some research.
Assignee: nobody → shailrishabh
Status: NEW → ASSIGNED
@harshal thanks :-) @YF am I thinking on the right track here?
(In reply to shailrishabh from comment #5)
> @harshal thanks :-) @YF am I thinking on the right track here?

I guess the "socksDNSPref.disabled = (isDefinitelySocks4 || proxyTypePref.value == 0);" should be improved.
(In reply to shailrishabh from comment #7)
> Created attachment 8721779 [details] [diff] [review]
> disable DNSserver checkbox when no SOCKS proxy is set

"Bug - 1249494" to be a typo.

Set a reviewer by "suggested reviewers" dropbox in flag: reivew.
https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch
okay sure will do that. Also I am sorry I don't understand the typo you are pointing to. Should it be like this "Bug 1249494 -" ?
(In reply to shailrishabh from comment #9)
> okay sure will do that. Also I am sorry I don't understand the typo you are
> pointing to. Should it be like this "Bug 1249494 -" ?

Yes, I think it will be correct, reference to other commit messages: https://hg.mozilla.org/mozilla-central/
Attachment #8721779 - Attachment is obsolete: true
Attachment #8721791 - Flags: ui-review?
Attachment #8721791 - Flags: review?(dolske)
So... The behavior of this checkbox was an explicit decision in 665319 comment 18, and the current behavior seems to work as expected there. Unless there's a change of thinking, that makes this bug WONTFIX.

(In reply to YF (Yang) from comment #0)
> It creates a confusion, like other forms of proxy support this option.

As I understand bug 665319, this _is_ a supported option with the other proxy settings. But I agree it's weird UI. Philipp, what do you think about moving this checkbox to the bottom (below the "Do not prompt for authentication" checkbox) instead?
 
This is already a bit of a mess, because the "Do not prompt" checkbox _also_ applies to all the proxy settings except "No Proxy".

I'd suggest:

* put the two checkboxes together
* use a new label, something like like "Proxy DNS when using SOCKS 4" to make context/purpose clearer
* Disable the "Do not prompt" checkbox when "No Proxy" selected
* Disable this DNS checkbox when "No Proxy" selected _or_ "Manual" + SOCKSv4 is selected (ie, retain current behavior) 

This does have the downside that someone manually configuring their SOCKS5 proxy needs to look further down the page for one more SOCKS setting, but feels like that's a fair tradeoff to make the UI less wonky. And someone doing manual configuration can probably be expected to figure it out? Note that this is already a problem when using the other proxy settings; the user has to look for the DNS checkbox buried in another radio selection!

[Aside: Yang, if you're seeking a change in behavior, please get an agreement on that before tagging bugs as [good first bug]... As it stands we'll likely either reject the patch or ask for a rewrite, and it makes me sad this will be Kumar's first-patch experience,  through no fault of their own.]
Flags: needinfo?(philipp)
(In reply to Justin Dolske [:Dolske] from comment #12)
> So... The behavior of this checkbox was an explicit decision in 665319
> comment 18, and the current behavior seems to work as expected there. Unless
> there's a change of thinking, that makes this bug WONTFIX.
> 
> (In reply to YF (Yang) from comment #0)
> > It creates a confusion, like other forms of proxy support this option.
> 
> As I understand bug 665319, this _is_ a supported option with the other
> proxy settings. But I agree it's weird UI. Philipp, what do you think about
> moving this checkbox to the bottom (below the "Do not prompt for
> authentication" checkbox) instead?
>  
> This is already a bit of a mess, because the "Do not prompt" checkbox _also_
> applies to all the proxy settings except "No Proxy".
> 
> I'd suggest:
> 
> * put the two checkboxes together
> * use a new label, something like like "Proxy DNS when using SOCKS 4" to
> make context/purpose clearer
> * Disable the "Do not prompt" checkbox when "No Proxy" selected
> * Disable this DNS checkbox when "No Proxy" selected _or_ "Manual" + SOCKSv4
> is selected (ie, retain current behavior) 
> 
> This does have the downside that someone manually configuring their SOCKS5
> proxy needs to look further down the page for one more SOCKS setting, but
> feels like that's a fair tradeoff to make the UI less wonky. And someone
> doing manual configuration can probably be expected to figure it out? Note
> that this is already a problem when using the other proxy settings; the user
> has to look for the DNS checkbox buried in another radio selection!
> 
> [Aside: Yang, if you're seeking a change in behavior, please get an
> agreement on that before tagging bugs as [good first bug]... As it stands
> we'll likely either reject the patch or ask for a rewrite, and it makes me
> sad this will be Kumar's first-patch experience,  through no fault of their
> own.]

Wow, this is way beyond my technical expertise (and I wasn't able to figure out what exactly the relationships are from the UI).
Given that, I trust Dolskes expertise and judgement on this.
Flags: needinfo?(philipp)
Kumar: Can you make the changes outlined in comment 12? Or, since the scope of this bug just changed significantly, would you prefer someone else work on it?
Flags: needinfo?(shailrishabh)
Attachment #8721791 - Flags: ui-review?
Attachment #8721791 - Flags: review?(dolske)
Attachment #8721791 - Flags: review-
Clearing needinfo and unassigning the bug due to lack of activity. Please reassign the bug if you are still working on this and have an updated patch.
Assignee: shailrishabh → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(shailrishabh)
Attached patch bug1249494.patchSplinter Review
Changed the position and label of the proxy DNS checkbox.
Attachment #8769430 - Flags: review?(dolske)
Assignee: nobody → djmdeveloper060796
Status: NEW → ASSIGNED
Comment on attachment 8769430 [details] [diff] [review]
bug1249494.patch

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

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

It's an unfortunate requirement of the Mozilla L10N tools that when changing a string, you also need to change the entity/property name so it gets flagged for re-translation. (Otherwise other languages would just continue to use the translate value of the old string.)

I'll go ahead and make this trivial change and land the patch for you.

Thanks!
Attachment #8769430 - Flags: review?(dolske) → review+
Also: I went down the rabbit hole looking at the background of this, and it's more of a mess than I first thought. Bug 468868 and bug 474824 indicate that the remote-DNS setting may or may not actually do anything, depending on particularities of how the proxy config is set up and where it's hosted. But I don't think this is realistically fixable with the UI, though, especially given that it's already complex. I think for things to really be truthful to the user, those underlying bugs would need to be fixed. And, in any case, we're not making things any worse than they already are with this bug.

[Frankly, I think we should really just take a hard look at jettisoning some of these options, maintaining 5 different ways to configure networking with all kinds of interacting settings is ridiculous. But that's fodder for a different bug.]
(In reply to Justin Dolske [:Dolske] from comment #12)

> I'd suggest:
> 
[...]
> * use a new label, something like like "Proxy DNS when using SOCKS 4" to
> make context/purpose clearer

Oops, this should have been "SOCKS v5", since it doesn't apply when v4 is selected. My fault for the typo, and I updated the patch to say v5.
Summary: networkProxySOCKSRemoteDNS checkbox should be disabled, if no SOCKS proxy can be configured → Move networkProxySOCKSRemoteDNS checkbox to clarify when it can be set
https://hg.mozilla.org/integration/mozilla-inbound/rev/921e9c3f7f7efb48d7a20c4e9d3387f04126e7d8
Bug 1249494 - Move networkProxySOCKSRemoteDNS checkbox to clarify when it can be set. r=dolske
(In reply to Justin Dolske [:Dolske] from comment #20)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 921e9c3f7f7efb48d7a20c4e9d3387f04126e7d8
> Bug 1249494 - Move networkProxySOCKSRemoteDNS checkbox to clarify when it
> can be set. r=dolske

This patch raises a question for me because I have been recently working on bug 610896.

I investigated the existing UI with Sukhbir Singh, and we were surprised to see that the "Remote DNS" checkbox was disabled when SOCKS4 is selected. Our experiments with Firefox SOCKS via ssh showed that the socks_remote_dns pref applies both to SOCKS4 and SOCKS5, and it is possible to enable the pref in the UI by first selecting SOCKS5, checking the checkbox, and then selecting SOCKS4, such that Firefox now tries to use SOCKS4a. So it seems like the new checkbox text in this patch may enhance the confusion by incorrectly giving the impression that the checkbox does not affect SOCKS4.
Wait, the Remote DNS option has an effect only if the SOCKS proxy is manually configured and this is also an intentional decision. See bug 468868 and bug 474824.
https://hg.mozilla.org/mozilla-central/rev/921e9c3f7f7e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
(In reply to Arthur Edelstein from comment #21)
> This patch raises a question for me because ...

(In reply to Masatoshi Kimura [:emk] from comment #22)
> Wait, the Remote DNS option has an effect only if ...

Let's continue this discussion in bug 1286701. The patch here just clarified existing behavior, so a new bug to change what bug 665319 did would be best.
You need to log in before you can comment on or make changes to this bug.