896091 - open a tab for users upgrading to Firefox 23 if we've reset their javascript prefs

Status: RESOLVED WONTFIX

(Firefox :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

Bug 851702 added code to reset the "Enable javascript" preference to its default value on upgrade to Firefox 23. To avoid upsetting users who've done that intentionally, we'd like to add code that will open a tab on upgrade pointing to an explanatory page describing the reasoning and some alternatives (e.g. NoScript).

(We considered other options, like a notification bar or some such, but past string freeze that's not feasible.)

Comment 1

[:Gavin Sharp [email: [e-mail]]]

We'll need the URL of the SUMO page to point to. That page should probably be written with two sets of users in mind:
- those who intentionally used this preference (and might be upset that we've removed the ability to do so)
- those who've done this accidentally and don't care. I'm not sure this set of users is very large, since their browser was probably mostly unusable and they probably would have switched to another.

Comment 2

[Scoobidiver (away)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #1)
> We'll need the URL of the SUMO page to point to.
Likely https://support.mozilla.org/kb/settings-fonts-languages-pop-ups-javascript (set the version picker to Firefox 23) or its new URL (see https://support.mozilla.org/en-US/kb/settings-fonts-languages-pop-ups-javascript/discuss/4905).

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Pretty straightforward - set a flag when resetting of the relevant prefs occurs, and then check that flag from within onWindowsRestored (i.e. when session store is done restoring all windows) to open the tab, similar to the many other paths where this currently happens (e.g. _showPluginUpdatePage). _migrateUI is garanteed to always be called before onWindowsRestored. 

The actual URL used is not necessarily final.

One thing that this highlights that I forgot about is that we're also resetting the preferences controlling image blocking and some DOM restrictions - it might be worth mentioning those in the page we're drafting as well.

Comment 4

[:Gijs (he/him)]

Comment on attachment 780027 [details] [diff] [review]
patch

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

Looks good, but that SUMO page is not the droid we're looking for as all mentions of this pref have simply been removed. Please ensure that that URL/Content actually, err, says something about what we did, before landing. If/when it does, please feel free to consider this an r+.

Comment 5

[:Gijs (he/him)]

Comment on attachment 780027 [details] [diff] [review]
patch

(In reply to :Gijs Kruitbosch from comment #4)
> Comment on attachment 780027 [details] [diff] [review]
> patch
> 
> Review of attachment 780027 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but that SUMO page is not the droid we're looking for as all
> mentions of this pref have simply been removed. Please ensure that that
> URL/Content actually, err, says something about what we did, before landing.
> If/when it does, please feel free to consider this an r+.

Oh blergh, I did read your comment before reviewing, but clearly not closely enough and/or I should just go to bed - you did say it was non-final. r+ with a useful URL/content, as this is less helpful than doing nothing ("Why did it open this page? What?")

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Laura: if we don't get the page set up soon (or at least the URL confirmed), we won't be able to get this done by Firefox 23.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Attachment: better patch

This is a better patch, in that it:
- avoids showing the tab for new profiles
- avoids showing the tab for upgrading users whose prefs weren't actually being reset (i.e. didn't have user values to begin with)

Doh!

Comment 8

[:Gijs (he/him)]

Comment on attachment 781835 [details] [diff] [review]
better patch

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

Thanks for catching that, and sorry I didn't! :X

::: browser/components/nsBrowserGlue.js
@@ +1444,5 @@
> +      ];
> +      // If this isn't a new profile and one of these prefs have a non-default
> +      // value, we want to open a tab notifying the user of the reset. Set a flag
> +      // here, _onWindowsRestored will take care of showing the tab later.
> +      if (currentUIVersion > 0 &&

Fascinating. I hadn't thought of this at all.

Now that you thought of it, I thought we'd need to check the other migrations, but I just went through them and from a quick look they cope with being called on a new profile. However, we could potentially short-circuit a bunch (maybe all?) of them? Maybe we should just change this function's contract and not call any of them for a new profile - the defaults should be sane already, and if they're not, that ought to be fixed. If you agree, please file a followup bug! :-)

(obvs this should still land if we want to fix the issue at hand as a more involved fix shouldn't go on branches)

@@ +1447,5 @@
> +      // here, _onWindowsRestored will take care of showing the tab later.
> +      if (currentUIVersion > 0 &&
> +          prefsToReset.some(Services.prefs.prefHasUserValue)) {
> +        this._showJSResetTab = true;
> +        prefsToReset.forEach(Services.prefs.clearUserPref);

I guess I would have gone with:

if (currentUIVersion > 0) {
  let modifiedPrefs = prefsToReset.filter(Services.prefs.prefHasUserValue);
  this._showJSResetTab = modifiedPrefs.length > 0;
  modifiedPrefs.forEach(Services.prefs.clearUserPref);
}

But that's personal preference; feel free to leave it as-is.

(Also, I'm assuming you tested this? ISTR our XPCOM code didn't like being called with extra arguments, and some of our lovely functional array code likes to pass extra arguments like the index into the array or somesuch)

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated better patch

(In reply to :Gijs Kruitbosch from comment #8)
> Now that you thought of it, I thought we'd need to check the other
> migrations, but I just went through them and from a quick look they cope
> with being called on a new profile. However, we could potentially
> short-circuit a bunch (maybe all?) of them? Maybe we should just change this
> function's contract and not call any of them for a new profile - the
> defaults should be sane already, and if they're not, that ought to be fixed.
> If you agree, please file a followup bug! :-)

I do! I filed bug 898575.

> I guess I would have gone with:

That is strictly better, updated (with a slight tweak).

> (Also, I'm assuming you tested this? ISTR our XPCOM code didn't like being
> called with extra arguments, and some of our lovely functional array code
> likes to pass extra arguments like the index into the array or somesuch)

Yes, I tested this manually and confirmed it works with:
- new profile
- upgrade with custom values for those prefs (by manually setting browser.migration.version=1)
- upgrade with no custom values (by manually setting browser.migration.version=1)

Comment 10

[Verdi [:verdi] Best to slack me]

The support article should be the one mentioned in comment 2 - https://support.mozilla.org/kb/settings-fonts-languages-pop-ups-javascript

I assume we'll need a special in-product link and not just a url (like we use for help links from the options window for example). In that case we need a sumo dev to set one up.

Comment 11

[:Gijs (he/him)]

(In reply to Verdi [:verdi] from comment #10)
> The support article should be the one mentioned in comment 2 -
> https://support.mozilla.org/kb/settings-fonts-languages-pop-ups-javascript
> 
> I assume we'll need a special in-product link and not just a url (like we
> use for help links from the options window for example). In that case we
> need a sumo dev to set one up.

Are you saying you will still change this article? Because as it is, it provides no clue to the users who we'll redirect here that (a) those prefs are now gone from that UI, (b) we've reset them for them, and (c) how to change them back if they really really need to.

Comment 12

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #11)
> (In reply to Verdi [:verdi] from comment #10)
> > The support article should be the one mentioned in comment 2 -
> > https://support.mozilla.org/kb/settings-fonts-languages-pop-ups-javascript
> > 
> > I assume we'll need a special in-product link and not just a url (like we
> > use for help links from the options window for example). In that case we
> > need a sumo dev to set one up.
> 
> Are you saying you will still change this article? Because as it is, it
> provides no clue to the users who we'll redirect here that (a) those prefs
> are now gone from that UI, (b) we've reset them for them, and (c) how to
> change them back if they really really need to.

And, thinking about it more, I don't think modifying the existing article is what we want here. We shouldn't be telling other users who google this article "oh by the way, this used to have other stuff and we got rid of it", that'll just be confusing.

Comment 13

[Verdi [:verdi] Best to slack me]

(In reply to :Gijs Kruitbosch from comment #11)
> Are you saying you will still change this article? Because as it is, it
> provides no clue to the users who we'll redirect here that (a) those prefs
> are now gone from that UI, (b) we've reset them for them, and (c) how to
> change them back if they really really need to.

Yes. Currently the article has a note shown to users of Firefox 23 (and above). Currently is says, "Note: If you need to disable JavaScript like you could in previous versions of Firefox, try using an extension like NoScript. For more info, see JavaScript settings and preferences for interactive web pages." 

I will add information about images to this note and probably change it's styling to be more noticeable (yellow).

(In reply to :Gijs Kruitbosch from comment #12)
> (In reply to :Gijs Kruitbosch from comment #11)
> > (In reply to Verdi [:verdi] from comment #10)
> > > The support article should be the one mentioned in comment 2 -
> > > https://support.mozilla.org/kb/settings-fonts-languages-pop-ups-javascript
> > > 
> > > I assume we'll need a special in-product link and not just a url (like we
> > > use for help links from the options window for example). In that case we
> > > need a sumo dev to set one up.
> > 
> > Are you saying you will still change this article? Because as it is, it
> > provides no clue to the users who we'll redirect here that (a) those prefs
> > are now gone from that UI, (b) we've reset them for them, and (c) how to
> > change them back if they really really need to.
> 
> And, thinking about it more, I don't think modifying the existing article is
> what we want here. We shouldn't be telling other users who google this
> article "oh by the way, this used to have other stuff and we got rid of it",
> that'll just be confusing.

Only people using Firefox 23 and above will see that information. So we do want it here in this article and for people searching with google to find it.

Comment 14

[:Gijs (he/him)]

(In reply to Verdi [:verdi] from comment #13)
> (In reply to :Gijs Kruitbosch from comment #11)
> > Are you saying you will still change this article? Because as it is, it
> > provides no clue to the users who we'll redirect here that (a) those prefs
> > are now gone from that UI, (b) we've reset them for them, and (c) how to
> > change them back if they really really need to.
> 
> Yes. Currently the article has a note shown to users of Firefox 23 (and
> above). Currently is says, "Note: If you need to disable JavaScript like you
> could in previous versions of Firefox, try using an extension like NoScript.
> For more info, see JavaScript settings and preferences for interactive web
> pages." 
> 
> I will add information about images to this note and probably change it's
> styling to be more noticeable (yellow).

Sounds like a plan - I missed it, sorry! I still wonder if the page we show users should mention that we've changed their settings for them, otherwise they might not understand why we've opened that tab for them...

> (In reply to :Gijs Kruitbosch from comment #12)
> > (In reply to :Gijs Kruitbosch from comment #11)
> > > (In reply to Verdi [:verdi] from comment #10)
> > > > The support article should be the one mentioned in comment 2 -
> > > > https://support.mozilla.org/kb/settings-fonts-languages-pop-ups-javascript
> > > > 
> > > > I assume we'll need a special in-product link and not just a url (like we
> > > > use for help links from the options window for example). In that case we
> > > > need a sumo dev to set one up.
> > > 
> > > Are you saying you will still change this article? Because as it is, it
> > > provides no clue to the users who we'll redirect here that (a) those prefs
> > > are now gone from that UI, (b) we've reset them for them, and (c) how to
> > > change them back if they really really need to.
> > 
> > And, thinking about it more, I don't think modifying the existing article is
> > what we want here. We shouldn't be telling other users who google this
> > article "oh by the way, this used to have other stuff and we got rid of it",
> > that'll just be confusing.
> 
> Only people using Firefox 23 and above will see that information. So we do
> want it here in this article and for people searching with google to find it.

Fair, but I guess we should, if possible, show it to users that get this as part of the first-run experience in a different way than to "ordinary" or "googling" users, if that makes sense. Which is why I thought a separate page would be simpler...

Comment 15

[Verdi [:verdi] Best to slack me]

(In reply to :Gijs Kruitbosch from comment #14)
> Fair, but I guess we should, if possible, show it to users that get this as
> part of the first-run experience in a different way than to "ordinary" or
> "googling" users, if that makes sense. Which is why I thought a separate
> page would be simpler...

I thought we were only showing this to people who upgrade and are having these prefs changed because of that. Or did I misunderstand?

Comment 16

[:Gijs (he/him)]

(In reply to Verdi [:verdi] from comment #15)
> (In reply to :Gijs Kruitbosch from comment #14)
> > Fair, but I guess we should, if possible, show it to users that get this as
> > part of the first-run experience in a different way than to "ordinary" or
> > "googling" users, if that makes sense. Which is why I thought a separate
> > page would be simpler...
> 
> I thought we were only showing this to people who upgrade and are having
> these prefs changed because of that. Or did I misunderstand?

You did not; I expressed myself poorly. I meant first-run-after-upgrade, so yes, upgrading users.

Comment 17

[Verdi [:verdi] Best to slack me]

(In reply to :Gijs Kruitbosch from comment #16)
> I meant first-run-after-upgrade, so
> yes, upgrading users.
Ah, now I understand. Laura and I talked about that yesterday over video and felt that this solution was good.

Comment 18

[Scoobidiver (away)]

(In reply to Verdi [:verdi] from comment #13)
> I will add information about images to this note and probably change it's
> styling to be more noticeable (yellow).
I didn't do that when I updated the article because I haven't seen any bug complaining about the missing image checkbox while there were a lot about JavaScript. If you do so, you can advice to use https://addons.mozilla.org/firefox/addon/image-block (one click - better than the Page Info window in five clicks).

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

We're going to build on the FF23 beta shortly and this hasn't landed yet, nor been confirmed to be needed.  Given that we expect a very small number of people to be impacted here, I'm wontfixing this. If there are any concerns about this please bring them to tomorrow morning's channel meeting or email me directly.

Comment 20

[Laura Forrest]

(In reply to [e-mail] [:lsblakk] from comment #19)
> We're going to build on the FF23 beta shortly and this hasn't landed yet,
> nor been confirmed to be needed.  Given that we expect a very small number
> of people to be impacted here, I'm wontfixing this. If there are any
> concerns about this please bring them to tomorrow morning's channel meeting
> or email me directly.

I can confirm it's still needed since we're still switching the JavaScript preference w/o letting those users know.

The implementation piece is another matter.