Bug 21227 - Implement placeholder-color CSS property
Summary: Implement placeholder-color CSS property
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Enhancement
Assignee: Adele Peterson
URL: http://phroggy.com/placeholder.html
Keywords: InRadar
Depends on:
Blocks: 21238
  Show dependency treegraph
 
Reported: 2008-09-29 19:05 PDT by Andy Lyttle
Modified: 2009-11-12 11:13 PST (History)
0 users

See Also:


Attachments
patch (173.41 KB, patch)
2008-09-30 14:47 PDT, Adele Peterson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Lyttle 2008-09-29 19:05:04 PDT
Since the text and background color for text input fields can be specified with CSS, I think it's necessary to be able to specify the placeholder color as well.  Gray is perfectly appropriate when the background is white, but if you happen to want the background-color to be #808080 for some reason, it would be nice if the placeholder text were readable.
Comment 1 Adele Peterson 2008-09-30 12:53:22 PDT
<rdar://problem/6222134>
Comment 2 Adele Peterson 2008-09-30 14:47:45 PDT
Created attachment 23955 [details]
patch
Comment 3 Darin Adler 2008-09-30 15:32:28 PDT
Comment on attachment 23955 [details]
patch

+    String placeholder;
+    if (value().isEmpty() && document()->focusedNode() != this)
+        placeholder = getAttribute(placeholderAttr);

It'd be more efficient to have a boolean here instead of a String. It's not necessary to copy the placeholder attribute into a local String just to later check if it's 0.

+    if (!placeholder.isEmpty() || m_placeholderShouldBeVisible)
+        m_placeholderShouldBeVisible = !placeholder.isEmpty();

The if statement here doesn't add value. The assignment would work the same without the if. I think I see how you cut this down from the original RenderTextControl function.

I'd just write it like this:

    m_placeholderShouldBeVisible = value().isEmpty()
        && document()->focusedNode() != this
        && !getAttribute(placeholderAttr).isEmpty();

That doesn't require any if statements or local variables.

+    if (oldPlaceholderShouldBeVisible != m_placeholderShouldBeVisible)
+        setChanged();
+
+}

There's an extra blank line here at the end of this function.

It seems a little weak that both the DOM and the render tree cache a boolean to remember whether the placeholder is visible, but I guess it's smart for the render object to cache it so it can properly handle changes in state, and the DOM needs to cache it to make style resolution fast.

+// Value chosen by observation.  This can be tweaked.
+static const int minColorDifferenceSquared = 1300;
+static Color disabledTextColor(const Color& textColor, const Color& backgroundColor)

Normally the constant would go in a separate paragraph at the start of the file. Maybe all that's missing is a blank line. I think the constant name should include the word "contrast".

+    // If there's not very much contrast between the disabled color and the background color,
+    // just leave the text color alone.  We don't want to change a good contrast color scheme so that it has really bad contrast.
+    // If the the contrast was already poor, then it doesn't do any good to change it to a different poor contrast color scheme.
+    if (differenceSquared(disabledColor, backgroundColor) >= minColorDifferenceSquared)
+        return disabledColor;
+    return textColor;

The code is the opposite of the comment. It says "if there's enough contrast, return the computed color". You should reverse the code to match the comment, which says "if there's not enough contrast, leave the color alone".

+    if (!m_multiLine && static_cast<HTMLInputElement*>(element)->placeholderShouldBeVisible())
+        textBlockStyle->setTextSecurity(TSNONE);

This needs a comment. Also, I think it's fixing a separate bug. Is there a regression test for this bug?

+        ExceptionCode ec = 0;

No need to set this to 0 here if you're not going to look at the result. This was the same in the old code.

r=me as-is, or with some or all of my suggestions
Comment 4 Adele Peterson 2008-09-30 16:23:38 PDT
Committed revision 37123.

You can now do style the input element when its showing the placeholder text like this:

input:-webkit-input-placeholder-mode {
    color: red;
}
Comment 5 Adele Peterson 2008-10-02 17:23:28 PDT
I changed this to be a pseudo-element because of bug 21299.

Now the syntax is:

input::-webkit-input-placeholder {
    color: red;
}

Comment 6 Adele Peterson 2008-10-02 17:26:19 PDT
latest fix is in revision 37217.