135208 - Text color does not print correctly on color printer

Status: VERIFIED FIXED

(Core Graveyard :: GFX, defect, P1)

Description

[Kevin McCluskey (gone)]

taken from http://bugscape.mcom.com/show_bug.cgi?id=12164

1. create a mail with various text colors in it
2. print the mail -> only select colors render
note: of all the colors in the color chart, only about 10 (including black)
render. does not effect images.

Don's comment:
I think the cause is the following.  Currently we do not print backgrounds.. so 
if we print and there was a background on the screen.. we darken (uncolor) the 
text.. because white text on a dark background is good.. white text on a white 
piece of paper.. no background is bad.  SO we darken that text.. and it looses 
color.

Comment 1

[Kevin McCluskey (gone)]

Don, I think the darkening code should use the HSV color space and darken the
colors using the V component instead of converting the color to black when we
are printing without background colors or images.

Comment 2

[Kevin McCluskey (gone)]

You can also test this by printing the URL
(http://www.geocities.com/debugweyers/colors.html) on a color printer

Comment 3

[dcone (gone)]

Attachment: Patch to fix color problem.

Comment 4

[Marc Attinasi]

Comment on attachment 78210 [details] [diff] [review]
Patch to fix color problem.

I think that the percentReduce value has to be at least 1, so maybe you should
have percentReduce = NS_MAX(1,(currentBrightness-64)*100/255);

Also, the comment right above that is incomplete.

percentReduce is a PRInt32 but it is passed as a param that is PRInt16 (in
NS_DarkenColor) - this should be fixed to use PRInt16 in both cases I think.

Finally, you are getting currentBirghtness again, after you darken it, which is
probably just DEBUG code and shoudl be removed or #ifdef'd

With those items tended to, sr=attinasi

Comment 5

[dcone (gone)]

Attachment: Patch to use HSV

Comment 6

[Marc Attinasi]

Comment on attachment 78409 [details] [diff] [review]
Patch to use HSV

Dude, this may be common-knowledge to you, but PLEASE comment those new
conversion functions for the rest of us ;)

sr=attinasi if you make it clear what is happening in HSV2RGB and RGB2HSV, and
explain in the bug why you are doing it this way vs. the old patch.

Comment 7

[Kevin McCluskey (gone)]

I don't think we are suppose to use PRIntn anymore, why not use PRUint16?
PRIntn  r,g,b,max,min,delta;
PRIntn  r,g,b;
PRIntn  i,p,q,t;

2. could you change the uppercase F for float constants to lowercase f for
consistency with the rest of the code


3. aHue = (int)hue;  Shouldn't this be: aHue = (PRUint16)hue;
4.  i = (int) floor(h); This should not use int. Why not PRUint16?

5. the function HSV2RGB(nscolor &aColor,PRUint16 aHue,PRUint16 aSat,PRUint16 aValue)

allows aHue to be an PRUint16 value that is later used to calculate i which is
used in
 switch(i){
      case 0: r = aValue; g = t; b = p;break;
      case 1: r = q; g = aValue; b = p;break;
       case 2: r = p; g = aValue; b = t;break;
       case 3: r = p; g = q; b = aValue;break;
       case 4: r = t; g = p; b = aValue;break;
       case 5: r = aValue; g = p; b = q;break;    
  }

I think you need to ensure that (i) will betwee 0-5 or add a default to catch
the case where (i) is greater than 5

6. Can you safely call floor() on all platforms? Is there a PRfloor you can call
instead?

Comment 8

[dcone (gone)]

Attachment: Newest patch taking marcs and kevins suggestions

Comment 9

[dcone (gone)]

Attachment: new fixes..

Comment 10

[Marc Attinasi]

Comment on attachment 78650 [details] [diff] [review]
Newest patch taking marcs and kevins suggestions

still has 'int' 
+  aHue = (int)hue; 

and 'floor'

+    i = (int) floor(h);

as well as 'F' 
+      hue= 2.0F+(float)(b-r)/(float)delta;

<nit>
Also, can you add a space before the 'break' in the switch stmt please?
</nit>

Comment 11

[Kevin McCluskey (gone)]

Comment on attachment 78654 [details] [diff] [review]
new fixes.. 

r=[e-mail]

Comment 12

[dcone (gone)]

Attachment: Last patch. That floor is used xplatform.

Comment 13

[dcone (gone)]

Attachment: Last patch. That floor is used xplatform.

Comment 14

[Kevin McCluskey (gone)]

Comment on attachment 78661 [details] [diff] [review]
Last patch.  That floor is used xplatform.

r=[e-mail]

Comment 15

[Marc Attinasi]

Comment on attachment 78661 [details] [diff] [review]
Last patch.  That floor is used xplatform.

Muchas Gracias, amigo!

SR=attinasi

Comment 16

[Asa Dotzler [:asa]]

Comment on attachment 78661 [details] [diff] [review]
Last patch.  That floor is used xplatform.

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 17

[Jaime Rodriguez, Jr.]

Needed by many shared customers on the branch. adt1.0.0+ (on ADTs behalf)
checkin to 1.0. Pls check this into the trunk and 1.0 branch.

Comment 18

[timeless]

this broke mac. when you go to land on the branch, please remember to use the fixed version from cvs instead of the patch that was checked into trunk.

Comment 19

[Aleksey Nogin]

This commit added the following warnings:

gfx/src/nsColor.cpp:460
 `float hue' might be used uninitialized in this function

gfx/src/nsColor.cpp:518
 `PRUint16 b' might be used uninitialized in this function
 `PRUint16 g' might be used uninitialized in this function
 `PRUint16 r' might be used uninitialized in this function

It seems that compiler is right, at least about the hue.

Comment 20

[dcone (gone)]

  if (aSat==0) {
    hue = 1000;
  } else {
    if(r==max){
      hue=(float)(g-b)/(float)delta;
    } else if (g==max) {
      hue= 2.0F+(float)(b-r)/(float)delta;
    } else if (b==max) {
      hue = 4.0F+(float)(r-g)/(float)delta;
    }
  
Since either r or g or b has to be a max.. hue will always get set.

the other.. warning is wrong to.. since r, g and b are set in the switch.. the
code above it guarentees that.  But I will set those(hue,r,g,b) to default
values just to get rid of the warnings.

Comment 21

[Aleksey Nogin]

If you know one of the 3 must be ==max, why not just do

- } else if (b==max) {
+ } else { // (b==max)

This way it's more efficient and the warning is gone. 

Comment 22

[dcone (gone)]

ok.. I will do it that way.

Comment 23

[dcone (gone)]

Fixed on trunk and branch

Comment 24

[Chris Petersen]

Verified on the OS X trunk (2002-04-18-07) and Windows ME trunk (2002-04-18-03).

Comment 25

[Chris Petersen]

Verified on branch OS X (2002-04-18-05) and Windows ME (2002-04-17-11).