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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri May 7 18:21:32 PDT 2010


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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #55391|review?                     |review-
               Flag|                            |




--- Comment #9 from Darin Adler <darin at apple.com>  2010-05-07 18:21:31 PST ---
(From update of attachment 55391)
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.

> +<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?

> +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.

> +    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.

> +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.

> +        // node() should be an HTMLInputElement. WMLInputElement doesn't support placeholder.

The phrase "should be" is not strong enough. I would say "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.



More information about the webkit-unassigned mailing list