209569 - view selection source shows style attribute with 0,5 instead of 0.5

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

On the above page, at least with LANG=de_AT.ISO-8859-1, View Selection Source
shows a -moz-opacity of 0,5 (for the second image)

bz suggested a simple patch for this bug, but I couldn't test it yet. I'll
attach it.

Comment 1

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

Attachment: bz's patch

Comment 2

[Benjamin Smedberg]

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

Comment 3

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

"locale" output:
$ locale
LANG=de_AT.ISO-8859-1
LC_CTYPE="de_AT.ISO-8859-1"
LC_NUMERIC="de_AT.ISO-8859-1"
LC_TIME="de_AT.ISO-8859-1"
LC_COLLATE="de_AT.ISO-8859-1"
LC_MONETARY="de_AT.ISO-8859-1"
LC_MESSAGES="de_AT.ISO-8859-1"
LC_PAPER="de_AT.ISO-8859-1"
LC_NAME="de_AT.ISO-8859-1"
LC_ADDRESS="de_AT.ISO-8859-1"
LC_TELEPHONE="de_AT.ISO-8859-1"
LC_MEASUREMENT="de_AT.ISO-8859-1"
LC_IDENTIFICATION="de_AT.ISO-8859-1"
LC_ALL=

Comment 4

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

I think I'm confident this is the right thing for all callers with the possible
exception of mozSqlResult::AppendValue and EnsureBuffer (varga?).  This will
probably fix a bunch of existing bugs (e.g., with nsPrintOptionsImpl, all the
CSS stuff).

Comment 5

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

(But you should probably add a comment like the one in |ToFloat| about locale
sensitivity.)

Comment 6

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

Attachment: comment added

Comment 7

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

See also bug 102690, for the same bug in nsTextFormatter (already fixed).

PR_snprintf is locale-sensitive for floats.  We need to use PR_dtoa or PR_cnvtf.

Comment 8

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

I guess there's also the question of whether it is a bug or a feature that
PR_snprintf, etc., are locale-sensitive for floats while being
locale-insensitive for everything else.  Should this be fixed in cvt_f (like bug
102690), or should this be fixed by making ns[C]String::AppendFloat not use
PR_snprintf?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch, tested and all.

Comment 10

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

For the code:

  nsCAutoString str;
  str.AppendFloat(1.0/3.0);
  str.Append(" ");
  str.AppendFloat(2.0/3.0);
  printf("%s\n", str.get());

this changes the output from:

0.333333 0.666667

to

0.3333333333333333 0.6666666666666666

Comment 11

[Boris Zbarsky [:bzbarsky]]

True.  We could change the precision arg to 6.

Comment 12

[Wan-Teh Chang]

Re: comment 8: it's a bug that we can't fix because
there is code depending on the bug.

Comment 13

[Jan Varga [:janv]]

I'm ok with the change, the sql module needs far more work to play nice with
currect locale, anyway.

Comment 14

[Jose Fandos]

Does this bug cover the fact that View Selection Source doesn't show anything?
Should I open a new bug? This is with Mozilla the Suite 2003061908

Comment 15

[Boris Zbarsky [:bzbarsky]]

no, no, and you need to update -- that's already fixed.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Set precision to 6

Comment 17

[Jose Fandos]

Does this cover the fact that on selecting some text and "view selection source"
I see:

<h3>“A Rosetta Stone of web design”</h3>

While if I look at the "view page source" I see:

<h3>&#8220;A Rosetta Stone of web design&#8221;</h3>

Which is what the page actually has written on it?
And if this is not the case, do I fill in a new bug or is there an existing one?

Comment 18

[Boris Zbarsky [:bzbarsky]]

If you actually look at the patch, you will see that this bug covers one thing
and one thing only -- representation of floating-point numbers in serialized
style data.  Please stop making irrelevant comments.  And learn to use the
bugzilla search.

Comment 19

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

When I tried that in my tree, it didn't actually do what I expected.  Did you
actually see this print only 6 digits or so?

Comment 20

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

Append(JS_dtostr(buf, sizeof(buf), DTOSTR_PRECISION, 6, aFloat)) works, except
it required string depending on internal headers within JS (plus use of
LD_PRELOAD) to learn that.

Comment 21

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

The following change also makes PR_cvtf do what we want (and work as documented):

Index: prdtoa.c
===================================================================
RCS file: /cvsroot/mozilla/nsprpub/pr/src/misc/prdtoa.c,v
retrieving revision 3.7.4.3
diff -p -u -3 -d -r3.7.4.3 prdtoa.c
--- prdtoa.c    7 Jan 2003 15:30:15 -0000       3.7.4.3
+++ prdtoa.c    2 Jul 2003 00:19:24 -0000
@@ -2552,8 +2552,7 @@ PR_cnvtf(char *buf,int bufsz, int prcsn,
                buf[0] = '\0';
                return;
        }
-       /* XXX Why use mode 1? */
-       if (PR_dtoa(dval(fval),1,prcsn,&decpt,&sign,&endnum,num,bufsz)
+       if (PR_dtoa(dval(fval),2,prcsn,&decpt,&sign,&endnum,num,bufsz)
                        == PR_FAILURE) {
                buf[0] = '\0';
                goto done;

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 126800 [details] [diff] [review]
Set precision to 6

This does not work on its own.

Comment 23

[Boris Zbarsky [:bzbarsky]]

wtc, any opinions on the change David mentions in comment 21?

Comment 24

[Wan-Teh Chang]

I am not familiar with prdtoa.c, so I can't comment
on that change.  If that change will change the
semantics of PR_cvtf, I may not be able to take it.
Unfortunately NSPR needs to be "bug compatible" in
some cases in order to provide the backward
compatibility its customers need.

Comment 25

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

Right now PR_cvtf ignores its prcsn argument.  This makes its behavior depend on
the argument.

Comment 26

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

Attachment: patch

Copy the necessary code to nsStrPrivate.  I made two changes to PR_cvtf when I
copied it:
 * changed the mode from 1 to 2
 * changed the test for -0.0 or -NaN to just use fval < 0.0 (and ignore NaN,
since it's not portable anyway), and removed the use of the fval / word0 /
word1 macros, which I didn't want to have to pull in.

Comment 27

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

Attachment: patch

same thing, but with indentation and the spacing around if/while expressions
matching the surrounding code, and no tabs.

Comment 28

[jag (Peter Annema)]

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

r=jag

Comment 29

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

On the test code:

    nsCAutoString str;
    str.AppendFloat(1.0/3.0);
    str.Append(" ");
    str.AppendFloat(2.0/3.0);
    str.Append(" ");
    str.AppendFloat(1.0/7.0);
    str.Append(" ");
    str.AppendFloat(-0.0);
    str.Append(" ");
    str.AppendFloat(-1073741824.0);
    str.Append(" ");
    str.AppendFloat(1073741824.0);
    str.Append(" ");
    str.AppendFloat(1024.0);
    str.Append(" ");
    str.AppendFloat(-1024.0);
    printf("%s\n", str.get());

this changes the behavior from:
0.333333 0.666667 0.142857 -0 -1.07374e+09 1.07374e+09 1024 -1024
to:
0.333333 0.666667 0.142857 0 -1.07374e+9 1.07374e+9 1024 -1024

So it fixes a bug that the negative 0.0 was being printed with a minus sign (or
should we not consider that a bug?), and it changes exponents from being printed
with an extra zero when not needed.  I think that's an acceptable change,
although we could tweak it later...

Comment 30

[Boris Zbarsky [:bzbarsky]]

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

Looks good, and changes to output are perfectly ok.

Comment 31

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

Fix checked in to trunk, 2003-07-08 23:53 -0700.

Comment 32

[Wan-Teh Chang]

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

I'm sorry that I didn't have time to look
into this bug until this morning.

1. I'd like to see an explanation of what the
PR_cnvtf bug is in the following comments.  (I believe
the bug is that PR_cvtf ignores its prcsn argument.)

>+/**
>+ * This is a copy of |PR_cnvtf| with a bug fixed.  (The second argument
>+ * of PR_dtoa is 2 rather than 1.)

>+  // A copy of PR_cnvtf with a bug fixed.

2. Some history on prdtoa.c.  This file is based on
David M. Gay's dtoa.c.	PR_cnvtf is our own addition,
a convenience function that calls PR_dtoa.

Originally prdtoa.c was used by JavaScript (and I
believe only by JavaScript).  This is why in the
comments in prdtoa.c before PR_cnvtf there were
references to the ECMA JavaScript spec. PR_cnvtf
does exactly what JavaScript needed to pass ECMA
JavaScript conformance test at that time.

Then the JavaScript team made JavaScript a
self-contained library by duplicating the NSPR
files they need.  So jsdtoa.c is their copy of
prdtoa.c.  Since then jsdtoa.c has been enhanced
to meet new JavaScript requirements.  I just took
a look at jsdtoa.c this morning and found that it
seems to have generalized PR_cnvtf to JS_dtostr,
which takes a mode argument.

If you have access to JS_dtostr, that'll be an ideal
solution.  But seems like you can't call JS_dtostr.
So we could consider adding a new PR_dtostr function
with a mode argument and reimplement PR_cnvtf to call
PR_dtostr with a mode of 1.

3. Since PR_cnvtf is merely a convenience function
that we added, it is fine to call PR_dtoa directly
if PR_cnvtf doesn't meet your needs.  Alternatively,
we could add a more flexible convenience function,
as I mentioned above.

NSPR's strict backward compatibility requirement
prevents me from changing the behavior of PR_cnvtf
unless I am sure that no code can possibly depend
on the current behavior.  Unfortunately I don't
have the expertise to understand PR_dtoa and PR_cnvtf.
So I have to be conservative and assume that there
is code that depends on the current behavior of
PR_cnvtf.  We can eliminate the duplicate code by
adding a more flexible convenience function to prdtoa.c,
but I don't have the expertise to design that function.
So I'll need some help from the JavaScript maintainers
who are familiar with jsdtoa.c.  I am not sure if it
is worth the time to do that.

Comment 33

[Hixie (not reading bugmail)]

> it changes exponents from being printed with an extra zero when not needed.

Drive-by comment (may not be relevant): Ideally we don't want exponents at all 
in any CSS output.