Bug 70586 - [Forms] Setting defaultValue should hide an input placeholder.
Summary: [Forms] Setting defaultValue should hide an input placeholder.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-20 23:35 PDT by yosin
Modified: 2011-10-24 01:13 PDT (History)
5 users (show)

See Also:


Attachments
Patch 1 (3.31 KB, patch)
2011-10-23 22:36 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch (3.31 KB, patch)
2011-10-23 22:38 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch (4.13 KB, patch)
2011-10-23 23:15 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch (4.37 KB, patch)
2011-10-24 00:04 PDT, yosin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yosin 2011-10-20 23:35:19 PDT
Following HTML should not should placeholder in text field:

<input type="text" placeholder="placeholder" id="t">
<script>
 document.getElementById("t").defaultValue="default";
</script>

Expect:
Text input field displays "default".

Result:
Text input field displays both "default" and "placeholder"
Comment 1 yosin 2011-10-23 22:16:49 PDT
This bug comes from Chromium bug http://crbug.com/93585
Comment 2 yosin 2011-10-23 22:36:50 PDT
Created attachment 112149 [details]
Patch 1
Comment 3 yosin 2011-10-23 22:38:00 PDT
Created attachment 112150 [details]
Patch
Comment 4 yosin 2011-10-23 22:39:14 PDT
Add tkent@ to CC
Comment 5 Darin Adler 2011-10-23 22:42:06 PDT
Comment on attachment 112150 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112150&action=review

Thanks for tackling this.

> Source/WebCore/html/HTMLInputElement.cpp:1303
> +    updatePlaceholderVisibility(false);

This is the wrong way to do it. You also want the placeholder to go away if the value attribute is set directly. For example, if your test said this:

    document.getElementById("c1").setAttribute("value", "Default");

We need to test both cases, and we need code that works in both cases.

The way to do that is to put the code into parseMappedAttribute, in the case for valueAttr. In fact, it should go inside the !hasDirtyValue if statement, since it only needs to run if the default value hasn’t been overwritten.
Comment 6 Kent Tamura 2011-10-23 22:46:04 PDT
Comment on attachment 112150 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112150&action=review

> Source/WebCore/ChangeLog:9
> +        Tests: fast/forms/70586-expected.html
> +               fast/forms/70586.html

70586.html is meaningless.  Please name it something representing a test case.
Comment 7 yosin 2011-10-23 23:15:43 PDT
Created attachment 112154 [details]
Patch
Comment 8 Kent Tamura 2011-10-23 23:35:30 PDT
Comment on attachment 112154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112154&action=review

> LayoutTests/ChangeLog:13
> +        * fast/forms/70586-expected.html: Added. Render text input element with "Default" value.
> +        * fast/forms/70586.html: Added. Render text input element with placeholder attribute and a script to change default value of input element to "Default" from empty.

Please rename the test.  Number-only name is hard to imagine the content.
Comment 9 yosin 2011-10-24 00:04:36 PDT
Created attachment 112156 [details]
Patch
Comment 10 Kent Tamura 2011-10-24 00:06:25 PDT
Comment on attachment 112156 [details]
Patch

Looks good.  Thank you for fixing this.
Comment 11 WebKit Review Bot 2011-10-24 01:13:10 PDT
Comment on attachment 112156 [details]
Patch

Clearing flags on attachment: 112156

Committed r98222: <http://trac.webkit.org/changeset/98222>
Comment 12 WebKit Review Bot 2011-10-24 01:13:15 PDT
All reviewed patches have been landed.  Closing bug.