Consider keeping browser.preferences.instantApply = false on Windows

VERIFIED FIXED in Firefox 38

Status

()

Firefox
Preferences
P2
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: MattN, Assigned: Yash Mehrotra, Mentored)

Tracking

({addon-compat})

unspecified
Firefox 38
addon-compat
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite -
qe-verify +

Firefox Tracking Flags

(firefox38 verified)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

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.
Flags: firefox-backlog+
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

4 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.
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!
Whiteboard: [good first bug][lang=js]

Comment 4

4 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?
Flags: needinfo?(jaws)
(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.
Flags: needinfo?(jaws)
Marking as a gfbc for now until we can figure out what we want to do here.
Whiteboard: [good first bug][lang=js] → [gfbc][lang=js]
(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.
Mentor: jaws
Whiteboard: [gfbc][lang=js] → [good-first-bug][lang=js]

Updated

4 years ago
Whiteboard: [good-first-bug][lang=js] → [good first bug][lang=js]
(Assignee)

Comment 8

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

Comment 10

4 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
Flags: needinfo?(manishearth)
(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.
Priority: -- → P2
Points: --- → 3
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
Flags: needinfo?(manishearth)

Comment 13

4 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.
Whiteboard: [good first bug][lang=js] → [lang=js]

Updated

4 years ago
Whiteboard: [lang=js] → [good first bug][lang=js]
(Assignee)

Comment 14

4 years ago
Created attachment 8556453 [details] [diff] [review]
Patch - WIN_XP preferences for instantappy false for windows everywhere
Attachment #8556453 - Flags: review?(gijskruitbosch+bugs)

Comment 15

4 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.
Attachment #8556453 - Flags: review?(gijskruitbosch+bugs) → review-
(Assignee)

Comment 16

4 years ago
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.
Attachment #8556453 - Attachment is obsolete: true
Attachment #8556538 - Flags: review?(gijskruitbosch+bugs)

Comment 17

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

Comment 18

4 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!
Attachment #8556538 - Flags: review?(gijskruitbosch+bugs) → feedback+
Assignee: nobody → yashmehrotra95
Status: NEW → ASSIGNED
(Assignee)

Comment 19

4 years ago
Created attachment 8557994 [details] [diff] [review]
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.
Attachment #8556538 - Attachment is obsolete: true
Attachment #8557994 - Flags: review?(gijskruitbosch+bugs)

Comment 20

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

Comment 21

4 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.
Attachment #8557994 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

4 years ago
Mentor: jaws → gijskruitbosch+bugs
Flags: qe-verify+
Flags: in-testsuite-

Comment 22

4 years ago
Good on try -> checkin-needed. :-)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/de795be12119
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/de795be12119
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 38

Updated

4 years ago
Iteration: --- → 38.2 - 9 Feb
QA Contact: camelia.badau
Verified fixed using latest Nightly 38.0a1 (buildID: 20150205030205).
Status: RESOLVED → VERIFIED
status-firefox38: --- → verified

Comment 26

4 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
Flags: needinfo?(camelia.badau)

Comment 27

4 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).
Flags: needinfo?(camelia.badau)

Updated

4 years ago
Depends on: 1129947

Comment 28

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

Comment 29

4 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

Updated

4 years ago
No longer depends on: 1129947
(Assignee)

Comment 30

4 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.
You need to log in before you can comment on or make changes to this bug.