Bug 26030 - [Chromium] Chromium Linux ignores background color on <select>.
Summary: [Chromium] Chromium Linux ignores background color on <select>.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-26 14:23 PDT by Adam Langley
Modified: 2009-06-10 10:41 PDT (History)
1 user (show)

See Also:


Attachments
patch (2.50 KB, patch)
2009-05-26 14:24 PDT, Adam Langley
eric: review-
Details | Formatted Diff | Diff
patch (3.23 KB, patch)
2009-06-02 15:06 PDT, Adam Langley
eric: review-
Details | Formatted Diff | Diff
patch (7.63 KB, patch)
2009-06-03 12:29 PDT, Adam Langley
eric: review-
Details | Formatted Diff | Diff
patch (7.81 KB, patch)
2009-06-05 16:27 PDT, Adam Langley
no flags Details | Formatted Diff | Diff
patch (8.13 KB, patch)
2009-06-05 17:08 PDT, Adam Langley
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Langley 2009-05-26 14:23:36 PDT
Chromium Linux ignores background color on <select>. We always just draw the default gray gradient.
Comment 1 Adam Langley 2009-05-26 14:24:41 PDT
Created attachment 30682 [details]
patch
Comment 2 Eric Seidel (no email) 2009-06-02 13:23:30 PDT
Comment on attachment 30682 [details]
patch

Style issues.  no { } on single line ifs.

Don't we have a more efficient way to set this?

paint.setARGB(o->style()->backgroundColor().alpha(), o->style()->backgroundColor().red(), o->style()->backgroundColor().green(), o->style()->backgroundColor().blue());

There should be a way to pass a Color object to a SkPaint, I'm sure we have such a function somewhere.  color has a .rgb() accessor which probably returns a skpaint compatible int.
Comment 3 Adam Langley 2009-06-02 15:06:07 PDT
Created attachment 30881 [details]
patch

Changes:

1) Color::rgb does indeed return a value which is
   compatible. However, it seems be a bit of a leap that the
   internal format of both Color and SkColor is register
   ARGB. Other platforms define specific constructors in
   Color. However, both RGBA32 and SkColor unwrap to an
   unsigned int, so we can't do that here.

   I've used .rgb() because it works and it's cleaner. I
   guess layout tests will catch if either side ever
   changes.

2) html4.css was specifiying a default background-color for
   buttons which meant that we could never tell if the page
   was setting one (which we should observe) or not (in
   which case we should draw our default button). So I added
   a CSS snippet to override this.
Comment 4 Eric Seidel (no email) 2009-06-02 15:08:39 PDT
I'm pretty sure I added a COMPILE_ASSERT in ColorSkia.cpp that ensures that the internal formats are identical.  We could/should add an SkColor cast operator to Color as necessary though!
Comment 5 Eric Seidel (no email) 2009-06-02 15:13:21 PDT
Comment on attachment 30881 [details]
patch

Still has the { } style issues.

The .css file should be a real .css file on disk and go through the normal processing scripts, no?


I think I would have written this:
+        if (!o->style()->hasBackground()) {
+            paint.setARGB(0xff, 0xe9, 0xe9, 0xe9);
+        } else {
+            paint.setColor(o->style()->backgroundColor().rgb());
+        }

As something like:

static Color defaultButtonGrey(0xff, 0xe9, 0xe9, 0xe9);
Color buttonBackground = !o->style()->hasBackground() ? o->style()->backgroundColor() : defaultButtonGrey;
paint.setColor(buttonBackground.rgb());

But this makes me wonder if CSS buttons (which do not go through the button theme calls) will look correct w/o a background?
Comment 6 Adam Langley 2009-06-03 12:29:01 PDT
Created attachment 30920 [details]
patch

Next attempt:

This time we remove all the magic colour values from the paintButtonLike
function, except for a fallback colour. For painting <select>s and buttons, we
generate the family of colours needed by converting to HSL and changing the
luminance.

With this in hand, we set the CSS ButtonFace colour and the <select>
background-color and everything works out exactly the way is used to, except
that <select>'s with background colours are now handled correctly.

With this code, we could also handling drawing buttons with background-colors,
but that's something for another patch.
Comment 7 Eric Seidel (no email) 2009-06-03 17:16:10 PDT
Comment on attachment 30920 [details]
patch

I would have given this color a name:
 129         return 0xffdddddd;

static Color someNiceNameForTheColor(0xffdddddd);
return someNiceNameForTheColor;

Braces get their own line:
 264 static float brightenColorCap(float in) {


Single line ifs don't get braces:
292     if (max == min) {
 293         hue = 0.f;
 294     } else if (max == r) {

Color functions belong on Color.h or somewhere in Skia...
 282 static SkColor brightenColor(SkColor base, float brightenAmount)

256341 static void paintButtonLike(RenderTheme* theme, RenderObject* o, const RenderObject::PaintInfo& i, const IntRect& rect) {
 need { on its own line too.

WK of course uses CamelCase:
 334     int final_r = 255.f * brightenColorDecode(p, q, tr);

Looks nice though! :)

r- for color stuff needing to move somewhere else.
Comment 8 Adam Langley 2009-06-05 16:27:48 PDT
Created attachment 31019 [details]
patch

Moved the RGB->HSL conversion to Color and used the existing HSL->RGB conversion therein.
Comment 9 Adam Langley 2009-06-05 17:08:50 PDT
Created attachment 31020 [details]
patch
Comment 10 Eric Seidel (no email) 2009-06-05 17:12:57 PDT
Comment on attachment 31020 [details]
patch

Looks fine.  Please add a ChangeLog when landing.
Comment 11 mitz 2009-06-05 17:16:14 PDT
Comment on attachment 31020 [details]
patch

Why the round-trip through HSL? Can't you just make a version of Color::light() that takes a parameter?
Comment 12 mitz 2009-06-05 17:17:44 PDT
Comment on attachment 31020 [details]
patch

See <http://trac.webkit.org/changeset/21798>.
Comment 13 mitz 2009-06-05 17:20:35 PDT
Never mind, you want HSL, not HSV!
Comment 14 Adam Langley 2009-06-05 17:22:00 PDT
I could change Color::light, but the scaling of RGB components didn't work for me when I was playing around with different schemes. The colours I ended up with just didn't look quite right. I think HSL is the right way to do what I want here.
Comment 15 mitz 2009-06-05 17:24:02 PDT
Comment on attachment 31020 [details]
patch

> +    while (hue >= 360.0)
> +        hue -= 360.0;
> +
> +    // makeRGBAFromHSLA assumes that hue is in [0...1).
> +    hue /= 360.0;

What is the purpose of the while loop?

> +    lightness = 0.5f * (max + min);

The "f" seems unhelpful at best.

> +    else if (lightness <= 0.5f)

Ditto.
Comment 16 Adam Langley 2009-06-05 17:34:30 PDT
> What is the purpose of the while loop?

The spec said "mod 360", which is most accurately turned into a while loop.

Considering the case where max == r (which looks most likely to overflow 720 since it has the largest constant).

We have
hue = (60.0 * ((g - b) / (max - min))) + 360.0

so (g - b) / (max - min) must be < 6 in order not to overflow.

We know that max > g and max > b (since max == r) and that min == g or min == b.

Consider g >= b, thus b == min and thus g - b < max - min so the whole fraction is <= 1.
The alternative case leads, via similar reasoning to the fraction being >= -1.

So, we can change the while to an if. I'll do that.

The f's were a left over from when I wrote it with floats. I'll remove them, thanks.
Comment 17 Brent Fulgham 2009-06-10 10:41:21 PDT
Landed in http://trac.webkit.org/changeset/44503.