[Webkit-unassigned] [Bug 36291] placeholder text should be stripped from line breaks

Mon May 10 06:25:52 PDT 2010

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

--- Comment #11 from Kent Tamura <tkent at chromium.org>  2010-05-10 06:25:51 PST ---
Thank you for reviewing!

(In reply to comment #9)
> I looked in HTML5 and could not find any text saying that we need to remove CR and LF from attribute values. What I saw was text telling HTML5 content authors they must not include CR and LF in their attribute values, but nothing that tells HTML5 browser implementations what to do.

Here it is:
http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-element-attributes.html#the-placeholder-attribute
| User agents should present this hint to the user,
| after having stripped line breaks from it, when the
| element's value is the empty string and the control
| is not focused (e.g. by displaying it inside a blank
| unfocused control).

> > +<head>
> > +<link rel="stylesheet" href="../../../../fast/js/resources/js-test-style.css"
> > +</head>
> 
> This is strange. Why do you have an unterminated <link> element here? I think you should just leave out this <head> entirely. Do you need this style sheet at all?

You're right.  This is not needed at all.  Removed.

> > +static bool isNewLineCharacter(UChar ch) { return ch == newlineCharacter || ch == carriageReturn; }
> > +
> > +static bool isNotNewLineCharacter(UChar ch) { return ch != newlineCharacter && ch != carriageReturn; }
> 
> Why these particular function names? Are these two characters called "new line characters"? Is that terminology from the HTML5 specification? I'd prefer a different name given that one of these characters is called "newlineCharacter". The function name seems too close to that.

My naming was wrong. The specification calls them "line breaks".  So I should name them is(Not)LineBreak().

> > +    if (attributeValue.string().find(isNewLineCharacter) == -1)
> > +        return attributeValue;
> 
> I'm sure it's much more efficient to call contains on a UChar twice than to call the version of find that takes a character-matching function once. I suggest using contains.

Ok, I changed it to two contains() calls.

> > +bool HTMLTextFormControlElement::isPlaceholderEmpty() const
> > +{
> > +    const AtomicString& attributeValue = getAttribute(placeholderAttr);
> > +    if (attributeValue.isEmpty())
> > +        return true;
> > +    return attributeValue.string().find(isNotNewLineCharacter) == -1;
> > +}
> 
> I don't think you need a special case for the empty string here. The find function is already quite fast for empty strings, and will give the correct result.

Removed attributeValue.isEmpty().

> > +        // node() should be an HTMLInputElement. WMLInputElement doesn't support placeholder.
> 
> The phrase "should be" is not strong enough. I would say "must be".

Changed to "must be".

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