176637 - Custom link colors not rendered correctly

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[deisom]

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

[Steve Smart]

Attachment: 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

[Steve Smart]

confirming

Comment 3

[Steve Smart]

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

[Prachi Gauriar]

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).

Comment 5

[Mike Pinkerton (not reading bugmail)]

not sure why saari would own this.

Comment 6

[Andreas Kunz]

This is a layout bug - a color conversion method there is making a mistake. Taking.

Comment 7

[Andreas Kunz]

Attachment: patch

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

Comment 8

[Andreas Kunz]

Attachment: almost same patch

Actually, I think the buffer can be made a bit smaller.

Comment 9

[Andreas Kunz]

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.

Comment 10

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

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

Comment 11

[Andreas Kunz]

Attachment: 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...

Comment 12

[Andreas Kunz]

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

...requesting additional sr= from bz.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

sr=me.	I assume you need this checked in too?

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 15

[Andreas Kunz]

Verified using the latest nightly.