189533 - background-position has low accuracy

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Hixie (not reading bugmail)]

There appears to be some eager rounding of percentage background-position values.

Testcase:
   http://www.hixie.ch/tests/adhoc/css/background/position/001.html

When you examine it, you'll notice that the image doesn't move smoothly as the
percentage changes, but jumps about.

Comment 1

[Hixie (not reading bugmail)]

you won't get out of it that easily! you own this code now remember? ;-)

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

This is probably due to floating-point arithmetic, and its low accuracy.  I'm
looking at a few potential fixes.

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Actually, the issue is that our style structs are storing background-position as
nscoords unconditionally.  That is a problem since nscoords are PRInt32s.  So
you currently cannot even _have_ positions that are fractions of a percent.  I
have a fix forthcoming.

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Store a float if we have a percentage

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 111900 [details] [diff] [review]
Store a float if we have a percentage

Oops, both of these changes should set mFloat, not mCoord, with a cast to float
instead of nscoord.  Fixed locally.

>Index: content/base/src/nsRuleNode.cpp

>   else if (eCSSUnit_Enumerated == colorData.mBackPositionX.GetUnit()) {
>-    bg->mBackgroundXPosition = (nscoord)colorData.mBackPositionX.GetIntValue();
>+    bg->mBackgroundXPosition.mCoord = (nscoord)colorData.mBackPositionX.GetIntValue();
>     bg->mBackgroundFlags |= NS_STYLE_BG_X_POSITION_PERCENT;
>     bg->mBackgroundFlags &= ~NS_STYLE_BG_X_POSITION_LENGTH;
>   }

>   else if (eCSSUnit_Enumerated == colorData.mBackPositionY.GetUnit()) {
>-    bg->mBackgroundYPosition = (nscoord)colorData.mBackPositionY.GetIntValue();
>+    bg->mBackgroundYPosition.mCoord = (nscoord)colorData.mBackPositionY.GetIntValue();
>     bg->mBackgroundFlags |= NS_STYLE_BG_Y_POSITION_PERCENT;
>     bg->mBackgroundFlags &= ~NS_STYLE_BG_Y_POSITION_LENGTH;
>   }

Comment 6

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

Comment on attachment 111900 [details] [diff] [review]
Store a float if we have a percentage

>Index: content/shared/public/nsStyleStruct.h
> 
>+  union {
>+    nscoord mCoord;
>+    float   mFloat;
>+  } mBackgroundXPosition,         // [reset]
>+    mBackgroundYPosition;         // [reset]
>+

I'm a little concerned about the portability of anonymous unions, but hopefully
you'll be OK.  I vaguely remember some problem in the past, but I don't
remember what.

>Index: content/shared/src/nsStyleStruct.cpp
>+  mBackgroundXPosition.mCoord = mBackgroundYPosition.mCoord = 0;

This isn't really convincing, since nothing guarantees that a float is 32 bits.
 I'd rather leave these uninitialized and add a comment to the header file that
the a member of the union is only valid if the appropriate bit in the flags is
set.

>@@ -1007,8 +1006,8 @@
>       (mBackgroundFlags == aOther.mBackgroundFlags) &&
>       (mBackgroundRepeat == aOther.mBackgroundRepeat) &&
>       (mBackgroundColor == aOther.mBackgroundColor) &&
>-      (mBackgroundXPosition == aOther.mBackgroundXPosition) &&
>-      (mBackgroundYPosition == aOther.mBackgroundYPosition) &&
>+      (mBackgroundXPosition.mCoord == aOther.mBackgroundXPosition.mCoord) &&
>+      (mBackgroundYPosition.mCoord == aOther.mBackgroundYPosition.mCoord) &&
>       (mBackgroundClip == aOther.mBackgroundClip) &&
>       (mBackgroundInlinePolicy == aOther.mBackgroundInlinePolicy) &&
>       (mBackgroundOrigin == aOther.mBackgroundOrigin) &&

Likewise, you might need to do what nsStyleCoord does here too.

>Index: content/base/src/nsRuleNode.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsRuleNode.cpp,v
>retrieving revision 1.54
>diff -u -r1.54 nsRuleNode.cpp
>--- content/base/src/nsRuleNode.cpp	17 Jan 2003 23:41:20 -0000	1.54
>+++ content/base/src/nsRuleNode.cpp	18 Jan 2003 08:11:56 -0000
>@@ -3275,18 +3275,18 @@
> 
>   // background-position: enum, length, percent (flags), inherit
>   if (eCSSUnit_Percent == colorData.mBackPositionX.GetUnit()) {
>-    bg->mBackgroundXPosition = (nscoord)(100.0f * colorData.mBackPositionX.GetPercentValue());
>+    bg->mBackgroundXPosition.mFloat = 100.0f * colorData.mBackPositionX.GetPercentValue();

Why not just skip the 100.0f now?  It's just slight loss of data in the round
trip.

>   else if (eCSSUnit_Enumerated == colorData.mBackPositionX.GetUnit()) {
>-    bg->mBackgroundXPosition = (nscoord)colorData.mBackPositionX.GetIntValue();
>+    bg->mBackgroundXPosition.mCoord = (nscoord)colorData.mBackPositionX.GetIntValue();
>     bg->mBackgroundFlags |= NS_STYLE_BG_X_POSITION_PERCENT;
>     bg->mBackgroundFlags &= ~NS_STYLE_BG_X_POSITION_LENGTH;

You need to be setting a float here.  (Did you test the background position
keywords?)

And if you remove the 100.0f factor as I suggested, that means dividing by 100
here, since the keyword table in nsCSSProps.cpp has 0, 50, and 100.

All these comments apply to the mBackgroundYPosition part as well.

>Index: content/base/src/nsStyleContext.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsStyleContext.cpp,v
>retrieving revision 3.205
>diff -u -r3.205 nsStyleContext.cpp
>--- content/base/src/nsStyleContext.cpp	8 Jan 2003 19:19:50 -0000	3.205
>+++ content/base/src/nsStyleContext.cpp	18 Jan 2003 08:11:57 -0000
>@@ -939,8 +939,8 @@
>     (int)bg->mBackgroundFlags,
>     (int)bg->mBackgroundRepeat,
>     (long)bg->mBackgroundColor,
>-    (long)bg->mBackgroundXPosition,
>-    (long)bg->mBackgroundYPosition,
>+    (long)bg->mBackgroundXPosition.mCoord,
>+    (long)bg->mBackgroundYPosition.mCoord,
>     NS_ConvertUCS2toUTF8(bg->mBackgroundImage).get());

You should at least comment that this is potentially lossy on some platforms.

>Index: layout/html/style/src/nsCSSRendering.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/html/style/src/nsCSSRendering.cpp,v
>retrieving revision 3.195
>diff -u -r3.195 nsCSSRendering.cpp
>--- layout/html/style/src/nsCSSRendering.cpp	17 Jan 2003 12:16:36 -0000	3.195
>+++ layout/html/style/src/nsCSSRendering.cpp	18 Jan 2003 08:11:58 -0000
>@@ -2397,13 +2397,13 @@
> {
>   nscoord x;
>   if (NS_STYLE_BG_X_POSITION_LENGTH & aColor.mBackgroundFlags) {
>-    x = aColor.mBackgroundXPosition;
>+    x = aColor.mBackgroundXPosition.mCoord;
>   }
>   else {

You should check the other flag here (for percent) to make sure you're not
reading data initialized as a PRInt32, and have an |else| for when neither is
set (presumably using zeros).

>-    nscoord t = aColor.mBackgroundXPosition;
>-    float pct = float(t) / 100.0f;
>-    nscoord tilePos = nscoord(pct * aTileWidth);
>-    nscoord boxPos = nscoord(pct * aOriginBounds.width);
>+    PRFloat64 posX = PRFloat64(aColor.mBackgroundXPosition.mFloat);
>+    PRFloat64 pct = posX / PRFloat64(100.0f);

If you drop the multiplying by 100 as I mentioned, you could drop the division
here.

The same comments apply to the Y case.

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v2

I am pretty sure that anonymous unions are OK: I know that we have them in
several other places in the codebase, nsROCSSPrimitiveValue.h among others.

This addresses the other comments.

Comment 8

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

Comment on attachment 111939 [details] [diff] [review]
Patch v2

>Index: content/shared/src/nsStyleStruct.cpp
>@@ -1007,12 +1005,20 @@
>+      (mBackgroundImage == aOther.mBackgroundImage) &&
>+      ((mBackgroundFlags & NS_STYLE_BG_X_POSITION_LENGTH) &&
>+       (mBackgroundXPosition.mCoord == aOther.mBackgroundXPosition.mFloat) ||

Coord, not Float

>+       (mBackgroundFlags & NS_STYLE_BG_X_POSITION_PERCENT) &&
>+       (mBackgroundXPosition.mFloat == aOther.mBackgroundXPosition.mFloat) ||
>+       !(mBackgroundFlags & (NS_STYLE_BG_X_POSITION_LENGTH | NS_STYLE_BG_X_POSITION_PERCENT))) &&
>+      ((mBackgroundFlags & NS_STYLE_BG_Y_POSITION_LENGTH) &&
>+       (mBackgroundYPosition.mCoord == aOther.mBackgroundYPosition.mFloat) ||

ditto

>+       (mBackgroundFlags & NS_STYLE_BG_Y_POSITION_PERCENT) &&
>+       (mBackgroundYPosition.mFloat == aOther.mBackgroundYPosition.mFloat) ||
>+       !(mBackgroundFlags & (NS_STYLE_BG_Y_POSITION_LENGTH | NS_STYLE_BG_Y_POSITION_PERCENT))))

However, would it be simpler to say

(!(mBackgroundFlags & NS_STYLE_BG_X_POSITION_PERCENT) ||
  (mBackgroundXPosition.mFloat == aOther.mBackgroundXPosition.mFloat)) &&
(!(mBackgroundFlags & NS_STYLE_BG_X_POSITION_LENGTH) ||
  (mBackgroundXPosition.mCoord == aOther.mBackgroundXPosition.mCoord))

(and the same for Y)?


with that, r+sr=dbaron

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

Checked in, with dbaron's suggested logic.