[Webkit-unassigned] [Bug 21248] Support placeholder on textarea

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=21248


TAMURA, Kent <tkent at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |darin at apple.com




--- Comment #11 from TAMURA, Kent <tkent at chromium.org>  2009-08-10 19:03:34 PDT ---
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.