Closed
Bug 209569
Opened 21 years ago
Closed 21 years ago
view selection source shows style attribute with 0,5 instead of 0.5
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: Biesinger, Assigned: dbaron)
References
()
Details
Attachments
(1 file, 5 obsolete files)
6.97 KB,
patch
|
jag+mozilla
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•21 years ago
|
||
Comment 2•21 years ago
|
||
*** Bug 189033 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 3•21 years ago
|
||
"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=
Assignee | ||
Comment 4•21 years ago
|
||
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).
Assignee | ||
Comment 5•21 years ago
|
||
(But you should probably add a comment like the one in |ToFloat| about locale sensitivity.)
Reporter | ||
Comment 6•21 years ago
|
||
Attachment #125760 -
Attachment is obsolete: true
Assignee | ||
Comment 7•21 years ago
|
||
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.
Assignee | ||
Comment 8•21 years ago
|
||
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?
Assignee | ||
Updated•21 years ago
|
Attachment #125768 -
Attachment is obsolete: true
Comment 9•21 years ago
|
||
Assignee | ||
Comment 10•21 years ago
|
||
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•21 years ago
|
||
True. We could change the precision arg to 6.
Comment 12•21 years ago
|
||
Re: comment 8: it's a bug that we can't fix because there is code depending on the bug.
Comment 13•21 years ago
|
||
I'm ok with the change, the sql module needs far more work to play nice with currect locale, anyway.
Comment 14•21 years ago
|
||
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•21 years ago
|
||
no, no, and you need to update -- that's already fixed.
Comment 16•21 years ago
|
||
Attachment #125784 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #126800 -
Flags: superreview?(dbaron)
Attachment #126800 -
Flags: review?(dbaron)
Comment 17•21 years ago
|
||
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>“A Rosetta Stone of web design”</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•21 years ago
|
||
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.
Assignee | ||
Comment 19•21 years ago
|
||
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?
Assignee | ||
Comment 20•21 years ago
|
||
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.
Assignee | ||
Comment 21•21 years ago
|
||
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•21 years ago
|
||
Comment on attachment 126800 [details] [diff] [review] Set precision to 6 This does not work on its own.
Attachment #126800 -
Attachment is obsolete: true
Attachment #126800 -
Flags: superreview?(dbaron)
Attachment #126800 -
Flags: review?(dbaron)
Comment 23•21 years ago
|
||
wtc, any opinions on the change David mentions in comment 21?
Comment 24•21 years ago
|
||
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.
Assignee | ||
Comment 25•21 years ago
|
||
Right now PR_cvtf ignores its prcsn argument. This makes its behavior depend on the argument.
Assignee | ||
Comment 26•21 years ago
|
||
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.
Assignee | ||
Comment 27•21 years ago
|
||
same thing, but with indentation and the spacing around if/while expressions matching the surrounding code, and no tabs.
Attachment #127330 -
Attachment is obsolete: true
Comment 28•21 years ago
|
||
Comment on attachment 127331 [details] [diff] [review] patch r=jag
Attachment #127331 -
Flags: review+
Assignee | ||
Comment 29•21 years ago
|
||
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•21 years ago
|
||
Comment on attachment 127331 [details] [diff] [review] patch Looks good, and changes to output are perfectly ok.
Attachment #127331 -
Flags: superreview+
Assignee | ||
Comment 31•21 years ago
|
||
Fix checked in to trunk, 2003-07-08 23:53 -0700.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 32•21 years ago
|
||
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•21 years ago
|
||
> 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•