show warning page before showing about:config

RESOLVED FIXED in mozilla1.8.1

Status

()

Core
General
--
enhancement
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: beltzner, Assigned: Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com])

Tracking

({fixed-seamonkey1.1b, relnote})

Trunk
mozilla1.8.1
fixed-seamonkey1.1b, relnote
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-seamonkey-trunk, fixed-firefox-trunk, URL)

Attachments

(2 attachments, 10 obsolete attachments)

19.69 KB, patch
Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
: review+
jag (Peter Annema)
: superreview+
Details | Diff | Splinter Review
10.29 KB, patch
mano
: review+
Details | Diff | Splinter Review
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.
Summary: [rfe] show warning page before showing about:config (first time only) → show warning page before showing about:config (first time only)
Created attachment 224289 [details] [diff] [review]
proof of concept

Not yet localized, pretty ugly.  Comments?
Assignee: nobody → cst
Status: NEW → ASSIGNED
Depends on: 339728
Created attachment 224290 [details] [diff] [review]
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.
Attachment #224289 - Attachment is obsolete: true
Attachment #224290 - Flags: ui-review?(beltzner)
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. :)
Attachment #224290 - Flags: ui-review?(beltzner) → ui-review+
Attachment #224290 - Flags: superreview?(neil)

Updated

11 years ago
Attachment #224290 - Flags: superreview?(neil) → superreview-

Comment 4

11 years ago
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");
(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.
Created attachment 232854 [details] [diff] [review]
even less ugly, address review issues

Neil, I'm looking for review on everything but the text strings.
Attachment #224290 - Attachment is obsolete: true
Attachment #232854 - Flags: superreview?(neil)
Attachment #232854 - Flags: review?(neil)
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

11 years ago
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

11 years ago
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).
Whiteboard: [cst: waiting for review to finish]

Comment 10

11 years ago
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.
Attachment #232854 - Flags: superreview?(neil)
Attachment #232854 - Flags: review?(neil)
Attachment #232854 - Flags: review-
Created attachment 236638 [details] [diff] [review]
patch
Attachment #232854 - Attachment is obsolete: true
Attachment #236638 - Flags: review?(neil)
Created attachment 236639 [details] [diff] [review]
diff -w
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
Summary: show warning page before showing about:config (first time only) → show warning page before showing about:config

Comment 14

11 years ago
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?]
Attachment #236638 - Flags: review?(neil) → review-
Created attachment 236727 [details] [diff] [review]
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.
Attachment #236638 - Attachment is obsolete: true
Attachment #236639 - Attachment is obsolete: true
Attachment #236727 - Flags: superreview?(neil)
Attachment #236727 - Flags: review?(neil)
Created attachment 236728 [details] [diff] [review]
diff -w

Comment 17

11 years ago
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
Attachment #236727 - Flags: superreview?(neil)
Attachment #236727 - Flags: superreview?(jag)
Attachment #236727 - Flags: review?(neil)
Attachment #236727 - Flags: review?(kairo)
Attachment #236727 - Flags: review+

Comment 18

11 years ago
(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.
Whiteboard: [cst: waiting for review to finish] → [cst: waiting for review to finish, reviews had nits]

Comment 19

11 years ago
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.
Attachment #236727 - Flags: review?(kairo) → review+
Created attachment 237503 [details] [diff] [review]
patch

Addresses review comments.

I'm not sure if neil preferred having jag sr or was just asking the question.
Attachment #236727 - Attachment is obsolete: true
Attachment #236728 - Attachment is obsolete: true
Attachment #237503 - Flags: superreview?(jag)
Attachment #237503 - Flags: review?(neil)
Attachment #236727 - Flags: superreview?(jag)
Whiteboard: [cst: waiting for review to finish, reviews had nits] → [cst: r? sr?]

Comment 21

11 years ago
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.
Attachment #237503 - Flags: review?(neil) → review+

Comment 22

11 years ago
Comment on attachment 237503 [details] [diff] [review]
patch

New patch with nits addressed/fixed, please.
Whiteboard: [cst: r? sr?] → [cst: sr?]
Created attachment 237727 [details] [diff] [review]
fix nits

This fixes Neil's nits, as well as a JST Review Simulacrum nit about putting "{" on its own line for methods.
Attachment #237503 - Attachment is obsolete: true
Attachment #237727 - Flags: superreview?(jag)
Attachment #237503 - Flags: superreview?(jag)

Updated

11 years ago
Attachment #237727 - Flags: superreview?(jag) → superreview+
Moving to core since I'm fixing both products
Component: General → XP Miscellany
Flags: ui-review+
Flags: review-
Flags: review+
Product: Firefox → Core
Whiteboard: [cst: sr?] → fixed-seamonkey-trunk
Target Milestone: --- → mozilla1.8.1
Version: 2.0 Branch → Trunk
Attachment #237727 - Flags: review+
Created attachment 237872 [details] [diff] [review]
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.
Attachment #237872 - Flags: review?
Attachment #237872 - Flags: review? → review?(mconnor)

Comment 26

11 years ago
This busted about:config in Camino; see bug 352425.
Depends on: 352425
Comment on attachment 237727 [details] [diff] [review]
fix nits

Requesting approval from drivers because I touch all.js
Attachment #237727 - Flags: approval1.8.1?
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 on attachment 237727 [details] [diff] [review]
fix nits

Cancelling approval request.  There's a way to do this entirely within XPFE for branch.
Attachment #237727 - Flags: approval1.8.1?
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.
Keywords: fixed-seamonkey1.1b
Blocks: 357014
No longer blocks: 357014
Depends on: 357014

Comment 31

11 years ago
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.
Keywords: relnote
Comment on attachment 237872 [details] [diff] [review]
firefox patch

Obsoleting; going to try a different reviewer [with updated patch].
Attachment #237872 - Attachment is obsolete: true
Attachment #237872 - Flags: review?(mconnor)
Created attachment 243145 [details] [diff] [review]
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).
Attachment #243145 - Flags: review?(mano)
Comment on attachment 243145 [details] [diff] [review]
updated firefox patch

for reason mentioned on irc .
Attachment #243145 - Flags: review?(mano) → review-
Created attachment 243147 [details] [diff] [review]
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.
Attachment #243145 - Attachment is obsolete: true
Attachment #243147 - Flags: review?(mano)
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).
Attachment #243147 - Flags: review?(mano) → review+
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">
(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 ...
Fixed in Firefox trunk (for FF3), with beltzner's strings from comment 37/38.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: fixed-seamonkey-trunk → fixed-seamonkey-trunk, fixed-firefox-trunk

Comment 40

11 years ago
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

10 years ago
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

10 years ago
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.
(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

10 years ago
(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!

Updated

10 years ago
Depends on: 369103

Updated

9 years ago
Component: XP Miscellany → General
QA Contact: general → general

Updated

9 years ago
Depends on: 395898
You need to log in before you can comment on or make changes to this bug.