339720 - show warning page before showing about:config

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

[Mike Beltzner [:beltzner, not reading bugmail]]

Increasingly, web pages and users are pointing each other to about:config as a way of tweaking various settings. This is all well and good, except that we don't really give any indication of how the settings in about:config can truly pooch a user's installation.

This RFE is suggesting that we add a small warning page, much like Windows XP shows users the first time they navigate to C:\Windows, which explains that the settings in about:config are set to certain defaults, and that they should be careful when changing them. Optionally we can link to websites like the Mozillazine KnowledgeBase article about the settings themselves (http://kb.mozillazine.org/Firefox_:_FAQs_:_About:config_Entries) and perhaps even offer a big ol' button to restore all defaults.

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: proof of concept

Not yet localized, pretty ugly.  Comments?

Comment 2

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Less ugly, more skinnable

Screenshot at http://ctho.ath.cx/tmp/aboutConfigWarning.png for those who don't build SeaMonkey and don't feel like applying it to the toolkit version.  Maybe someone more css-inclined can make it look nicer.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 224290 [details] [diff] [review]
Less ugly, more skinnable

I think this is definitely a good approach to protecting users and a good start. I especially like the idea of linking to relevant help docs and the restore all defaults button - would that also clear any user created (ie: in user.js) prefs?

I think to be realistic, though, we should target this at Gecko 1.9. :)

Comment 4

[[e-mail]]

Comment on attachment 224290 [details] [diff] [review]
Less ugly, more skinnable

Bugzilla seems to have eaten my comments. I hope I can remember them all...

>+<deck id="configDeck" flex="1">
>+<vbox id="warningScreen" flex="1" align="center">
Don't need flex inside a deck.

>+  <hbox id="warningBox" align="top">
align="top" is deprecated. Use align="start" instead.

>+    <image id="exclam" src="chrome://global/skin/icons/alert-exclam.gif"/>
Not skinnable. Use class="alert-icon" instead, and no src in chrome ever.

>+      <hbox id="warningSubBox" align="right">
>+        <spacer flex="1"/>
I think you mean pack="end" here, and no spacer.

>+      <checkbox id="showWarningNextTime" label="&scaryWarningCheckbox.label;"/>
Needs checked="true" here, rather than in the JS.

>+<vbox flex="1">
flex again.

>+<hbox id="filterRow" align="center">
Why the id?

> </tree>
>+</vbox>
>+</deck>
> </window>
Is this a -w diff ;-)

>Index: mozilla/xpfe/global/resources/content/config.css
This isn't skinnable.

>+  document.getElementById("configDeck").setAttribute("selectedIndex", 1);
>+  var showNextTime = document.getElementById("showWarningNextTime").checked;
>+  gPrefBranch.setBoolPref("general.warnOnAboutConfig", showNextTime);
As an alternative to using prefs consider
document.getElementById("configDeck").setAttribute("selectedIndex", "1");
if (!document.getElementById("showWarningNextTime").checked)
  document.persist("configDeck", "selectedIndex");

Comment 5

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #4)
> >+<deck id="configDeck" flex="1">
> >+<vbox id="warningScreen" flex="1" align="center">
> Don't need flex inside a deck.

Fixed

> >+  <hbox id="warningBox" align="top">
> align="top" is deprecated. Use align="start" instead.

Fixed

> >+    <image id="exclam" src="chrome://global/skin/icons/alert-exclam.gif"/>
> Not skinnable. Use class="alert-icon" instead, and no src in chrome ever.

Fixed, sorry, that was laziness, that version was sort of a proof of concept.

> >+      <hbox id="warningSubBox" align="right">
> >+        <spacer flex="1"/>
> I think you mean pack="end" here, and no spacer.

Fixed (actually, modified to look better)

> >+      <checkbox id="showWarningNextTime" label="&scaryWarningCheckbox.label;"/>
> Needs checked="true" here, rather than in the JS.

Fixed

> >+<vbox flex="1">
> flex again.

Fixed

> >+<hbox id="filterRow" align="center">
> Why the id?

Toolkit has it

> > </tree>
> >+</vbox>
> >+</deck>
> > </window>
> Is this a -w diff ;-)

Fixed

> >Index: mozilla/xpfe/global/resources/content/config.css
> This isn't skinnable.

Fixed

> >+  document.getElementById("configDeck").setAttribute("selectedIndex", 1);
> >+  var showNextTime = document.getElementById("showWarningNextTime").checked;
> >+  gPrefBranch.setBoolPref("general.warnOnAboutConfig", showNextTime);
> As an alternative to using prefs consider
> document.getElementById("configDeck").setAttribute("selectedIndex", "1");
> if (!document.getElementById("showWarningNextTime").checked)
>   document.persist("configDeck", "selectedIndex");

But then there's no easy way for someone to cause the warning to show up again, right?  I think that's useful to have.

Comment 6

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: even less ugly, address review issues

Neil, I'm looking for review on everything but the text strings.

Comment 7

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Note that it probably won't apply directly, because I added a file and diffing new files is too hard.  Chop off the last part and it should work normally.

Comment 8

[[e-mail]]

Comment on attachment 232854 [details] [diff] [review]
even less ugly, address review issues

> @import url("chrome://global/skin/");
>+@import url("chrome://global/skin/config.css");
config.css should import global.css (see filepicker.css).

>+  var showWarning = true;
>+  try {
>+    showWarning = gPrefBranch.getBoolPref("general.warnOnAboutConfig");
>+  } catch (e) {}
Still not setting a default for this pref?

>+  if (showWarning) {
>+    document.getElementById("configDeck").setAttribute("selectedIndex", 0);
>+  }
>+  else {
>+    document.getElementById("configDeck").setAttribute("selectedIndex", 1);
>+  }
Why not use showWarning ? "0" : "1"

>+  <vbox>
>+  <hbox id="filterRow" align="center">
>+  </hbox>
>+  <tree id="configTree" flex="1" class="plain focusring" seltype="single"
>+        enableColumnDrag="true" context="configContext">
>+  </tree>
>+</vbox>
>+</deck>
> </window>
Nit: indentation still not quite right.

>Config.css follows (same for modern and classic)
No it's not (see netError.css).

Comment 9

[[e-mail]]

Comment on attachment 232854 [details] [diff] [review]
even less ugly, address review issues

Oh, and you also need to do something about focus (by default config.js focuses the filter textbox).

Comment 10

[[e-mail]]

Comment on attachment 232854 [details] [diff] [review]
even less ugly, address review issues

And I don't think we should load the tree until we show it.

Comment 11

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Comment 12

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: diff -w

Comment 13

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Just so we don't lose these...

<Neil> CTho: just looking at it, I have two nits
<Neil> CTho: the first is I said that you don't need to set the warning checkbox in JS but you didn't remove that line
<Neil> CTho: the second is that the nice new ShowPrefs function means that you don't need to set the deck index in onConfigLoad
<Neil> CTho: actually, spanish inquisition time - I missed two more nits
<Neil> CTho: the third is that the ShowPrefs function should be nearer onConfigLoad so that you don't lose history by moving the lines
<Neil> CTho: the fourth is that I don't think you can compare the tree view to null, instead you should check the selected index or something

Comment 14

[[e-mail]]

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

>+    ShowPrefs();
>+    document.getElementById("textbox").focus();
ShowPrefs() should do the focusing, for when you click the button.

>+      <vbox>
>+        <label id="warningTitle" value="&scaryWarning.label;"/>
>+        <label id="warningText" value="&scaryWarningText.label;"/>
>+        <button id="warningButton" oncommand="ShowPrefs();" label="&scaryWarningButton.label;" flex="0"/>
>+        <checkbox id="showWarningNextTime" label="&scaryWarningCheckbox.label;" checked="true"/>
>+      </vbox>
Your button is now the width of the box. flex="0" doesn't affect this.
[Should the button be after the checkbox, like it is in prompts?]

Comment 15

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

> <Neil> CTho: the first is I said that you don't need to set the warning
> checkbox in JS but you didn't remove that line
Fixed

> <Neil> CTho: the second is that the nice new ShowPrefs function means that you
> don't need to set the deck index in onConfigLoad
Fixed

> <Neil> CTho: the third is that the ShowPrefs function should be nearer
> onConfigLoad so that you don't lose history by moving the lines
Fixed

> <Neil> CTho: the fourth is that I don't think you can compare the tree view to
> null, instead you should check the selected index or something
Fixed

> ShowPrefs() should do the focusing, for when you click the button.
Fixed

> Your button is now the width of the box. flex="0" doesn't affect this.
> [Should the button be after the checkbox, like it is in prompts?]
Fixed and fixed

I also updated the CSS and made some other tweaks to the XUL to get better behavior and closer matching of netError.xhtml's look and behavior as the window is resized.

Comment 16

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: diff -w

Comment 17

[[e-mail]]

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

>+<!ENTITY scaryWarning.label "Be careful!">
>+<!ENTITY scaryWarningText.label "This configuration page contains settings that keep &brandShortName; working properly.  Modifying these settings improperly may cause problems such as instability, broken features, or even security vulnerabilities.">
>+<!ENTITY scaryWarningButton.label "I understand and wish to continue">
>+<!ENTITY scaryWarningCheckbox.label "Show this warning next time">
Much better wording than before ;-) but I'd like jag to see it anyway

>Index: mozilla/xpfe/global/resources/content/config.css
jag, should we move all of these styles into the skin?

>+  else {
>+    ShowPrefs();
>+  }
Lose the {}s

>+  document.getElementById("configDeck").setAttribute("selectedIndex", 1);
>+  if (document.getElementById("configDeck").selectedIndex == 1) {
Nit: Inconsistent

>+        <label id="warningTitle">&scaryWarning.label;</label>
>+        <label id="warningText">&scaryWarningText.label;</label>
KaiRo, should that be scaryWarningTitle.label?

>+        <hbox pack="center"><button id="warningButton" oncommand="ShowPrefs();" label="&scaryWarningButton.label;"/></hbox>
Don't put these on one line

Comment 18

[jag (Peter Annema)]

(In reply to comment #17)
> (From update of attachment 236727 [details] [diff] [review] [edit])
> >+<!ENTITY scaryWarningText.label "This configuration page contains settings that keep &brandShortName; working properly.  Modifying these settings improperly may cause problems such as instability, broken features, or even security vulnerabilities.">
> Much better wording than before ;-) but I'd like jag to see it anyway

It doesn't feel right. "Modifying improperly" to me suggests the interface provides me a proper and an improper way to modify settings and I need to make sure I only do it the proper way. Maybe just drop the "improperly"?


> >Index: mozilla/xpfe/global/resources/content/config.css
> jag, should we move all of these styles into the skin?

The content of content/config.css does seem a very skin-y thing.

Comment 19

[Robert Kaiser]

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

> >+        <label id="warningTitle">&scaryWarning.label;</label>
> >+        <label id="warningText">&scaryWarningText.label;</label>
> KaiRo, should that be scaryWarningTitle.label?

The current entity name looks fine as well, even though scaryWarningTitle.label would make it even clearer.

Comment 20

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Addresses review comments.

I'm not sure if neil preferred having jag sr or was just asking the question.

Comment 21

[[e-mail]]

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

>+<!ENTITY scaryWarningText.label "This configuration page contains settings that keep &brandShortName; working properly.  Modifying these settings may cause problems such as instability, broken features, or even security vulnerabilities.">
How about data loss?

>+pref("general.warnOnAboutConfig", true);
I guess all.js suitable if you want to port this to toolkit.

>+AAAAAAAAAAAAAAAAAA (just ensuring this won't compile))
You don't compile CSS :-P You do need to kill it from the jar.mn though.

> }
> 
>+
Bogus blank line.

>+/* ::::: tree rows ::::: */
>+
>+treechildren::-moz-tree-cell-text(user)
>+{
>+  font-weight: bold;
>+}
>+
>+treechildren::-moz-tree-cell-text(locked)
>+{
>+  font-style: italic;
>+}
>+
>+#warningScreen 
Nit: Trailing whitespace (1 of 4, according to jst-review simulacrum)
Either add a /* ::::: warning screen ::::: */ separator comment or
put the warning screen styles first or possibly even both.

r=me with these fixed.

Comment 22

[jag (Peter Annema)]

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

New patch with nits addressed/fixed, please.

Comment 23

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: fix nits

This fixes Neil's nits, as well as a JST Review Simulacrum nit about putting "{" on its own line for methods.

Comment 24

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Moving to core since I'm fixing both products

Comment 25

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: firefox patch

Straight port.  Seems to work with brief testing.  Someone else gets to figure out how it should look for your various themes in toolkit - I only patched winstripe.

Comment 26

[Chris Lawson (gone)]

This busted about:config in Camino; see bug 352425.

Comment 27

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 237727 [details] [diff] [review]
fix nits

Requesting approval from drivers because I touch all.js

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 237727 [details] [diff] [review]
fix nits

approval-seamonkey1.1b=biesi for the seamonkey-specific parts (seems to be everything except all.js)

Comment 29

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 237727 [details] [diff] [review]
fix nits

Cancelling approval request.  There's a way to do this entirely within XPFE for branch.

Comment 30

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Moved the pref from all.js to browser-prefs.js with r=db48x, got a=biesi again, checked in on 1.8 branch.  Included regression fix in bug 352425 to avoid breaking camino.

Comment 31

[Michael Ventnor]

I apologize for the bugspam, but this just seems over the top:
"This configuration page contains settings that keep &brandShortName; working properly.  Modifying these settings may cause problems such as instability, data loss, broken features, or even security vulnerabilities."

We shouldn't be scaring the living daylights out of them. The warning should be more subtle like this:

"This configuration page contains many internal settings for &brandShortName;. You should not modify these settings unless you are sure about what you want to do. Changing some settings incorrectly may cause harmful side effects for &brandShortName;'s functionality.

For more information about this page, please visit the Knowledge Base: http://kb.mozillazine.org/About:config

Are you sure you wish to continue?"

While it isn't the best wording, I hope I got my point across. A link to the knowledge base opening in a new tab would be a good idea IMO because it contains articles about about:config and many of the preferences.

Comment 32

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 237872 [details] [diff] [review]
firefox patch

Obsoleting; going to try a different reviewer [with updated patch].

Comment 33

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: updated firefox patch

This includes fixes for the regression (busted embeddors) / incorrect behavior (didn't remember the pref properly - warning came back after third time using about:config).

Comment 34

[Asaf Romano (gone)]

Comment on attachment 243145 [details] [diff] [review]
updated firefox patch

for reason mentioned on irc .

Comment 35

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: updated firefox patch w/pinstripe themed

Changed the margin as requested.  Copied style to pinstripe.  http://www.adggda.com/~ss/seamonkey/about-config.png - the SeaMonkey code looks OK on mac.

Comment 36

[Asaf Romano (gone)]

Comment on attachment 243147 [details] [diff] [review]
updated firefox patch w/pinstripe themed

>Index: mozilla/toolkit/components/viewconfig/content/config.js
>===================================================================

>+  var showWarning = gPrefBranch.getBoolPref("general.warnOnAboutConfig");
>+
>+  if (showWarning)
>+    document.getElementById("warningButton").focus();
>+  else
>+    ShowPrefs();
>+}
>+
>+// Unhide the warning message
>+function ShowPrefs()

Hide or unhide? ;)

r=mano assuming beltzner approved the strings (If not, please request ui-review as well).

Comment 37

[Mike Beltzner [:beltzner, not reading bugmail]]

I'm with Michael - the warning shouldn't be scaremongering, but rather making sure that someone understands what's going to happen. Short and sweet works best, IMO:

+<!ENTITY scaryWarningTitle.label "Be careful, this gun is loaded!">
+<!ENTITY scaryWarningText.label "The about:config page allows you to modify advanced preferences that control this application. It is possible to create harmful side effects by changing the values of these preferences. You should only continue if you know what you are doing or if you are following trusted advice.">
+<!ENTITY scaryWarningButton.label "I'll be careful, I promise!">
+<!ENTITY scaryWarningCheckbox.label "Show this warning next time">

Comment 38

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #37)
> +<!ENTITY scaryWarningTitle.label "Be careful, this gun is loaded!">

BTW, I'm actually serious about this. I think it grabs people's attention, and keeps a sense of fun.

> +<!ENTITY scaryWarningText.label "The about:config page allows you to modify

Ooops, that should be "This configuration page" since not all apps will have it as about:config, I guess ...

Comment 39

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Fixed in Firefox trunk (for FF3), with beltzner's strings from comment 37/38.

Comment 40

[bug chaser]

This isn't a bug and should not have been fixed.  There is no reason to bug the user about something he already wants to do.  The example of Windows nagging the user when navigating to C:\Windows should demonstrate how this harms usability, and should not be emulated.

Comment 41

[bigcactus]

I can't believe someone wasted time on this completely nonessential obstruction when there are still CSS compliance issues. In any case, they blew it because they used some kind of default text color on a white background, thus making it unreadable for some users (like me).

Comment 42

[Jesse Ruderman]

bigcactus, please file a new bug report for the text-color / background-color issue with the warning page and mark it as blocking this bug.

Comment 43

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #41)
> I can't believe someone wasted time on this completely nonessential obstruction
> when there are still CSS compliance issues.

I wrote the patch specifically to avoid making the product better, while simultaneously adding annoyances.  Glad it worked!  I actually have a patch that fixes all the CSS2 compliance issues, but chose not to submit it just to keep you unhappy.

Comment 44

[bug chaser]

(In reply to comment #43)
> (In reply to comment #41)
> > I can't believe someone wasted time on this completely nonessential obstruction
> > when there are still CSS compliance issues.
> I wrote the patch specifically to avoid making the product better, while
> simultaneously adding annoyances.  Glad it worked!  I actually have a patch
> that fixes all the CSS2 compliance issues, but chose not to submit it just to
> keep you unhappy.

Maybe you could write a patch for warning the user that there are CSS2 compliance issues and making them press a button acknowledging the warning, before they can view the page!