383009 - Integer pref dialog silently accepts non-numbers

Status: RESOLVED FIXED

(Toolkit :: Preferences, defect)

Description

[Mats Palmgren (inactive)]

Integer pref dialog silently accepts non-numbers

STEPS TO REPRODUCE
1. Start Firefox/Suiterunner
2. about:config
3. double-click an integer pref
4. delete the current value then type 333hello in the text input box
5. ENTER or click OK

ACTUAL RESULT
value changes to 333

EXPECTED RESULT
Only base-10 numbers should be accepted as input.
This would help avoiding mistyping values as in bug 53098.

PLATFORMS AND BUILDS TESTED
Bug occurs in trunk.
Bug does not occur in xpfe SeaMonkey (displays a warning alert and does
not change the value, so it's a regression for SeaMonkey)

Comment 1

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Disable the OK button until there is an acceptable number.

Comment 2

[[e-mail]]

Comment on attachment 267035 [details] [diff] [review]
Patch rev. 1

>+<dialog id="intValueDialog"
>+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" 
>+	ondialogaccept="return onIntValueDialogOK();"
>+	ondialogcancel="return onIntValueDialogCancel();"
>+        onload="onIntValueDialogLoad();">
Some tabs crept in here. You also forgot to specify the dimensions.

>+    var gIntValueDialogParams;
Are you planning on making this dialog accessible to C++ callers? If not, you should just pass a JS object instead of a dialog param block.

>+      textbox.value = gIntValueDialogParams.GetInt(2);
>+      validateIntValue();
I would be very surprised if GetInt(2) returned an invalid int value...

>+    function onIntValueDialogOK()
>+    {
>+      if (!validateIntValue())
>+        return false;
I would be almost as surprised if you could click OK when it was disabled. (The Enter key also checks that the default button is not disabled.)

>+    function validateIntValue()
>+    {
>+      // We intentionally don't want to support octal/hex numbers here (bug 63117).
>+      var value = document.getElementById("textbox").value;
>+      var intValue = parseInt(value, 10);
>+      var valid = !isNaN(value) && value == intValue.toString();
Hmm... I guess (value | 0) == (value - 0) access hex numbers. But at least it only accepts integers, whereas this code accepts numbers up to 100000000000000000 (which appears to be the limit of double precision). Maybe (value | 0) == parseFloat(value) would be a suitable compromise?

>+      <textbox id="textbox" flex="1" oninput="validateIntValue()"/>
flex="1" is wrong here.

>+  else if (entry.typeCol == nsIPrefBranch.PREF_INT) {
>+    if (!openIntValueDialog(entry, title))
>+      return false;
>+  }
I don't like the way this conceals the pref setting code.

>-      var supportsString = Components.classes[nsSupportsString_CONTRACTID].createInstance(nsISupportsString);
>-      supportsString.data = result.value;
>-      gPrefBranch.setComplexValue(entry.prefCol, nsISupportsString, supportsString);
>-    }
>+    var supportsString = Components.classes[nsSupportsString_CONTRACTID].createInstance(nsISupportsString);
>+    supportsString.data = result.value;
>+    gPrefBranch.setComplexValue(entry.prefCol, nsISupportsString, supportsString);
I assume this is just a whitespace change?

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Comment 4

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (diff -w)

(In reply to comment #2)
> Some tabs crept in here. You also forgot to specify the dimensions.

Fixed.

> >+    var gIntValueDialogParams;
> Are you planning on making this dialog accessible to C++ callers? If not, you
> should just pass a JS object instead of a dialog param block.

Fixed.

> >+    function onIntValueDialogOK()
> >+    {
> >+      if (!validateIntValue())
> >+        return false;
> I would be almost as surprised if you could click OK when it was disabled. (The
> Enter key also checks that the default button is not disabled.)

I'd like to keep it, because we have bugs that would make you surprised ;-)
I have added the following comment:

      // XXX Validate one more time because there are ways to change
      // the text that does not trigger 'oninput' such as drag-n-drop
      // (bug 128066) and autocomplete (bug 320462).

> >+      var value = document.getElementById("textbox").value;
> >+      var intValue = parseInt(value, 10);
> >+      var valid = !isNaN(value) && value == intValue.toString();
> Hmm... I guess (value | 0) == (value - 0) access hex numbers. But at least it
> only accepts integers, whereas this code accepts numbers up to
> 100000000000000000 (which appears to be the limit of double precision). Maybe
> (value | 0) == parseFloat(value) would be a suitable compromise?

I kept the current code and added "(value | 0) == intValue" to limit
the number to the integer range without overflow, [-2147483648 .. 2147483647]

> >+      <textbox id="textbox" flex="1" oninput="validateIntValue()"/>
> flex="1" is wrong here.

Removed.

> I don't like the way this conceals the pref setting code.

Fixed.

> I assume this is just a whitespace change?

Well, yes for the non-integer branch, but for integers the old
code actually sets the value twice - first with setIntPref()
and then again with setComplexValue(), I guessed this
was just a bug and that setIntPref() is enough.

Comment 5

[Mats Palmgren (inactive)]

(In reply to comment #4)
> > I assume this is just a whitespace change?

Please disregard my last answer to this question, I misread the code.
Yes, this is just a whitespace change.

Comment 6

[[e-mail]]

Comment on attachment 267100 [details] [diff] [review]
Patch rev. 2 (diff -w)

>+<dialog id="intValueDialog"
>+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" 
>+        ondialogaccept="return onIntValueDialogOK();"
>+        ondialogcancel="return onIntValueDialogCancel();"
>+        onload="onIntValueDialogLoad();"
>+        style="min-width: 29em; min-height: 10em;">
I just noticed you're missing the buttonpack... can you add that too please?

>+      textbox.value = gIntValueDialogParams.value;
>+      validateIntValue();
I worked out why you need to validate this - it's a string, not an int, so the old code was wrong to pass it as such ;-)

>+                   cancelled: false };
I think this should default to true, just in case. r=me with these fixed.

Comment 7

[Mats Palmgren (inactive)]

Attachment: nits fixed

Comment 8

[Mats Palmgren (inactive)]

Checked in to trunk at 2007-06-08 02:52 PDT.

-> FIXED