1482271 - Implement preferences to configure DoH

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement, P1)

Description

[Daniel Stenberg [:bagder]]

We have working DoH support in Firefox since 62, and it would be great to get support for setting your user preferences with a UI similar to how proxies are specified. (Currently, manually fiddling with prefs is the only way.)

Here's an idea how it could work:

1] at the bottom of preferences, where it says "network proxy" change that to "network settings"

2] when you click settings you get the connection settings pop up just as you do now.. no changes

3] at the bottom of that page add a checkbox for dns over https enabled/disabled

4] also add a text box for dns over https URL .. greyed out if disabled

the checkbox needs to be enabled for prefs of network.trr.mode 1->4 and disabled for modes 0 and 5. If someone changes things to disable, make them mode 0 and if someone changes things to enable make them mode 2. The text box should reflect network.trr.uri pref (read/writable)

Comment 1

[Amy Lee [:amylee] UX]

Attachment: Prefs Screen.png

Hi, 

I've attached a mock based on the proposal. Let me know if this works. As for the copy, NI Meridel to review/provide suggestions.

Comment 2

[Daniel Stenberg [:bagder]]

Except for the lowercase s in HTTPS, it looks perfect to me!

Comment 3

[Patrick McManus [:mcmanus]]

do we have an eta on landing this?

Comment 4

[Justin Dolske [:Dolske]]

Sam should be starting it this week (if not already), we were just wrapping up some other work first. Still expecting to land before uplift.

Comment 5

[Sam Foster [:sfoster] (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #4)
> Sam should be starting it this week (if not already), we were just wrapping
> up some other work first. Still expecting to land before uplift.

Yeah I'm working on a patch for this today. No questions so far. Thanks for the mockup Amy.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1482271 - Add preferences UI for the DNS over HTTPS mode and uri prefs

MozReview-Commit-ID: G10vRxluGCa

Comment 7

[Sam Foster [:sfoster] (he/him)]

Just a couple open questions on attachment 9003365 [details]: 

* Any test that tries to clear the uri textbox and set the pref back to empty string times out waiting for that pref changed. The same operation works just fine when manually testing afaict. I've commented out that one for now. I can continue investigating tomorrow. Its probably not critical for this patch, but seems like a reasonable test to have. 

* I didn't see any search keywords explicitly added for the connections screen, and I'm not sure what if any needs to be added for these new controls. "DNS" works already...

* I didn't add an accesskey for the new controls. I'm not sure if that is wanted and if there is any significance to which key is picked?

For the new strings, I went ahead and changed the id for network-settings-title (was network-proxy-title) as that string changed and needed a new id. The other related strings for that section didn't need to change, so I didn't think we would want to change the ids even though they are now a little inconsistent with the title (network-proxy-*)

Comment 8

[Francesco Lodolo [:flod]]

We're short on time, but I find the mock-up a bit inconsistent. 

Proxy settings have a label, followed by an input field. In this case, there's a label, then same label + " URL" followed by an input field. 

Would it be confusing to use the same approach at proxy settings, and drop the second label? Or just use "URL", in case you think the reason for the input field could be unclear, since it's indented, and it clearly belongs to "DNS over HTTPS"?

> * I didn't see any search keywords explicitly added for the connections
> screen, and I'm not sure what if any needs to be added for these new
> controls. "DNS" works already...

I think you need to add the new keys here, for example to allow people to search for "DNS over HTTPS"
https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/main.xul#697

> * I didn't add an accesskey for the new controls. I'm not sure if that is
> wanted and if there is any significance to which key is picked?

I think we need one for consistency, I doubt you'll find one not already used in this dialog, so it's going to be a duplicate.

> For the new strings, I went ahead and changed the id for
> network-settings-title (was network-proxy-title) as that string changed and
> needed a new id. The other related strings for that section didn't need to
> change, so I didn't think we would want to change the ids even though they
> are now a little inconsistent with the title (network-proxy-*)

Good call on not changing them. Changing the IDs would require retranslation, and I'd prefer to avoid it, since the strings don't actually change.

Comment 9

[Sam Foster [:sfoster] (he/him)]

(In reply to Francesco Lodolo [:flod] from comment #8)
> We're short on time, but I find the mock-up a bit inconsistent. 
> 
> Proxy settings have a label, followed by an input field. In this case,
> there's a label, then same label + " URL" followed by an input field. 
> 
> Would it be confusing to use the same approach at proxy settings, and drop
> the second label? Or just use "URL", in case you think the reason for the
> input field could be unclear, since it's indented, and it clearly belongs to
> "DNS over HTTPS"?

Michelle is already need-info'd on this bug. I'll see if I can reach her or someone else in UX to make a decision on this. I don't have a strong opinion - yes its a bit redundant, but we also need to be clear that you can check the box and not have a value in the URI field to use our default (cloudflare).

Comment 10

[Sam Foster [:sfoster] (he/him)]

I just met with Michelle and Amy to discuss the URL label issue and we propose the following. 

Inactive state: 
[ ] DNS over HTTPS
    URL [                                   ]

The URL line is greyed out following the same pattern as the proxy settings above. The URL textbox can be readonly. I suggest we don't add the default url here as it represents detail that isn't relevant until you opt-in to the feature. 

Active state: 
[x] DNS over HTTPS
    URL [https://whtaevethedefaultis.com/dns]

The URL line is no longer greyed out, and we populate the textbox with the default/fallback when network.trr.uri is not defined. 

The concern was that people wouldn't know what to put in this field. Or to not touch it if they didn't know. Having the default there should provide these cues. However, that value isn't exposed to the chrome code right now, is that possible?

Comment 11

[Sam Foster [:sfoster] (he/him)]

Thinking about it, using the default URI as the placeholder text would be good. It gives the cues, but has the behavior we want as it isn't going to interfere with the pref value for network.trr.uri, it goes away as soon as the users puts anything in there, and comes back if they clear the field. 

The other possibility is adding some more context in the tooltip.

Is there a reason why we don't just set the initial/default value of the network.trr.uri pref to whatever our fallback value is?

Comment 12

[Daniel Stenberg [:bagder]]

Similar to the proxy fields, there really is no "default" URI to use and there's no fallback either. Users would need to enter a URI for a service they trust and want to use.

The only URI that we actually know and that we *could* enter as a default URI would be the cloudflare DoH server URI used for the TRR shield studies: "https://mozilla.cloudflare-dns.com/dns-query". That's however a single external operator and one that all users aren't going to think is the best idea to use for this service...

I personally would probably prefer to have the default URI that vanishes when a user starts to type to be just an example that doesn't actually work but shows roughly what we expect the field to get. Like "https://doh.example.com/dns-query"

Comment 13

[Sam Foster [:sfoster] (he/him)]

(In reply to Daniel Stenberg [:bagder] from comment #12)
> The only URI that we actually know and that we *could* enter as a default
> URI would be the cloudflare DoH server URI used for the TRR shield studies:
> "https://mozilla.cloudflare-dns.com/dns-query". That's however a single
> external operator and one that all users aren't going to think is the best
> idea to use for this service...

Ah ok, that's clearer now. I had thought that we were supporting an ongoing service like this as a part of shipping this feature.

> I personally would probably prefer to have the default URI that vanishes
> when a user starts to type to be just an example that doesn't actually work
> but shows roughly what we expect the field to get. Like
> "https://doh.example.com/dns-query"

Ok, I can do something like that. Thanks.

Comment 14

[Francesco Lodolo [:flod]]

Comment on attachment 9003365 [details]
Bug 1482271 - Add preferences UI for the DNS over HTTPS mode and uri prefs

Francesco Lodolo [:flod] has approved the revision.

Comment 15

[Johann Hofmann [:johannh]]

Comment on attachment 9003365 [details]
Bug 1482271 - Add preferences UI for the DNS over HTTPS mode and uri prefs

Johann Hofmann [:johannh] has approved the revision.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8657207d083c
Add preferences UI for the DNS over HTTPS mode and uri prefs r=johannh,flod

Comment 17

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/8657207d083c

Comment 18

[Ton [:Tonnes]]

(In reply to Francesco Lodolo [:flod] from comment #8)

> > * I didn't add an accesskey for the new controls. I'm not sure if that is
> > wanted and if there is any significance to which key is picked?
> 
> I think we need one for consistency, I doubt you'll find one not already
> used in this dialog, so it's going to be a duplicate.

Fwiw: for "Enable DNS over HTTPS", the b is available, and when ignoring the XUL Accesskey Policy for strings with descenders (which is already done for the one pixel wide i character), g could be used in "Use system proxy settings" to free the U for URL. I would recommend to avoid the H because of Help buttons already present or possibly getting introduced in all cases, and the above appears to work.

Comment 19

[Sam Foster [:sfoster] (he/him)]

> Fwiw: for "Enable DNS over HTTPS", the b is available, and when ignoring the
> XUL Accesskey Policy for strings with descenders (which is already done for
> the one pixel wide i character), g could be used in "Use system proxy
> settings" to free the U for URL. I would recommend to avoid the H because of
> Help buttons already present or possibly getting introduced in all cases,
> and the above appears to work.

I've filed bug 1486951 for this. Thanks for the suggestion.