Custom link colors not rendered correctly

VERIFIED FIXED

Status

()

VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: deisom, Assigned: durbacher)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
Found a glitch when changing the link color defaults in
Preferences/Appearance/Colors & Links. When I specify a dark blue with the RGB
values of 6,56,122 for Unvisited Links, Chimera renders links in a totally
different color (neither the default nor the color that I've specified). The
same thing happens when I try to use 7,73,23 for Visited Links. I've also
noticed the same effect with some other color choices—some work fine, others do not.

Comment 1

16 years ago
Created attachment 104100 [details]
Crash Log

Confirmed in 2002102104; the dark blue color renders as plumb, and the dark
green as apricot. This happens no matter what you set them to (link, text,
back, etc.), and actually occurs with lots of colors entered with the RGB
sliders. I was able to crash Chim [Thread 0:
 #0   0x90025e8c in select
 #1   0x0101c67c in poll
 #2   0x01018e50 in _pr_poll_with_poll
 #3   0x00173ef8 in _ZN24nsSocketTransportService3RunEv
 #4   0x05052a34 in _ZN8nsThread4MainEPv
 #5   0x0101a2bc in _pt_root
 #6   0x90021428 in _pthread_body]

after using the RGB sliders enough.

Comment 2

16 years ago
confirming
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

16 years ago
This doesn't happen in Mail.app (v.1.2 on 10.2.1), so it doens't appear to be a
system color picker bug.

Comment 4

16 years ago
I noticed this today and did a little investigation.  I've narrowed down the
problem to an issue in Gecko (and hence all Gecko-embedders).  If any of the RGB
components in the default has a value of less than 0x10, i.e. 00-0F the
component's digits are switched, i.e. #0F0F0F becomes #F0F0F0.

So in the reporters two examples, the colors that my tests yielded are:

rgb(6,56,122) = #60387A is rendered as #06387A
rgb(7,73,23)  = #074917 is rendered as #704917

This does not affect hardcoded colors via CSS or HTML.  It only applies to
custom colors set via preferences (or prefs.js/user.js). 

I have verified this in Camino 2003040406, Mozilla 20030407 for Mac OS X and
Windows, and Mozilla 20030312 for Linux (Red Hat) (needs a more recent
verification).  Like I said, this is an issue with Gecko, so I'm making the
following changes:

Product -> Browser
Component -> Preferences
Hardware -> All
OS -> All

Feel free to set it back as you see fit, but I've pretty much verified it on the
three major platforms and in an embedder.  Also, the embed keyword might want to
be added, but this is not a blocker by any means.

To verify my claims, Mac OS X users may use the DigitalColor Meter in
/Applications/Utilities/.  Set the popup to "RGB as Hex Value, 8-bit."  Then set
the default link color/visited link color to a hex number like #0F0F0F, i.e.
rgb(15, 15, 15), and use the DigitalColor Meter to verify that the actual
rendered result is #F0F0F0 = rgb(240,240,240).  There may be very small
discrepancies due to anti-aliasing on Mac OS X.  To avoid this, make your text
size very large, and test near the center of a character. 

If I missed the proper bug and this is a dupe, please include these comments in
the bug it's a duplicate of (if this information has not already been presented).
Component: General → Preferences
OS: MacOS X → All
Product: Camino → Browser
Hardware: Macintosh → All
Version: unspecified → Trunk
not sure why saari would own this.
Assignee: saari → ben
QA Contact: winnie → sairuh
(Assignee)

Comment 6

15 years ago
This is a layout bug - a color conversion method there is making a mistake. Taking.
Assignee: bugs → durbacher
Component: Preferences → Layout
(Assignee)

Comment 7

15 years ago
Created attachment 139282 [details] [diff] [review]
patch

Replaces the bad color-to-string conversion routine by a (hopefully) error free
one which by the way is much shorter.
(Assignee)

Comment 8

15 years ago
Created attachment 139285 [details] [diff] [review]
almost same patch

Actually, I think the buffer can be made a bit smaller.
Attachment #139282 - Attachment is obsolete: true
(Assignee)

Comment 9

15 years ago
Comment on attachment 139285 [details] [diff] [review]
almost same patch

Requesting r= (or r/sr?) from dbaron.

As said on IRC, this is as inFlasher.cpp#74 with the buffer made smaller
because I don't think it has to have the size [10]. Might be wrong, but at
least does not crash here...
I also tested with a debugger that the conversion is being done correctly.
I had to #include prprf.h. Biesi said it's no problem. :-)
However, if it is or the place where I inserted it is inappropriate, just say
it.
Attachment #139285 - Flags: review?(dbaron)
Comment on attachment 139285 [details] [diff] [review]
almost same patch

>+NS_ASSERTION(nsnull, "jetzt");

remove this

>+  aString.Assign(NS_ConvertASCIItoUCS2(buf));

Change this to:

  CopyASCIItoUTF16(buf, aString);

and r=dbaron
Attachment #139285 - Flags: review?(dbaron) → review+
(Assignee)

Comment 11

15 years ago
Created attachment 139312 [details] [diff] [review]
patch with review comments addressed

> > +NS_ASSERTION(nsnull, "jetzt");

Damn, this is awkward...
Patch also changes the other thing and was compiled and tested. Assuming
r=dbaron, for this new one but...
(Assignee)

Updated

15 years ago
Attachment #139285 - Attachment is obsolete: true
(Assignee)

Comment 12

15 years ago
Comment on attachment 139312 [details] [diff] [review]
patch with review comments addressed

...requesting additional sr= from bz.
Attachment #139312 - Flags: superreview?(bz-vacation)
Comment on attachment 139312 [details] [diff] [review]
patch with review comments addressed

sr=me.	I assume you need this checked in too?
Attachment #139312 - Flags: superreview?(bz-vacation) → superreview+
Checking in layout/html/base/src/nsPresShell.cpp;
/cvsroot/mozilla/layout/html/base/src/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.686; previous revision: 3.685
done
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Comment 15

15 years ago
Verified using the latest nightly.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.