Bug 21248 – Support placeholder on textarea

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Andy Lyttle]

There's no reason why <textarea> shouldn't support the placeholder property, just like other text input fields do.

[Ojan Vafai]

I'd add also that contentEditable elements and designMode documents should support it as well. Really any text-input should support it.

[Adele Peterson]

This has now been added in the HTML5 draft.

[sideshowbarker]

http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#the-textarea-element
http://html5.org/tools/web-apps-tracker?from=2920&to=2921

[Kent Tamura]

*** Bug 26507 has been marked as a duplicate of this bug. ***

[Kent Tamura]

I'm making a patch for this.

[Kent Tamura]

Created attachment 31906 [details]
Proposed patch

[Eric Seidel (no email)]

Comment on attachment 31906 [details]
Proposed patch

It's sad we can't share more code here.  Adele should really comment here.

[Kent Tamura]

Created attachment 31919 [details]
Proposed patch (rev.2)

Some small changes.

[Kent Tamura]

Created attachment 32114 [details]
Proposed patch (rev.3)

Update for the rename of css/html4.css.

[Darin Adler]

Comment on attachment 32114 [details]
Proposed patch (rev.3)

Looks really good. Just a few comments.

> +String HTMLTextAreaElement::placeholder() const
> +{
> +    return getAttribute(placeholderAttr).string();
> +}
> +
> +void HTMLTextAreaElement::setPlaceholder(const String& value)
> +{
> +    setAttribute(placeholderAttr, value);
> +}

These are not great. There's no need to convert to a plain string here, and in fact it's bad for performance. The right way to do this nowadays is to just put [Reflect] in the IDL file and not define a setPlaceholder function in the .h and .cpp file at all, so that should be done here.

Since there are a couple call sites that do want placeholder, it would be OK to keep it but it could return const AtomicString& instead of String to avoid a bit of reference count churn and you probably would only have to use the string() function in one place.

> +    if ((oldPlaceholderShouldBeVisible != m_placeholderShouldBeVisible || placeholderValueChanged)
> +        && renderer())
> +        static_cast<RenderTextControlMultiLine*>(renderer())->updatePlaceholderVisibility();

Indenting here is awkward.

> +    bool placeholderShouldBeVisible() const { return m_placeholderShouldBeVisible; }

I think it would be a better design for the HTMLTextAreaElement to tell the 

> +    virtual void dispatchFocusEvent();
> +    virtual void dispatchBlurEvent();

These overrides should be private, not public, because we generally want all functions to be as private as possible.

> +    void setPlaceholderShouldBeVisible(bool visible) { m_placeholderShouldBeVisible = visible; }

I don't think you need this setter function. It doesn't seem to add any abstraction.

> +                 attribute  DOMString            placeholder;

Just adding the [Reflect] attribute here will do the trick.

> +void RenderTextControl::updatePlaceholderVisibility()
> +{
> +    RefPtr<RenderStyle> textBlockStyle = createInnerTextStyle(parentStyle());
> +    HTMLElement* innerText = innerTextElement();
> +    innerText->renderer()->setStyle(textBlockStyle);
> +
> +    for (Node* n = innerText->firstChild(); n; n = n->traverseNextNode(innerText)) {
> +        if (RenderObject* renderer = n->renderer())
> +            renderer->setStyle(textBlockStyle);
> +    }
> +
> +    updateFromElement();
> +}

I know you must moved this from one class to another, but this function needs some comments. It's unclear why updatePlaceholderVisibility needs to update the style of all the renderers and then call updateFromElement. A comment could explain the relationship. Also, we could consider moving code to set the text block style into a separate function, since the name of that function would help document it.

> -    if (result.innerNode() == node() || result.innerNode() == innerTextElement())
> +    HTMLTextAreaElement* textArea = static_cast<HTMLTextAreaElement*>(node());
> +    if (result.innerNode() == node()
> +        || !textArea->placeholderShouldBeVisible() && result.innerNode() == innerTextElement()
> +        || textArea->placeholderShouldBeVisible() && result.innerNode()->isDescendantOf(innerTextElement()))
>          hitInnerTextElement(result, x, y, tx, ty);

The way the formatting makes the clauses of the if statement line up with its body makes things like this hard to read.

I think you could consider fixing that by using a boolean local variable; the name of the boolean could even help document a bit. I think the code also needs a comment. The rule here is slightly unclear unless the reader really aware of how placeholders should behave.

I'm not so sure that it's great to move the m_placeholderVisible data member into the RenderTextControl base class. While it's true that both derived classes use it, the base class does not.

> +RenderStyle* RenderTextControlMultiLine::parentStyle()

I don't understand the use of "parent" in this name. This isn't really the style of the renderer's parent, because if so it wouldn't need to be a virtual function. I know the local variable was named that, but a local variable has less scope than a member function, and naming is slightly less critical when the name is that tightly scoped. But could we give this function a name that's more precise? Sorry, I don't have a specific name to suggest.

> +protected:
> +    virtual RenderStyle* parentStyle();

This can and should be private instead of protected. You can override a function with a private function, and we should make things as private as possible. If we ever did want to derive from this class we could make it protected at that time.

> +protected:
> +    virtual RenderStyle* parentStyle();

Ditto.

review- because at least some of the changes above should be made

[Kent Tamura]

Thank you for the comments!

(In reply to comment #10)
> (From update of attachment 32114 [details])
> Looks really good. Just a few comments.
> 
> > +String HTMLTextAreaElement::placeholder() const
> > +{
> > +    return getAttribute(placeholderAttr).string();
> > +}
> > +
> > +void HTMLTextAreaElement::setPlaceholder(const String& value)
> > +{
> > +    setAttribute(placeholderAttr, value);
> > +}
> 
> These are not great. There's no need to convert to a plain string here, and in
> fact it's bad for performance. The right way to do this nowadays is to just put
> [Reflect] in the IDL file and not define a setPlaceholder function in the .h
> and .cpp file at all, so that should be done here.

Ok, I changed this to [Reflect].

> Indenting here is awkward.

Fixed.

> > +    bool placeholderShouldBeVisible() const { return m_placeholderShouldBeVisible; }
> 
> I think it would be a better design for the HTMLTextAreaElement to tell the 

I agree.  When I looked at the existing placeholder code first time, I had an impression that the code was complex.  I think the reason is that both of a DOM node and a renderer have flags for placeholder visibility.
I have removed the flag of InputElemntData, and updated many place to tell the visibility from a DOM node to a renderer.

> > +    virtual void dispatchFocusEvent();
> > +    virtual void dispatchBlurEvent();
> 
> These overrides should be private, not public, because we generally want all
> functions to be as private as possible.

Fixed.

> > +    void setPlaceholderShouldBeVisible(bool visible) { m_placeholderShouldBeVisible = visible; }
> 
> I don't think you need this setter function. It doesn't seem to add any
> abstraction.

Removed.

> > +void RenderTextControl::updatePlaceholderVisibility()
> > +{
> > +    RefPtr<RenderStyle> textBlockStyle = createInnerTextStyle(parentStyle());
> > +    HTMLElement* innerText = innerTextElement();
> > +    innerText->renderer()->setStyle(textBlockStyle);
> > +
> > +    for (Node* n = innerText->firstChild(); n; n = n->traverseNextNode(innerText)) {
> > +        if (RenderObject* renderer = n->renderer())
> > +            renderer->setStyle(textBlockStyle);
> > +    }
> > +
> > +    updateFromElement();
> > +}
> 
> I know you must moved this from one class to another, but this function needs
> some comments. It's unclear why updatePlaceholderVisibility needs to update the
> style of all the renderers and then call updateFromElement. A comment could
> explain the relationship. Also, we could consider moving code to set the text
> block style into a separate function, since the name of that function would
> help document it.

I have addded a new method, setInnerTextStyle().

> > -    if (result.innerNode() == node() || result.innerNode() == innerTextElement())
> > +    HTMLTextAreaElement* textArea = static_cast<HTMLTextAreaElement*>(node());
> > +    if (result.innerNode() == node()
> > +        || !textArea->placeholderShouldBeVisible() && result.innerNode() == innerTextElement()
> > +        || textArea->placeholderShouldBeVisible() && result.innerNode()->isDescendantOf(innerTextElement()))
> >          hitInnerTextElement(result, x, y, tx, ty);
> 
> The way the formatting makes the clauses of the if statement line up with its
> body makes things like this hard to read.
> 
> I think you could consider fixing that by using a boolean local variable; the
> name of the boolean could even help document a bit. I think the code also needs
> a comment. The rule here is slightly unclear unless the reader really aware of
> how placeholders should behave.

Done.

> I'm not so sure that it's great to move the m_placeholderVisible data member
> into the RenderTextControl base class. While it's true that both derived
> classes use it, the base class does not.

In the next patch, RenderTextControl refers m_placeholderVisible.

> > +RenderStyle* RenderTextControlMultiLine::parentStyle()

Renamed it to textBaseStyle().

> > +protected:
> > +    virtual RenderStyle* parentStyle();
> 
> This can and should be private instead of protected. You can override a
> function with a private function, and we should make things as private as
> possible. If we ever did want to derive from this class we could make it
> protected at that time.

Fixed.

[Kent Tamura]

Created attachment 34534 [details]
Proposed patch (rev.4)

Sorry for the large patch.
* The most of changes in HTMLInputElment, InputElment, WMLInputElement are trivial. (removal of unused parameters)
* We have a bug about maxlength.  The patch accidentally fixes it because of RenderTextControlSingleLine::updateFromElement() change.  input-text-maxlength-expected.txt had wrong expected data.

[Eric Seidel (no email)]

Comment on attachment 34534 [details]
Proposed patch (rev.4)

This looks great.  I'm just sad we can's share implementations:

+    static void dispatchFocusEvent(InputElement*, Element*);
+    static void dispatchBlurEvent(InputElement*, Element*);
+    static bool placeholderShouldBeVisible(const InputElement*, const Element*);
+    static void updatePlaceholderVisibility(InputElement*, Element*, bool placeholderValueChanged = false);

We could put those on HTMLFormControlElement... or on a new HTMLTextControlElement subclass.  Or on some class which both of these used (since "having" placeholder text does not necessarily mean you need to "be" a classs which can have placehodler text.  We could use 'has a' instead of 'is a' to share code.)

I'm going to approve this as is.  I would like you to consider a follow-up patch to share more of this code.

[Eric Seidel (no email)]

Comment on attachment 34534 [details]
Proposed patch (rev.4)

Rejecting patch 34534 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Eric Seidel (no email)]

fast/forms/input-text-maxlength.html -> failed

[Kent Tamura]

Created attachment 38467 [details]
Proposed patch (rev.5)

Updated test cases.

[Kent Tamura]

Created attachment 38468 [details]
Proposed patch (rev.6)

Oops, I found LayoutTests/ChangeLog in the rev.5 was incomplete.  Corrected it.

[Kent Tamura]

(In reply to comment #13)
> I'm going to approve this as is.  I would like you to consider a follow-up
> patch to share more of this code.

Sure.  I'll try it.

[Eric Seidel (no email)]

Comment on attachment 38468 [details]
Proposed patch (rev.6)

LGTM.

[Eric Seidel (no email)]

Comment on attachment 38468 [details]
Proposed patch (rev.6)

Clearing flags on attachment: 38468

Committed r47702: <http://trac.webkit.org/changeset/47702>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

4 textarea tests are still failing after this change.

[Eric Seidel (no email)]

(In reply to comment #22)
> 4 textarea tests are still failing after this change.

On what platform?  I assume windows since mac is green.

[Eric Seidel (no email)]

http://build.webkit.org/results/Windows%20Release%20(Tests)/r47719%20(3707)/results.html

Looks like windows will need new baselines or these tests will need to be converted not to be render tree dumps. tkent could do the test conversion, or we can just land the windows results from the bots.

[Eric Seidel (no email)]

Will land windows results from the bot, shortly.

[Eric Seidel (no email)]

Committed r47721: <http://trac.webkit.org/changeset/47721>