1037225 - Consider keeping browser.preferences.instantApply = false on Windows

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P2)

Description

[Matthew N. [:MattN]]

While Gijs was investigating bug 1035308, it came to our attention that bug 738797 changed the default of browser.preferences.instantApply for Windows may be fine with in-content preferences but may not be fine with extension or other pref windows outside of about:preferences since browser.preferences.instantApply = false removes the OK/Cancel buttons and applies changes immediately which may not be expected for Windows users.

I think we should try to make the in-content preferences work without changing the pref to avoid compat. problems and provide an interaction that is more natural for Windows.

Comment 1

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

If we do decide to go this route, we may be fine with undoing this pref change and nothing else since http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/preferences.js?rev=7a7f6701c903#23 already sets document.documentElement.instantApply = false for the preferences document.

I'm not sure if this will be inherited into the special iframe tab-modal dialogs though.

Comment 2

[:Gijs (out for now; he/him)]

(In reply to Jared Wein [:jaws] [Away for most of July] (please needinfo? me) from comment #1)
> I'm not sure if this will be inherited into the special iframe tab-modal
> dialogs though.

It doesn't, so we'd need to fix that.

Comment 3

[Manish Goregaokar [:manishearth]]

New contributors:

Here we mostly need to take the #ifded XP_WIN section and move it out of the #ifndef RELEASE_BUILD section

http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js?rev=0a6dbb9910a1#883

Let me know if you have any more questions!

Comment 4

[:Gijs (out for now; he/him)]

(In reply to Manish Goregaokar [:manishearth] from comment #3)
> New contributors:
> 
> Here we mostly need to take the #ifded XP_WIN section and move it out of the
> #ifndef RELEASE_BUILD section
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.
> js?rev=0a6dbb9910a1#883
> 
> Let me know if you have any more questions!

That's not enough for this bug, and I'm not sure this is suitable as a good first bug... Jared?

Comment 5

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

(In reply to :Gijs Kruitbosch from comment #2)
> (In reply to Jared Wein [:jaws] [Away for most of July] (please needinfo?
> me) from comment #1)
> > I'm not sure if this will be inherited into the special iframe tab-modal
> > dialogs though.
> 
> It doesn't, so we'd need to fix that.

Actually, I'm hearing from UX that we don't want instantApply in the tab-modal prompts, since they shouldn't apply until they are closed. With that, I guess this bug is a lot simpler.

Comment 6

[Manish Goregaokar [:manishearth]]

Marking as a gfbc for now until we can figure out what we want to do here.

Comment 7

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

(In reply to Manish Goregaokar [:manishearth] from comment #6)
> Marking as a gfbc for now until we can figure out what we want to do here.

https://bugzilla.mozilla.org/show_bug.cgi?id=1032790#c7 states that these sub-dialogs should not auto-apply. So it appears that this bug is good to move forward.

Comment 8

[Yash Mehrotra]

Hello, I would like to work on this bug, I have already set up the firefox development environment. From the above comments, I was able to understand this to some extent.

Comment 9

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

(In reply to Yash Mehrotra from comment #8)
> Hello, I would like to work on this bug, I have already set up the firefox
> development environment. From the above comments, I was able to understand
> this to some extent.

Hi Yash, that sounds good. Please attach a patch to this file when you are ready to get some feedback.

Comment 10

[Yash Mehrotra]

Even though comment #3 proposes a possible fix, but it is worth noting that the reference to [0] does not exist now. (it is about an older file version).

> Here we mostly need to take the #ifded XP_WIN section and move it out of the #ifndef RELEASE_BUILD section

The latest file revision [1] already satisfies the proposed fix

[0] : http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js?rev=0a6dbb9910a1#883
[1] : http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#857

Comment 11

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

(In reply to Yash Mehrotra from comment #10)
> The latest file revision [1] already satisfies the proposed fix

The latest file revision only satisfies the proposed fix for Release users, which aren't using in-content preferences. Everywhere that in-content preferences is enabled still uses instantApply=true on Windows.

Comment 12

[Manish Goregaokar [:manishearth]]

http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#852

>     #ifdef EARLY_BETA_OR_EARLIER

We need to move the pref outside of this block

Comment 13

[:Gijs (out for now; he/him)]

(In reply to Manish Goregaokar [:manishearth] from comment #12)
> http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.
> js#852
> 
> >     #ifdef EARLY_BETA_OR_EARLIER
> 
> We need to move the pref outside of this block

No, the pref needs to be removed from that block, and this bit:

857 #ifdef XP_WIN
858 pref("browser.preferences.instantApply", false);
859 #else
860 pref("browser.preferences.instantApply", true);
861 #endif

needs to move outside of the #else part of that block.

The result here should be that the instantApply pref is false on Windows no matter whether we're RELEASE, NIGHTLY, EARLY_BETA_OR_EARLIER or whatever, and true everywhere else.

Comment 14

[Yash Mehrotra]

Attachment: Patch - WIN_XP preferences for instantappy false for windows everywhere

Comment 15

[:Gijs (out for now; he/him)]

Comment on attachment 8556453 [details] [diff] [review]
Patch - WIN_XP preferences for instantappy false for windows everywhere

No, this also needs to remove this line:

 pref("browser.preferences.instantApply", true);

from the #ifdef EARY_BETA_OR_EARLIER block, as noted in comment 13.

Ideally you should then also test that that works, iow, that the in-content prefs on Nightly will still have no OK/Cancel/Apply buttons on Windows, but that the dialogs in those in-content prefs *do* have the buttons, and that the prefs work accordingly.

Comment 16

[Yash Mehrotra]

Attachment: Patch Updated

I have updated the patch, also:

> Ideally you should then also test that that works, iow, that the in-content
> prefs on Nightly will still have no OK/Cancel/Apply buttons on Windows, but
> that the dialogs in those in-content prefs *do* have the buttons, and that
> the prefs work accordingly.

Unfortunately, I am unable to test the latest patch since I only have Linux installed in my machine.

Comment 17

[:Gijs (out for now; he/him)]

(In reply to Yash Mehrotra from comment #16)
> Created attachment 8556538 [details] [diff] [review]
> Patch Updated
> 
> I have updated the patch, also:
> 
> > Ideally you should then also test that that works, iow, that the in-content
> > prefs on Nightly will still have no OK/Cancel/Apply buttons on Windows, but
> > that the dialogs in those in-content prefs *do* have the buttons, and that
> > the prefs work accordingly.
> 
> Unfortunately, I am unable to test the latest patch since I only have Linux
> installed in my machine.

OK. I will test this tomorrow. In theory the patch looks good, but this should really get tested before landing. :-)

Comment 18

[:Gijs (out for now; he/him)]

Comment on attachment 8556538 [details] [diff] [review]
Patch Updated

So, this is the right thing for the preferences themselves, as I said in my previous comment.

However... it doesn't actually work, unfortunately. You can check this on Linux by setting the instantApply pref to false in about:config manually.

Then open the preferences, go to the content pane, and click "Advanced..." next to the fonts. Configure some other fonts. Click OK. Reopen the dialog. Note that now all of the preferences have gone back to what they were before... (if you have a web page with default fonts in use open in a separate tab, and don't close the preferences tab but switch to the other page, you'll also see that the fonts there don't update).

The expected results right now would be that the fonts change immediately after clicking OK, whereas when you turn off in-content preferences and the preferences are in a dialog, they should only be saved once the parent window is accepted.

Anyway, the current preference window binding implements the second behaviour. It checks the instantApply pref to decide whether to save immediately or put the preferences in the parent window here:

http://hg.mozilla.org/mozilla-central/annotate/29b05d283b00/toolkit/content/widgets/preferences.xml#l1064

1064           var psvc = Components.classes["@mozilla.org/preferences-service;1"]
1065                                .getService(Components.interfaces.nsIPrefBranch);
1066           var instantApply = psvc.getBoolPref("browser.preferences.instantApply");
1067           if (instantApply) {
1068             var panes = this.preferencePanes;
1069             for (var i = 0; i < panes.length; ++i)
1070               panes[i].writePreferences(true);
1071           }
1072           else {
1073             // Clone all the preferences elements from the child document and
1074             // insert them into the pane collection of the parent.
<rest of the code omitted for brevity's sake>

You want this to take the first branch, ie to call writePreferences on all the things, instead of the current behaviour (where it takes the second approach and clones all the things).

In order for that to happen, instead of checking the pref service for the instantApply pref, it needs to check *the parent window* (window.opener) to see if it's instantApply or not. In particular, you can remove the pref service declaration and instead use:

            var pdocEl = window.opener.document.documentElement;
            if (pdocEl.instantApply) {

and that should make it work.

Can you try this out and test it on Linux by chancing the instantApply pref in about:config, and verify that it fixes this issue? Thanks!

Comment 19

[Yash Mehrotra]

Attachment: 1037225.diff

(In reply to :Gijs Kruitbosch from comment #18)

I have made the changes in preferences.xml as you said.

> However... it doesn't actually work, unfortunately. You can check this on
> Linux by setting the instantApply pref to false in about:config manually.
> 
> Then open the preferences, go to the content pane, and click "Advanced..."
> next to the fonts. Configure some other fonts. Click OK. Reopen the dialog.
> Note that now all of the preferences have gone back to what they were
> before... (if you have a web page with default fonts in use open in a
> separate tab, and don't close the preferences tab but switch to the other
> page, you'll also see that the fonts there don't update).

After applying the patch and setting instantApply pref to false in about:config manually, the font settings did not go back to default on reopening the dialog, they were the set to my settings. Also font is changing in the other web-page as in the settings.

> 
> The expected results right now would be that the fonts change immediately
> after clicking OK, whereas when you turn off in-content preferences and the
> preferences are in a dialog, they should only be saved once the parent
> window is accepted.
> 
> Anyway, the current preference window binding implements the second
> behaviour. It checks the instantApply pref to decide whether to save
> immediately or put the preferences in the parent window here:
> 
> http://hg.mozilla.org/mozilla-central/annotate/29b05d283b00/toolkit/content/
> widgets/preferences.xml#l1064
> 

Before the patch, the settings were not being saved, after building it again with the patch (and keeping insantApply to false) the font is being changed instantly on the other page(that takes default fonts)

Please tell me if any other changes are required.

Comment 20

[:Gijs (out for now; he/him)]

Comment on attachment 8557994 [details] [diff] [review]
1037225.diff

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

In principle this looks right to me. I'll do a final review/check tomorrow morning (GMT).

Comment 21

[:Gijs (out for now; he/him)]

Comment on attachment 8557994 [details] [diff] [review]
1037225.diff

Looks good, thanks!

I pushed this to try for you:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=06473e7adaf9

Assuming it passes test, this is good to land.

Comment 22

[:Gijs (out for now; he/him)]

Good on try -> checkin-needed. :-)

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/de795be12119

Comment 25

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed using latest Nightly 38.0a1 (buildID: 20150205030205).

Comment 26

[Alice0775 White]

(In reply to Camelia Badau, QA [:cbadau] from comment #25)
> Verified fixed using latest Nightly 38.0a1 (buildID: 20150205030205).

How I can verify this?

The prefs in about:config seems to be changed instantly when I changed the related items in in-content preferences.


https://hg.mozilla.org/mozilla-central/rev/34a66aaaca81
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0 ID:20150205030205

Comment 27

[:Gijs (out for now; he/him)]

(In reply to Alice0775 White from comment #26)
> (In reply to Camelia Badau, QA [:cbadau] from comment #25)
> > Verified fixed using latest Nightly 38.0a1 (buildID: 20150205030205).
> 
> How I can verify this?
> 
> The prefs in about:config seems to be changed instantly when I changed the
> related items in in-content preferences.

For items not in the dialogs, that is exactly what happened before, and that's intentional.

Basically, there should be no behaviour change with previous in-content prefs behaviour, but the pref should be back to false (so that add-ons that open preference windows are not affected by in-content prefs).

Comment 28

[:Gijs (out for now; he/him)]

Yash, congrats on your first fixed bug! Maybe you'd like to look at e.g. bug 1129529 next? :-)

Comment 29

[Alice0775 White]

(In reply to :Gijs Kruitbosch from comment #27)
> (In reply to Alice0775 White from comment #26)
> > (In reply to Camelia Badau, QA [:cbadau] from comment #25)
> > > Verified fixed using latest Nightly 38.0a1 (buildID: 20150205030205).
> > 
> > How I can verify this?
> > 
> > The prefs in about:config seems to be changed instantly when I changed the
> > related items in in-content preferences.
> 
> For items not in the dialogs, that is exactly what happened before, and
> that's intentional.
> 
> Basically, there should be no behaviour change with previous in-content
> prefs behaviour, but the pref should be back to false (so that add-ons that
> open preference windows are not affected by in-content prefs).

Hmmmm, It is not consistent across the main preferences panels and the sub-dialog(incl. add-ons's dialog)....

Anyway, I filed Bug 1129947

Comment 30

[Yash Mehrotra]

(In reply to :Gijs Kruitbosch from comment #28)
> Yash, congrats on your first fixed bug! Maybe you'd like to look at e.g. bug
> 1129529 next? :-)

Thanks Gijs, I will be looking into it.