Bug 64253 - Implement text field placeholders using shadow DOM
Summary: Implement text field placeholders using shadow DOM
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Major
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks: 51290 54797 54814 63367 67408
  Show dependency treegraph
 
Reported: 2011-07-11 00:26 PDT by Kent Tamura
Modified: 2012-10-29 23:59 PDT (History)
5 users (show)

See Also:


Attachments
WIP (129.53 KB, patch)
2011-07-11 02:47 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch (351.09 KB, patch)
2011-07-12 02:50 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (351.09 KB, patch)
2011-07-12 03:00 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.41 MB, application/zip)
2011-07-12 03:46 PDT, WebKit Review Bot
no flags Details
Patch 3 (387.33 KB, patch)
2011-07-12 03:54 PDT, Kent Tamura
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2011-07-11 00:26:22 PDT
In order to fix multiple placeholder regressions, adds new shadow elements (and text nodes) representing placeholders.
Comment 1 Kent Tamura 2011-07-11 02:47:23 PDT
Created attachment 100258 [details]
WIP
Comment 2 Kent Tamura 2011-07-11 02:49:48 PDT
(In reply to comment #1)
> Created an attachment (id=100258) [details]
> WIP

The patch still has a problem:
If a placeholder box for a textarea is higher than the textarea, a horizontal scrollbar is shown.
Comment 3 Dimitri Glazkov (Google) 2011-07-11 10:28:25 PDT
I tweaked the title a bit, sorry :)
Comment 4 Dimitri Glazkov (Google) 2011-07-11 10:30:14 PDT
Comment on attachment 100258 [details]
WIP

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

Looking forward to the final patch!

> Source/WebCore/rendering/RenderTextControl.cpp:143
> +    if (RenderStyle* pseudoStyle = getCachedPseudoStyle(INPUT_PLACEHOLDER))
> +        style = RenderStyle::clone(pseudoStyle);
> +    else {
> +        style = RenderStyle::create();
> +        style->inheritFrom(startStyle);
> +    }
> +    style->setTextSecurity(TSNONE);
> +    style->setPosition(RelativePosition);
> +    style->setDisplay(BLOCK);
> +    style->setVisibility(m_placeholderVisible ? VISIBLE : HIDDEN);
> +    style->setPointerEvents(PE_NONE);
> +    return style.release();

Should use shadowPseudoId here.
Comment 5 Kent Tamura 2011-07-12 02:50:14 PDT
Created attachment 100457 [details]
Patch
Comment 6 WebKit Review Bot 2011-07-12 02:52:58 PDT
Attachment 100457 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

ERROR: FAILURES FOR <, x86, release, cpu>
ERROR: Line:6 Path does not exist. fast/form/textarea-placeholder-wrapping.html
LayoutTests/platform/gtk/test_expectations.txt:6:  Path does not exist. fast/form/textarea-placeholder-wrapping.html  [test/expectations] [2]
ERROR: FAILURES FOR <, x86, release, cpu>
ERROR: Line:6 Path does not exist. fast/form/textarea-placeholder-wrapping.html
LayoutTests/platform/qt/test_expectations.txt:6:  Path does not exist. fast/form/textarea-placeholder-wrapping.html  [test/expectations] [2]
ERROR: FAILURES FOR <lucid, x86_64, release, cpu>
ERROR: Line:3788 Path does not exist. fast/form/textarea-placeholder-wrapping.html
LayoutTests/platform/chromium/test_expectations.txt:3788:  Path does not exist. fast/form/textarea-placeholder-wrapping.html  [test/expectations] [2]
Total errors found: 3 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Kent Tamura 2011-07-12 03:00:39 PDT
Created attachment 100459 [details]
Patch 2

Fix text_expectations.txt
Comment 8 WebKit Review Bot 2011-07-12 03:46:35 PDT
Comment on attachment 100459 [details]
Patch 2

Attachment 100459 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9016115

New failing tests:
http/tests/misc/slow-loading-mask.html
fast/blockflow/japanese-rl-selection.html
svg/transforms/text-with-mask-with-svg-transform.svg
fast/backgrounds/background-leakage.html
fast/box-shadow/scaled-box-shadow.html
fast/backgrounds/repeat/negative-offset-repeat.html
fast/css/pseudo-cache-stale.html
svg/W3C-SVG-1.1/struct-use-01-t.svg
transforms/2d/hindi-rotated.html
svg/repaint/filter-repaint.svg
fast/blockflow/japanese-lr-selection.html
Comment 9 WebKit Review Bot 2011-07-12 03:46:39 PDT
Created attachment 100460 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 10 Kent Tamura 2011-07-12 03:54:16 PDT
Created attachment 100463 [details]
Patch 3

Update for fast/css/pseudo-cache-stale.html
Comment 11 Dominic Cooney 2011-07-12 04:35:47 PDT
Comment on attachment 100463 [details]
Patch 3

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

This is very cool.

The placeholder text in the last textbox in each group in the input-placeholder-text-indent has moved significantly… is that expected?

Why do the pixel results for isindex-placeholder and password-placeholder-text-security need to be updated? Eyeballing them they are the same.

> LayoutTests/fast/forms/placeholder-position.html:16
> +<input style="line-height:25px" value="Value">

For consistency with above maybe:
- put a space after :
- end with a semicolon
- quote the placeholder attribute
- put the placeholder attribute before the inline style

Below too.
Comment 12 Kent Tamura 2011-07-12 06:55:00 PDT
(In reply to comment #11)
> The placeholder text in the last textbox in each group in the input-placeholder-text-indent has moved significantly… is that expected?

It's expected. The placeholders with this patch correctly respect text-indent:50%.

> Why do the pixel results for isindex-placeholder and password-placeholder-text-security need to be updated? Eyeballing them they are the same.

The difference is that the bottom 1px line of a placeholder is not drawn in the updated images. It's because the current placeholder rendering doesn't clip characters by the content-box and the patched placeholder rendering does.
Comment 13 Dimitri Glazkov (Google) 2011-07-12 08:54:14 PDT
Comment on attachment 100463 [details]
Patch 3

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

> Source/WebCore/rendering/RenderTextControlMultiLine.h:52
> +    virtual RenderObject* layoutSpecialExcludedChild(bool relayoutChildren);

Whoa, this is crazy stuff. I didn't realize this mechanism existed.
Comment 14 Kent Tamura 2011-07-13 19:17:09 PDT
Committed r90971: <http://trac.webkit.org/changeset/90971>
Comment 15 Alexey Proskuryakov 2012-05-22 12:07:25 PDT
This has caused bug 87155.