Matthew N. [:MattN] Reporter
	Description • 10 years ago

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.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 1 • 10 years ago

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.

	:Gijs (he/him)
	Comment 2 • 10 years ago

(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.

	Manish Goregaokar [:manishearth]
	Comment 3 • 10 years ago

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!

	:Gijs (he/him)
	Comment 4 • 10 years ago

(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?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 5 • 10 years ago

(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.

	Manish Goregaokar [:manishearth]
	Comment 6 • 10 years ago

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

	Jared Wein [:jaws] (please needinfo? me)
	Comment 7 • 10 years ago

(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.

	Yash Mehrotra Assignee
	Comment 8 • 9 years ago

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.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 9 • 9 years ago

(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.

	Yash Mehrotra Assignee
	Comment 10 • 9 years ago

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

	Jared Wein [:jaws] (please needinfo? me)
	Comment 11 • 9 years ago

(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.

	Manish Goregaokar [:manishearth]
	Comment 12 • 9 years ago

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

	:Gijs (he/him)
	Comment 13 • 9 years ago

(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.

	:Gijs (he/him)
	Comment 15 • 9 years ago

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.

	Yash Mehrotra Assignee
	Comment 16 • 9 years ago

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.

	:Gijs (he/him)
	Comment 17 • 9 years ago

(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. :-)

	:Gijs (he/him)
	Comment 18 • 9 years ago

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!

	Yash Mehrotra Assignee
	Comment 19 • 9 years ago

(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.

	:Gijs (he/him)
	Comment 20 • 9 years ago

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).

	:Gijs (he/him)
	Comment 21 • 9 years ago

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.

	:Gijs (he/him)
	Comment 22 • 9 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 23 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 24 • 9 years ago

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

	Camelia Badau [:cbadau], Desktop QA
	Comment 25 • 9 years ago

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

	Alice0775 White
	Comment 26 • 9 years ago

(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

	:Gijs (he/him)
	Comment 27 • 9 years ago

(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).

	:Gijs (he/him)
	Comment 28 • 9 years ago

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

	Alice0775 White
	Comment 29 • 9 years ago

(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

	Yash Mehrotra Assignee
	Comment 30 • 9 years ago

(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.