If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Text color does not print correctly on color printer

VERIFIED FIXED in mozilla1.0

Status

Core Graveyard
GFX
P1
normal
VERIFIED FIXED
16 years ago
9 years ago

People

(Reporter: Kevin McCluskey (gone), Assigned: dcone (gone))

Tracking

({topembed+})

Trunk
mozilla1.0
x86
All
topembed+

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt3], URL)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

16 years ago
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.
(Reporter)

Comment 1

16 years ago
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.
Keywords: nsbeta1+, topembed+
Priority: -- → P1
Whiteboard: [adt2]
Target Milestone: --- → mozilla1.0
(Reporter)

Comment 2

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

Updated

16 years ago
Whiteboard: [adt2] → [adt3]
(Assignee)

Comment 3

16 years ago
Created attachment 78210 [details] [diff] [review]
Patch to fix color problem.

Comment 4

16 years ago
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
Attachment #78210 - Flags: superreview+
(Assignee)

Comment 5

16 years ago
Created attachment 78409 [details] [diff] [review]
Patch to use HSV
(Assignee)

Updated

16 years ago
Attachment #78210 - Attachment is obsolete: true

Comment 6

16 years ago
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.
Attachment #78409 - Flags: superreview+
(Reporter)

Comment 7

16 years ago
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?
(Assignee)

Comment 8

16 years ago
Created attachment 78650 [details] [diff] [review]
Newest patch taking marcs and kevins suggestions
Attachment #78409 - Attachment is obsolete: true
(Assignee)

Comment 9

16 years ago
Created attachment 78654 [details] [diff] [review]
new fixes..
Attachment #78650 - Attachment is obsolete: true

Comment 10

16 years ago
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>
Attachment #78650 - Attachment is obsolete: false
Attachment #78650 - Flags: needs-work+
(Reporter)

Comment 11

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

r=kmcclusk@netscape.com
Attachment #78654 - Flags: review+
(Assignee)

Comment 12

16 years ago
Created attachment 78660 [details] [diff] [review]
Last patch.  That floor is used xplatform.
(Assignee)

Comment 13

16 years ago
Created attachment 78661 [details] [diff] [review]
Last patch.  That floor is used xplatform.
(Assignee)

Updated

16 years ago
Attachment #78650 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #78654 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #78660 - Attachment is obsolete: true
(Reporter)

Comment 14

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

r=kmcclusk@netscape.com
Attachment #78661 - Flags: review+

Comment 15

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

Muchas Gracias, amigo!

SR=attinasi
Attachment #78661 - Flags: superreview+

Comment 16

16 years ago
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
Attachment #78661 - Flags: approval+

Comment 17

16 years ago
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.
Keywords: adt1.0.0+, mozilla1.0

Comment 18

16 years ago
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

16 years ago
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.
(Assignee)

Comment 20

16 years ago
  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

16 years ago
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. 
(Assignee)

Comment 22

16 years ago
ok.. I will do it that way.
(Assignee)

Comment 23

16 years ago
Fixed on trunk and branch
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Reporter)

Updated

16 years ago
Keywords: fixed1.0.0

Comment 24

16 years ago
Verified on the OS X trunk (2002-04-18-07) and Windows ME trunk (2002-04-18-03).
Status: RESOLVED → VERIFIED

Comment 25

16 years ago
Verified on branch OS X (2002-04-18-05) and Windows ME (2002-04-17-11).
Keywords: fixed1.0.0 → verified1.0.0
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.