Bug 21227 - Implement placeholder-color CSS property
: Implement placeholder-color CSS property
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: All Mac OS X 10.5
: P2 Enhancement
Assigned To:
: http://phroggy.com/placeholder.html
: InRadar
:
: 21238
  Show dependency treegraph
 
Reported: 2008-09-29 19:05 PST by
Modified: 2009-11-12 11:13 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-09-29 19:05:04 PST
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 From 2008-09-30 12:53:22 PST -------
<rdar://problem/6222134>
------- Comment #2 From 2008-09-30 14:47:45 PST -------
Created an attachment (id=23955) [details]
patch
------- Comment #3 From 2008-09-30 15:32:28 PST -------
(From update of attachment 23955 [details])
+    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 From 2008-09-30 16:23:38 PST -------
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 From 2008-10-02 17:23:28 PST -------
I changed this to be a pseudo-element because of bug 21299.

Now the syntax is:

input::-webkit-input-placeholder {
    color: red;
}
------- Comment #6 From 2008-10-02 17:26:19 PST -------
latest fix is in revision 37217.