[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