56022 - object props decrementing percentage value

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[sujay]

using 10/10 build of netscape

1) launch netscape
2) launch composer
3) isnert H. Line
4) click on it and bring up H. Line props
5) change the following:

 Width = 80%
 Height = 3 pixels
 alignment = center

6) click OK

now go back into H. Line props and notice the following:

 Width = 78%  (should be 80)
 Height = 3 pixels (this is fine)
 Alignment = left   (should be center)

its not remembering the changes you made and also its screwing up
the Width by decrementing it by 2 each time you pullup the H. line
props window.

Comment 1

[Simon Fraser [no longer active]]

-> brade, future.

Comment 2

[Kathleen :Brade]

cc buster; he made some recent fixes to layout and hrule's (2px sounds familiar).
Accept bug to keep the bugzilla daemon happy.

Comment 3

[Kathleen :Brade]

I'm not sure if buster fixed this (or someone else); I can't reproduce this 
anymore.

Comment 4

[sujay]

I just tried it again in 10/18 build and it is still happening
for me..I'm using Windows 98.

Comment 5

[sujay]

changing summary.

the overall issue here is that for image, table and H. rule when you change
the %-age value for size, width, height for any of those properties, 
it decrememnts the value by 2 or 1 in some cases. In other words its not
remembering the initial value you set.

I'm filing a new bug for the alignment issue for H. Line...

Comment 6

[Kathleen :Brade]

accepting bug; for some reason this problem doesn't show up on Macintosh but
does show up on NT and Linux...

note:  value is correct when dialog is dismissed, it seems to be getting mangled
on open of the dialog

Comment 7

[kinmoz]

I'm only seeing this in Win32 Netscape_20000922_BRANCH install opt builds. My
debug build doesn't have this problem.

Comment 8

[Gervase Markham [:gerv]]

Relnoting, assuming someone can reproduce this.

Gerv

Comment 9

[Kathleen :Brade]

The problem is that percentages are stored in layout as a floating number.  We 
are seeing rounding errors.  On Mac, I see it if I set the hrule to 53% (it goes 
to 52%) but I don't see it for lots of other numbers.

I see the problem coming up (for me) in nsGenericHTMLElement::ParseValueOrPercent 
where we typecast to a float and divide by 100.0f.

buster suggests we look into using the inlines defined in mozilla/xpcom/ds/
nsUnitConversion.h

Comment 10

[Johnny Stenback (:jst)]

This patch fixes the problem for me, on windows the correct value is stored in
the nsHTMLValue, but when requested, multiplied by 100.0f and casted into an
PRInt32 we get a rounding error. The name of the method I used to do the
rounding doesn't really make sense, but it does work. Is there a better one?

Index: html/content/src/nsGenericHTMLElement.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/content/src/nsGenericHTMLElement.cpp,v
retrieving revision 1.210.4.6
diff -u -r1.210.4.6 nsGenericHTMLElement.cpp
--- nsGenericHTMLElement.cpp    2000/10/17 08:26:17     1.210.4.6
+++ nsGenericHTMLElement.cpp    2000/10/26 01:37:49
@@ -1597,7 +1597,8 @@
     case eHTMLUnit_Percent:
       {
         nsAutoString intStr;
-        intStr.AppendInt(PRInt32(value->GetPercentValue() * 100.0f));
+        float percentVal = value->GetPercentValue() * 100.0f;
+        intStr.AppendInt(NSToCoordRoundExclusive(percentVal));

         aResult.Assign(intStr);
         aResult.Append(PRUnichar('%'));

There could be more than this one place in mozilla that needs a similar "fix", I
didn't look for others yet...

Comment 11

[Kristi Fredrickson]

Adding to Composer section of RTM Release Notes

Comment 12

[Kathleen :Brade]

Attachment: newly proposed patch file (replaces inline suggestion by jst)

Comment 13

[Kathleen :Brade]

ok, I've attached a new patch file to replace jst's patch above.  This is to fix
all places in the file where we typecast to float rather than using the inlines.
 I'm sure there are more places to correct but I felt this was a better start at
this stage of the game.

jst -- can you r=?
buster -- can you sr=?

Thanks!

Comment 14

[buster]

sr=buster

Comment 15

[Johnny Stenback (:jst)]

r=jst

Comment 16

[Kathleen :Brade]

fix checked into mozilla trunk

Comment 17

[sujay]

verified in 2/1 build.

Comment 18

[Kristi Fredrickson]

Changing kristif to robinf in cc: as Robin is now the doc contact for editor 
issues

Comment 19

[Kristi Fredrickson]

Copying Jatin, who's doing the next round of release notes, to ensure that this 
bug is exorcised from the next draft.