226294 - format-number() ignores the sign of its numerical argument

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Ashley Winters]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5a) Gecko/20031113 Mozilla Firebird/0.6.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5a) Gecko/20031113 Mozilla Firebird/0.6.1

The format-number() function is only displaying the abs() value of its numerical
argument. It never displays the minus sign before the number, and will never use
the "negative" format after the ; in the format string.

Reproducible: Always

Steps to Reproduce:
1. Copy the XSLT document on the MSDN site into a file named decimalformat.xsl
2. Open the file in Mozilla
Actual Results:  
I see a version of the table matching the example provided by Microsoft, except
that the sign of the number is not preserved. The negative numbers are shown as
being positive.


Expected Results:  
The table should match the example provided by Microsoft, with the sign of the
number being preserved, and the negative format being used on negative numbers.

Comment 1

[Duke]

Confirmed.  I see the same problem.

Ashley Winters and I tried a number of tricks to get around this.  We tried
specifying the following to no avail:

<xml:decimal-format minus-sign="-" />
format-number(-123.45, '0.00;-0.00')
format-number(-123.45, '0.00;(0.00)')

Note that the last one wouldn't actually yeild a minus sign, but it should put
parentheses around negative numbers.  It had no effect, however.

I finally used a conditional and <xml:text>-$</xml:text> to get the minus sign.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5a) Gecko/20030912 Mozilla
Firebird/0.6.1

Comment 2

[Axel Hecht]

Attachment: decimalformat.xsl, self referencing

I cannot reproduce this on windows 1.4 and trunk.
Somebody should try a linux build, though I don't expect a platform issue
within intel cpus here.

Comment 3

[Ashley Winters]

I tried the decimalformat.xsl example on my

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5b) Gecko/20030820 Mozilla
Firebird/0.6.1+

and it works as expected. I can only replicate the problem on a Linux build.

Comment 4

[Axel Hecht]

We copied the Double::isNeg code from jsnum.h, which since then got a check-in
to handle some gcc 3 alias optim issue, see bug 83388, and diff
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/js/src&command=DIFF_FRAMESET&file=jsnum.h&rev2=3.7&rev1=3.6

We should port the current state of jsnum.h to Double.cpp and be hopefully done
with it.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: selfreferencing that works in bugzilla too

Comment 6

[Sean McMurray]

In Mozilla 1.4 (Linux) a format-number xpath always resulted in a negative number.
I noted the problem was "fixed" in Mozilla 1.5.

However, I now see that the problem remains in {} expressions. For instance:

<form>
    <input name="total" value="{format-number(total, '###0.0')}"/>
</form>

I've tacked this report onto this issue because it sounds related.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch v1 [Checked in: Comment 16]

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 138210 [details] [diff] [review]
patch v1
[Checked in: Comment 16]

This patch has been tested on gcc 3.2.2 and is verified to fix the problem.
It'd be great if i could get reviews by monday so that we could get this into
1.6. Sorry for the late patch :(

Comment 9

[Peter Van der Beken [:peterv]]

As long as we're syncing with jsnum, could you please use the latest version of
the code? Seems like that also has ARM and Os/2 changes, though I'm not sure if
we need the SET_FPU/RESTORE_FPU changes.

Comment 10

[Axel Hecht]

Comment on attachment 138210 [details] [diff] [review]
patch v1
[Checked in: Comment 16]

I don't have anything even loosely resembling a tree for yet another week.
Reading what peter said, I should have that to do the review.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 138210 [details] [diff] [review]
patch v1
[Checked in: Comment 16]

could we please try to get at least this in for 1.6? There might be other
things that should be ported too, but this fixes a known bug. I most likly
won't have time to do the rest before 1.6 is released since that will happen
any day now and i'm out of town. I'll look into porting the other things as
soon as i get back though.

Comment 12

[Brian Ryner (not reading)]

Comment on attachment 138210 [details] [diff] [review]
patch v1
[Checked in: Comment 16]

r/sr=bryner

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 138210 [details] [diff] [review]
patch v1
[Checked in: Comment 16]

sr=bzbarsky

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 138210 [details] [diff] [review]
patch v1
[Checked in: Comment 16]

This patch is pretty safe since the same code has been well tested in the
js-library on the trunk for a while now. Also it only affects XSLT/XPath.

Comment 15

[Mike Kaply [:mkaply]]

Comment on attachment 138210 [details] [diff] [review]
patch v1
[Checked in: Comment 16]

a=mkaply for 1.6

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

checked in to the 1.6 branch. I'm keeping this open to investigate syncronizing
other changes to jsnum.h

Comment 17

[Peter Van der Beken [:peterv]]

Nice, no XSLT peers reviewed.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I didn't think that was needed since this code isn't xslt specific. It was also
an xslt peer that created the patch. I had to choose between leaving the tree in
a known broken state for 1.6 or doing it this way, i thought the latter was better.

Comment 19

[chris hofmann]

lets mark fixed and reopen another bug for addtional work.  that is the only way
to get items on the testing, release note, and change tracking for releases...

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The problem isn't fixed on the trunk yet so i'd rather not close this bug as is.
OTOH peterv doesn't seem happy about the landing that happend on the branch so
i'm not really sure what to do?

I'll try to write up a patch that compleatly syncs us to jsnum.h, possibly I can
have something sometime tomorrow.

Comment 21

[Peter Van der Beken [:peterv]]

Well, get a better fix for the trunk. I was unhappy about this patch because it
copies code from jsnum.h that has been modified on the trunk already. Water
under the bridge.

Comment 22

[chris hofmann]

follow up work for trunk in http://bugzilla.mozilla.org/show_bug.cgi?id=230528

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok, closing as fixed

Comment 24

[Peter Weilbacher]

I stumbled over this bug when trying to compile Mozilla 1.6 for OS/2 from CVS
checkout yesterday morning using --with-extensions=all. I got several compile
errors in Double.cpp starting with:

G:/MozCompile/mozilla16/extensions/transformiix/source/base/Double.cpp:78: '
   u_int32_t' is used as a type, but is not defined as a type.
G:/MozCompile/mozilla16/extensions/transformiix/source/base/Double.cpp:79: '
   u_int32_t' is used as a type, but is not defined as a type.

All other errors were just resulting from this one. I fixed this by adding 

#include <sys/types.h>

to Double.cpp, and Mozilla compiled for me. Not sure though, if that header has
the correct definition of that type. If this is not cross platform bracketing it
with #ifdef OS2 could be an option.

Comment 25

[Serge Gautherie (:sgautherie)]

Comment on attachment 138210 [details] [diff] [review]
patch v1
[Checked in: Comment 16]


Re comment 24:

Comment 16 says that this patch was checked in in v1.6branch
but
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/transformiix/so
urce/base/Double.cpp>
says that the Trunk is still at { 1.20	axel%pike.org	Jan 30 2003 }.

Odd !? May be not ?? see Comment 22...

Comment 26

[Peter Van der Beken [:peterv]]

Please stop making a mess. This is the bug that contains the branch work, bug
230528 is for the equivalent work on the trunk. Resetting the target milestone
is the privilege of the bug owner. I'll leave it to sicking to decide how he
wants to handle the build bustage (reopening this or filing a new bug).

Comment 27

[Peter Weilbacher]

Attachment: Double.cpp patch to include sys/types.h on OS/2

Forgot to upload this trivial patch to make in compile on OS/2 yesterday.

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 138863 [details] [diff] [review]
Double.cpp patch to include sys/types.h on OS/2

r=me for landing on the branch, no need to do this on the trunk yet since the
patch isn't landed there

Comment 29

[Peter Weilbacher]

Comment on attachment 138863 [details] [diff] [review]
Double.cpp patch to include sys/types.h on OS/2

As this is my bug, I guess I have to set "superreview?" and "approval?"?

But when granted somebody else has to check it in because I don't have
permission to do so...

Comment 30

[Peter Weilbacher]

Argh, I meant _my_ _patch_. Sorry for the extra bugspam...

Comment 31

[jhp (no longer active)]

Attachment: better OS/2 patch

Using "uint32" instead of "u_int32_t", which should work since it's used for
all platforms on jsnum.h.

Comment 32

[jhp (no longer active)]

Attachment: better better os/2 patch [Checked in: Comment 37]

Scratch that. Let's use NSPR type instead (PRUint32).  Weeeeeee!

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 138906 [details] [diff] [review]
better better os/2 patch
[Checked in: Comment 37]

Thanks! Sorry for being flaky about which datatype to use. Discussed with tor
and we decided that PRUint32 would probably be the safest one to use.

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 138906 [details] [diff] [review]
better better os/2 patch
[Checked in: Comment 37]

got sr=tor

Comment 35

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 138906 [details] [diff] [review]
better better os/2 patch
[Checked in: Comment 37]

requesting approval. This should fix buildbustage on a few platforms where
u_int32_t doesn't exist. The patch should be safe since there are no functional
changes, only bustagefix by using standard NSPR datatypes

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 138906 [details] [diff] [review]
better better os/2 patch
[Checked in: Comment 37]

check in really soon, though.

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 138906 [details] [diff] [review]
better better os/2 patch
[Checked in: Comment 37]

checked in