566444 - Make Proxy Exceptions Multi Line

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[John Wayne Hill (UX Intern)]

(Note: this is filed as part of the “Paper Cut” bugs — we assume that there may be multiple existing bugs on this. Please make them block this bug, and we will de-dupe if they are indeed exactly the same. Thanks!)

To reproduce:
1. Go to preferences -> advanced -> network -> settings
2. Click on Manual Proxy Configuration 
3. the "No proxy for: " box should allow for multiple entries

From a User:
"The proxy exception settings ("No proxy for") under: edit->preferences->advanced->network->settings is completely unusable if you want to bypass the proxy for many sites.
Its a single line text box. At work I have to bypass about 15 (and rapidly increasing) domains. There is no easy way to see all the domains which makes debugging connection issues difficult.
This must be very easy to fix, please make it a multi-line text area and them please make it one domain/pattern per line instead of comma separated."

Recommendation:
Make the text input a multi-line text area

Comment 1

[John Wayne Hill (UX Intern)]

Attachment: Current Proxy Settings Menu (one line only)

Comment 2

[Matthew N. [:MattN]]

We should probably use the onsynctopreference[1] attribute to remove the new-lines from the value before it gets written.

I think 3 lines would be good (IE has 2; OS X has 4) and show a horizontal scrollbar if there are more lines.

[1] https://developer.mozilla.org/en/Preferences_System/New_attributes#onsyncfrompreference.2Fonsynctopreference

Comment 3

[Andrew Hurle [:ahurle]]

Some things I should note here about my work on this bug so far:

- If we use onsynctopreference like Matt said, it will actually replace newlines with commas as you type them.  This is inconsistent with how we already replace semicolons onbeforeaccept.  I'm going to use onbeforeaccept to strip newlines instead.

- Matt suggested I write a browser-chrome test to open up the connection dialog and press the accept button, then make sure input is indeed sanitized correctly and saved to prefs.

Because connection.xul uses type="child", it has to be opened as a subdialog of preferences.xul and that main window also has to be accepted before the prefs in connection.xul are saved.  I ran into a problem where preferences.xul refused to close itself with acceptDialog() after the subdialog was accepted.  This makes the test time out.

I'll attach a patch demonstrating the problem.  For now, I'll try getting a test to work using only in-content prefs.  I seem to be able to acceptDialog() properly on the main pref dialog if I never open the subdialog, so hopefully when I only have the subdialog to accept it will work fine.

Comment 4

[Andrew Hurle [:ahurle]]

Attachment: Test demonstrating acceptDialog failing to close pref window

In this test, the prefs dialog will open, connection dialog will open, connection dialog will close, then the prefs dialog _doesn't_ close for some reason.  It will even hit the expected dumps in preferences.xml.  Tried some other things with the help of MattN and jwein: synthesizing click events on the accept buttons, putting acceptDialog() behind setTimeout and waitForFocus, using different ways to listen for new windows - nothing worked.  It seems that there are no existing browser-chrome tests for subdialogs like this to look at for help.

Sort of irrelevant now, but attaching it for reference.

Comment 5

[Andrew Hurle [:ahurle]]

Attachment: v1 - Make proxy exceptions multiline

Here's a patch accompanied by a browser chrome test for the dialog that makes sure everything in the textbox is sanitized correctly.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 625738 [details] [diff] [review]
v1 - Make proxy exceptions multiline

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

::: browser/components/preferences/in-content/tests/Makefile.in
@@ +24,5 @@
>      browser_privacypane_5.js \
>      browser_privacypane_6.js \
>      browser_privacypane_7.js \
>      browser_privacypane_8.js \
> +    browser_connection.js \

nit: please keep these in alphabetical order, so move this after the browser_bug* ones.

::: browser/components/preferences/in-content/tests/browser_connection.js
@@ +12,5 @@
> +  let windowWatcher = Cc["@mozilla.org/embedcomp/window-watcher;1"]
> +                          .getService(Components.interfaces.nsIWindowWatcher);
> +
> +  //instantApply must be true, otherwise connection dialog won't save
> +  //when opened from in-content prefs

Has this been noticed on your machine? instantApply isn't explicitly set for in-content preferences because the root element of the in-content preferences has the special instantApply attribute. Does doing this help the test pass but leave a bug for the actual usage of the preferences?

@@ +23,5 @@
> +      if (aTopic == "domwindowopened") {
> +        //when connection window loads, run tests and acceptDialog()
> +        let win = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
> +        win.addEventListener("load", function() {
> +          win.removeEventListener("load", arguments.callee, false);

arguments.callee is deprecated. Please name the anonymous function above and use its name here.

@@ +118,5 @@
> +  networkProxyNonePref.value = ".a.com;.b.com\n.c.com";
> +}
> +
> +function resetPreferences() {
> +  let oldNoProxies = Services.prefs.getCharPref("network.proxy.no_proxies_on");

Use Services.prefs.clearUserPref(prefName) here so you don't have to keep around the previous preference value.

Comment 7

[Andrew Hurle [:ahurle]]

(In reply to Jared Wein [:jaws] from comment #6)

> ::: browser/components/preferences/in-content/tests/browser_connection.js
> @@ +12,5 @@
> > +  let windowWatcher = Cc["@mozilla.org/embedcomp/window-watcher;1"]
> > +                          .getService(Components.interfaces.nsIWindowWatcher);
> > +
> > +  //instantApply must be true, otherwise connection dialog won't save
> > +  //when opened from in-content prefs
> 
> Has this been noticed on your machine? instantApply isn't explicitly set for
> in-content preferences because the root element of the in-content
> preferences has the special instantApply attribute. Does doing this help the
> test pass but leave a bug for the actual usage of the preferences?

Yes, if the instantApply pref is false the dialog never saves any changes here on Windows (even if I close the tab).  Setting instantApply to true makes the dialog save changes when I click OK.  I thought the instantApply pref was going to be changed to true by default according to your comment on bug 733473, but I should have read the rest of the referenced bug in the second comment.  document.documentElement.instantApply is indeed set to true according to scratchpad, but I guess that doesn't work.

> @@ +23,5 @@
> > +      if (aTopic == "domwindowopened") {
> > +        //when connection window loads, run tests and acceptDialog()
> > +        let win = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
> > +        win.addEventListener("load", function() {
> > +          win.removeEventListener("load", arguments.callee, false);
> 
> arguments.callee is deprecated. Please name the anonymous function above and
> use its name here.

Yeah, lazy copypasta from some other test.  I assume you want line 60 fixed as well?

> @@ +118,5 @@
> > +  networkProxyNonePref.value = ".a.com;.b.com\n.c.com";
> > +}
> > +
> > +function resetPreferences() {
> > +  let oldNoProxies = Services.prefs.getCharPref("network.proxy.no_proxies_on");
> 
> Use Services.prefs.clearUserPref(prefName) here so you don't have to keep
> around the previous preference value.

I was restoring the old values here because Mochitest apparently changes the proxy settings from default to facilitate other testing [1]. It definitely changes network.proxy.type, so I restored that from its previous value in order to not break other tests.  In the interest of robustness, I restored the old value of no_proxies_on, though that's not strictly necessary at the moment.  Does it sound okay to use clearUserPref on no_proxies_on and and instantApply, but restore the old value of proxy.type, or is there some reason to not bother restoring proxy.type?

If I can restore proxy.type, I should probably also add a comment explaining why I'm restoring the old value of that particular pref.

[1] https://developer.mozilla.org/en/Mochitest#How_it_works

Comment 8

[Andrew Hurle [:ahurle]]

Attachment: v2 - patch with jared's suggested changes

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 626211 [details] [diff] [review]
v2 - patch with jared's suggested changes

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

::: browser/components/preferences/in-content/tests/browser_connection.js
@@ +6,5 @@
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +function test() {
> +  waitForExplicitFinish();
> +  registerCleanupFunction(resetPreferences());

Can you cache the old value before the call to registerCleanupFunction, and then pass an anonymous inline function to registerCleanupFunction so it is a little easier to follow? There aren't any instances in the tree that I could find where we are calling a function that returns an anonymous function to registerCleanupFunction.

@@ +12,5 @@
> +  let windowWatcher = Cc["@mozilla.org/embedcomp/window-watcher;1"]
> +                          .getService(Components.interfaces.nsIWindowWatcher);
> +
> +  //instantApply must be true, otherwise connection dialog won't save
> +  //when opened from in-content prefs

General overall nit, can you please add a space between the // and the comment?

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Also, can you attach a screenshot of the dialog with your patch applied?

Comment 11

[Andrew Hurle [:ahurle]]

Attachment: Screenshot of proxy settings dialog with multiline patch applied

Comment 12

[Andrew Hurle [:ahurle]]

Attachment: v3 - spaces before comments, clearer cleanup function

I did make sure that the cleanup function still restores the old value of proxy.type.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 626257 [details]
Screenshot of proxy settings dialog with multiline patch applied

I looked through our UI for multiline textareas and all of the instances I could find all have the label above the textarea. Can you move the label to be located above the textarea and then make the text area wider to available space?

Comment 14

[Andrew Hurle [:ahurle]]

Attachment: Screenshot of proxy settings dialog with patch applied, label above textbox

Comment 15

[Andrew Hurle [:ahurle]]

Attachment: v4 - Patch with label above textbox

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 626498 [details] [diff] [review]
v4 - Patch with label above textbox

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

Looks good to me :)

Comment 17

[Andrew Hurle [:ahurle]]

Attachment: Successful test for opening/closing the pref and connection windows

Also, for that earlier patch 625298, I figured out that I can listen for the DOMModalDialogClosed event and I'll be able to properly accept both dialogs.  In the earlier patch, nsGlobalWindow::close thought that the main pref window still had the modal connection dialog open (IsInModalState() was true).  This prevented the window from closing.  LeaveModalState wasn't being called until sometime after the domwindowclosed notification from windowWatcher.

I don't think I'll bother making the test for out-of-content prefs since the patch just got reviewed, but this might be useful if we want to write similar browser chrome tests in the future.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/218771a2c9db
https://hg.mozilla.org/integration/mozilla-inbound/rev/4eaf67a7f50f

Comment 19

[Andrew Hurle [:ahurle]]

(In reply to Ryan VanderMeulen from comment #18)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4eaf67a7f50f

Ryan, that second patch shouldn't be checked in.  It's just here because it might be useful elsewhere.  jaws didn't review it (notice all the wonderful dumps and printfs).

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Sorry about that, it was a thinko on my part.
https://hg.mozilla.org/integration/mozilla-inbound/rev/963e816184b9

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/218771a2c9db