299580 - Cannot enter 8 or 9 into the history "days" box

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Dan Fischbach]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050703 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050703 Firefox/1.0+

I cannot seem to enter 8 or 9 into the "Days to keep history" box without
highlighting the entire number in the box and pushing Backspace.  Pushing
backspace all the way automatically enters a 0 (zero) in the box.  Entering
other numbers (1-7) when the zero is present yields the expected result
(replacing the zero with the number), however, when evering an 8 or 9, the field
acts as if the number is going to be entered by displaying it for a split second
(such as "08" or "09"), but then erases it, as if the Backspace key had been pushed.

**Note: this only happens when the caret is to the right of the 0 and entering a
number.  Numbers such as "80" and "90" appear fine.**

Reproducible: Always

Steps to Reproduce:
1. Open up DPa1
2. Go to Tools -> Options... -> Privacy -> History
3. Try and enter "08" or "09" in the box
4. See the wacky bug
Actual Results:  
Went to Bugzilla and reported this bug

Expected Results:  
"8" or "9" should have been entered into the box

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050703
Firefox/1.0+ ID:2005070312
Zip build
Default theme
Default extensions
Default profile

Comment 1

[Anders Conbere]

WFM

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050624 Firefox/1.0+

Comment 2

[Anders Conbere]

and... another with a new build

WFM
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050703 Firefox/1.0+

Comment 3

[Aaron Slunt]

Confirmed Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050630 Firefox/1.0+ ID:2005063006

entering 08 and 09 into the box reverts the number to 0.

Comment 4

[Dan Fischbach]

I can reproduce this on: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.8b2) Gecko/20050703 Firefox/1.0+ ID:2005070317
Build from:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/pacifica-trunk/firefox-1.0+.en-US.win32.zip
Default theme/extensions
Clean profile

Comment 5

[Anders Conbere]

possible windows builds only bug?

Comment 6

[Pardal Freudenthal (:ShareBird)]

I can confirm this: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050703 Firefox/1.0+ ID:2005070306

Comment 7

[Dan Fischbach]

Anyone know any Mac OS testers to see if this is a true Windows-only bug?

Comment 8

[Caleb]

I think this probably belongs to Firefox/Preferences.

Comment 9

[Mathieu Parent]

I think this is because 
parseInt("08") = 0
(08 looks like an octal number, incorrect, so the value is 0)

should use parseInt("08",10) to enforce parsing as decimal.

References:
http://developer-test.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Functions:parseInt#Description

* If the input string begins with "0", the radix is eight (octal). This feature
is deprecated.

Comment 10

[Mathieu Parent]

located at
http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/preferences.xml#370
(becomes firefox/chrome/toolkit.jar/content/global/bindings/preferences.xml
after compilation)

-             return value != "" ? parseInt(value) : 0;
+             return value != "" ? parseInt(value, 10) : 0;

(i can't make a patch, because i don't have the sources, :-) )

Ben review ?

Comment 11

[Mathieu Parent]

-> cc ben goodger
Can you please review the "inline" patch ?
thanks

Comment 12

[Caleb]

(In reply to comment #9)
> I think this is because 
> parseInt("08") = 0
> (08 looks like an octal number, incorrect, so the value is 0)
> 
> should use parseInt("08",10) to enforce parsing as decimal.
> 
> * If the input string begins with "0", the radix is eight (octal). This feature
> is deprecated.

Nice catch Mathieu!

The line that is supposed to be modified is this:
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/preferences.xml#370

The problem is, that would kinda affect anyone using the preferences (so people
who MAY want to store hex values will get their values converted to decimals or
NaN).

Comment 13

[Caleb]

Attachment: patch v1.0

This will check if the value string starts with "0x" (a hexadecimal) and
attempt to return a hexadecimal parsed int, otherwise we think it's a decimal,
so we force the conversion.

Comment 14

[Asaf Romano (gone)]

Comment on attachment 188186 [details] [diff] [review]
patch v1.0

The third |return| statement will never happen...

Comment 15

[Caleb]

(In reply to comment #14)
> (From update of attachment 188186 [details] [diff] [review] [edit])
> The third |return| statement will never happen...
> 

Argh, that one accidently sneaked in.. should have been removed.

Updated patch coming!

Comment 16

[Caleb]

Attachment: patch v1.1

Without the extra line...

Comment 17

[Asaf Romano (gone)]

Comment on attachment 188188 [details] [diff] [review]
patch v1.1

this still has |else| after |return|.

Comment 18

[Caleb]

Attachment: patch v1.2

If this doesn't do it, I'm going to roll over and die :P

Comment 19

[Aaron Slunt]

+            return 0;

Shouldn't that be break; instead of return 0?

Comment 20

[Mathieu Parent]

I think my 'inline' patch do the thing:

-             return value != "" ? parseInt(value) : 0;
+             return value != "" ? parseInt(value, 10) : 0;

because 0x... numbers are converted to 0 when typing x.
Example (browser.preferences.instantApply= true):
if you type 0xA => 0
because it will do step by step:
0 => 0
0x => 0
0xA = 0A => 0

So the current version doesn't work with hexa values (only if you paste the
complete value)

Comment 21

[Mike Connor [:mconnor]]

Comment on attachment 188191 [details] [diff] [review]
patch v1.2

>           switch (this.type) {
>           case "int":
>-            return value != "" ? parseInt(value) : 0;
>+            if (value != "") {
>+              // work around the fact that parseInt (in JS 1.5) converts 
>+              // ints starting with "08" to octal
>+              return value.substring(1, 2) == "0x" ? parseInt(value, 16) : parseInt(value, 10);
>+            } 

why do we care about hex values? and why is hex ok but real octal isn't?  don't
overthink this stuff, just add the second arg to force it to base ten.

Comment 22

[Dan Fischbach]

This does not seem to happen when browser.preferences.instantApply is set to
true.  When entering 8 or 9 in the box then, the 0 becomes 08 or 09 (NOT 8 or 9
as it should be).

Testing using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050704 Firefox/1.0+ ID:2005070405

Comment 23

[Dan Fischbach]

Why don't you guys look at the code for entering the amount of disk space to use
for cache?  This bug doesn't happen there.

Comment 24

[Caleb]

Attachment: patch vI.III

No more funny shtuff, as per request.

Comment 25

[Caleb]

Comment on attachment 188280 [details] [diff] [review]
patch vI.III

Requesting approval for this very small patch.

Comment 26

[Mike Shaver (:shaver emeritus)]

Comment on attachment 188280 [details] [diff] [review]
patch vI.III

a=shaver.

Comment 27

[Asaf Romano (gone)]

Checking in preferences.xml;
/cvsroot/mozilla/toolkit/content/widgets/preferences.xml,v  <--  preferences.xml
new revision: 1.23; previous revision: 1.22
done

Comment 28

[Jo Hermans]

*** Bug 300420 has been marked as a duplicate of this bug. ***