Implement placeholder-color CSS property

• Component: CSS
• Status: RESOLVED FIXED

[Andy Lyttle]

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.

[Adele Peterson]

<rdar://problem/6222134>

[Adele Peterson]

Created attachment 23955 [details]
patch

[Darin Adler]

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

[Adele Peterson]

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;
}

[Adele Peterson]

I changed this to be a pseudo-element because of bug 21299.

Now the syntax is:

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

[Adele Peterson]

latest fix is in revision 37217.