REGRESSION (r90971): the cursor is painted “behind” the placeholder text
authortkent@chromium.org <tkent@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 May 2012 08:23:17 +0000 (08:23 +0000)
committertkent@chromium.org <tkent@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 May 2012 08:23:17 +0000 (08:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=87155

Reviewed by Hajime Morita.

Source/WebCore:

This regression happened only on platforms on which
RenderTheme::shouldShowPlaceholderWhenFocused() returns true.

Because the order of renderers for the editable node and the placeholder
node was:
 - A renderer for the editable node
 - A renderer for the placeholder node,
The text caret was painted, then the palceholder was painted.

We should not use z-index in the built-in shadow nodes. So the patch
fixes this bug by re-ordering these renderers.

Tests: fast/forms/input-placeholder-paint-order-2.html
       fast/forms/input-placeholder-paint-order.html
       fast/forms/textarea/textarea-placeholder-paint-order-2.html
       fast/forms/textarea/textarea-placeholder-paint-order.html

* html/HTMLTextFormControlElement.cpp:
(WebCore::HTMLTextFormControlElement::fixPlaceholderRenderer):
Added. Reorder the order of renderers so that the placeholder renderer
precedes the inner text renderer.
* html/HTMLTextFormControlElement.h: Add fixPlaceholderRenderer() declaration.

* html/HTMLTextAreaElement.cpp:
(WebCore::HTMLTextAreaElement::attach): Calls fixPlaceholderRenderer().
(WebCore::HTMLTextAreaElement::updatePlaceholderText):
ditto. Also, use innerTextElement() to improvde code readability.
* html/HTMLTextAreaElement.h:
(HTMLTextAreaElement): Overrides attach().

* html/TextFieldInputType.cpp:
(WebCore::TextFieldInputType::updatePlaceholderText):
Calls fixPlaceholderRenderer().
(WebCore::TextFieldInputType::attach): ditto.
* html/TextFieldInputType.h:
(TextFieldInputType): Overrides attach().

LayoutTests:

* fast/forms/input-placeholder-paint-order-2-expected.html: Added.
* fast/forms/input-placeholder-paint-order-2.html: Added.
* fast/forms/input-placeholder-paint-order.html: Added.
* fast/forms/textarea/textarea-placeholder-paint-order-2-expected.html: Added.
* fast/forms/textarea/textarea-placeholder-paint-order-2.html: Added.
* fast/forms/textarea/textarea-placeholder-paint-order.html: Added.
* platform/chromium-mac-snowleopard/fast/forms/input-placeholder-paint-order-expected.png: Added.
* platform/chromium-mac-snowleopard/fast/forms/input-placeholder-paint-order-expected.txt: Added.
* platform/chromium-mac-snowleopard/fast/forms/textarea/textarea-placeholder-paint-order-expected.png: Added.
* platform/chromium-mac-snowleopard/fast/forms/textarea/textarea-placeholder-paint-order-expected.txt: Added.
* platform/chromium/test_expectations.txt:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@118733 268f45cc-cd09-0410-ab3c-d52691b4dbfc

19 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/forms/input-placeholder-paint-order-2-expected.html [new file with mode: 0644]
LayoutTests/fast/forms/input-placeholder-paint-order-2.html [new file with mode: 0644]
LayoutTests/fast/forms/input-placeholder-paint-order.html [new file with mode: 0644]
LayoutTests/fast/forms/textarea/textarea-placeholder-paint-order-2-expected.html [new file with mode: 0644]
LayoutTests/fast/forms/textarea/textarea-placeholder-paint-order-2.html [new file with mode: 0644]
LayoutTests/fast/forms/textarea/textarea-placeholder-paint-order.html [new file with mode: 0644]
LayoutTests/platform/chromium-mac-snowleopard/fast/forms/input-placeholder-paint-order-expected.png [new file with mode: 0644]
LayoutTests/platform/chromium-mac-snowleopard/fast/forms/input-placeholder-paint-order-expected.txt [new file with mode: 0644]
LayoutTests/platform/chromium-mac-snowleopard/fast/forms/textarea/textarea-placeholder-paint-order-expected.png [new file with mode: 0644]
LayoutTests/platform/chromium-mac-snowleopard/fast/forms/textarea/textarea-placeholder-paint-order-expected.txt [new file with mode: 0644]
LayoutTests/platform/chromium/test_expectations.txt
Source/WebCore/ChangeLog
Source/WebCore/html/HTMLTextAreaElement.cpp
Source/WebCore/html/HTMLTextAreaElement.h
Source/WebCore/html/HTMLTextFormControlElement.cpp
Source/WebCore/html/HTMLTextFormControlElement.h
Source/WebCore/html/TextFieldInputType.cpp
Source/WebCore/html/TextFieldInputType.h

index 29bf537..7c41dc8 100644 (file)
@@ -1,3 +1,22 @@
+2012-05-29  Kent Tamura  <tkent@chromium.org>
+
+        REGRESSION (r90971): the cursor is painted “behind” the placeholder text
+        https://bugs.webkit.org/show_bug.cgi?id=87155
+
+        Reviewed by Hajime Morita.
+
+        * fast/forms/input-placeholder-paint-order-2-expected.html: Added.
+        * fast/forms/input-placeholder-paint-order-2.html: Added.
+        * fast/forms/input-placeholder-paint-order.html: Added.
+        * fast/forms/textarea/textarea-placeholder-paint-order-2-expected.html: Added.
+        * fast/forms/textarea/textarea-placeholder-paint-order-2.html: Added.
+        * fast/forms/textarea/textarea-placeholder-paint-order.html: Added.
+        * platform/chromium-mac-snowleopard/fast/forms/input-placeholder-paint-order-expected.png: Added.
+        * platform/chromium-mac-snowleopard/fast/forms/input-placeholder-paint-order-expected.txt: Added.
+        * platform/chromium-mac-snowleopard/fast/forms/textarea/textarea-placeholder-paint-order-expected.png: Added.
+        * platform/chromium-mac-snowleopard/fast/forms/textarea/textarea-placeholder-paint-order-expected.txt: Added.
+        * platform/chromium/test_expectations.txt:
+
 2012-05-29  János Badics  <jbadics@inf.u-szeged.hu>
 
         [Qt] Unreviewed gardening after r118631. http/tests/appcache/load-from-appcache-defer-resume-crash.html
diff --git a/LayoutTests/fast/forms/input-placeholder-paint-order-2-expected.html b/LayoutTests/fast/forms/input-placeholder-paint-order-2-expected.html
new file mode 100644 (file)
index 0000000..9760058
--- /dev/null
@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<body>
+<style>
+input {
+    color: lime;
+    text-indent: 1px;
+    zoom: 8;
+}
+
+input::-webkit-input-placeholder {
+    text-indent: -1px;
+    font-weight: bold;
+    color: red;
+}
+</style>
+<p>This test is meaningfull only if RenderTheme::shouldShowPlaceholderWhenFocused() returns true.</p>
+<p>The green caret in the following text box should painted over the red placeholder text.</p>
+<input placeholder="Placeholder" autofocus>
+</body>
diff --git a/LayoutTests/fast/forms/input-placeholder-paint-order-2.html b/LayoutTests/fast/forms/input-placeholder-paint-order-2.html
new file mode 100644 (file)
index 0000000..06e01cf
--- /dev/null
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<body>
+<style>
+input {
+    color: lime;
+    text-indent: 1px;
+    zoom: 8;
+}
+
+input::-webkit-input-placeholder {
+    text-indent: -1px;
+    font-weight: bold;
+    color: red;
+}
+</style>
+<p>This test is meaningfull only if RenderTheme::shouldShowPlaceholderWhenFocused() returns true.</p>
+<p>The green caret in the following text box should painted over the red placeholder text.</p>
+<input autofocus>
+<script>
+document.getElementsByTagName('input')[0].placeholder = 'Placeholder';
+</script>
+</body>
diff --git a/LayoutTests/fast/forms/input-placeholder-paint-order.html b/LayoutTests/fast/forms/input-placeholder-paint-order.html
new file mode 100644 (file)
index 0000000..be5fb3e
--- /dev/null
@@ -0,0 +1,24 @@
+<!DOCTYPE html>
+<body>
+<script>
+// We don't care a render tree of this test.
+if (window.layoutTestController)
+    layoutTestController.dumpAsText(true);
+</script>
+<style>
+input {
+    color: lime;
+    text-indent: 1px;
+    zoom: 8;
+}
+
+input::-webkit-input-placeholder {
+    text-indent: -1px;
+    font-weight: bold;
+    color: red;
+}
+</style>
+<p>This test is meaningfull only if RenderTheme::shouldShowPlaceholderWhenFocused() returns true.</p>
+<p>The green caret in the following text box should painted over the red placeholder text.</p>
+<input placeholder="Placeholder" autofocus>
+</body>
diff --git a/LayoutTests/fast/forms/textarea/textarea-placeholder-paint-order-2-expected.html b/LayoutTests/fast/forms/textarea/textarea-placeholder-paint-order-2-expected.html
new file mode 100644 (file)
index 0000000..21361c7
--- /dev/null
@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<body>
+<style>
+textarea {
+    color: lime;
+    text-indent: 1px;
+    zoom: 8;
+}
+
+textarea::-webkit-input-placeholder {
+    text-indent: -1px;
+    font-weight: bold;
+    color: red;
+}
+</style>
+<p>This test is meaningfull only if RenderTheme::shouldShowPlaceholderWhenFocused() returns true.</p>
+<p>The green caret in the following text box should painted over the red placeholder text.</p>
+<textarea autofocus placeholder="Placeholder"></textarea>
+</body>
diff --git a/LayoutTests/fast/forms/textarea/textarea-placeholder-paint-order-2.html b/LayoutTests/fast/forms/textarea/textarea-placeholder-paint-order-2.html
new file mode 100644 (file)
index 0000000..3852835
--- /dev/null
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<body>
+<style>
+textarea {
+    color: lime;
+    text-indent: 1px;
+    zoom: 8;
+}
+
+textarea::-webkit-input-placeholder {
+    text-indent: -1px;
+    font-weight: bold;
+    color: red;
+}
+</style>
+<p>This test is meaningfull only if RenderTheme::shouldShowPlaceholderWhenFocused() returns true.</p>
+<p>The green caret in the following text box should painted over the red placeholder text.</p>
+<textarea autofocus></textarea>
+<script>
+document.getElementsByTagName('textarea')[0].placeholder = 'Placeholder';
+</script>
+</body>
diff --git a/LayoutTests/fast/forms/textarea/textarea-placeholder-paint-order.html b/LayoutTests/fast/forms/textarea/textarea-placeholder-paint-order.html
new file mode 100644 (file)
index 0000000..8aa6c25
--- /dev/null
@@ -0,0 +1,24 @@
+<!DOCTYPE html>
+<body>
+<script>
+// We don't care a render tree of this test.
+if (window.layoutTestController)
+    layoutTestController.dumpAsText(true);
+</script>
+<style>
+textarea {
+    color: lime;
+    text-indent: 1px;
+    zoom: 8;
+}
+
+textarea::-webkit-input-placeholder {
+    text-indent: -1px;
+    font-weight: bold;
+    color: red;
+}
+</style>
+<p>This test is meaningfull only if RenderTheme::shouldShowPlaceholderWhenFocused() returns true.</p>
+<p>The green caret in the following text box should painted over the red placeholder text.</p>
+<textarea placeholder="Placeholder" autofocus></textarea>
+</body>
diff --git a/LayoutTests/platform/chromium-mac-snowleopard/fast/forms/input-placeholder-paint-order-expected.png b/LayoutTests/platform/chromium-mac-snowleopard/fast/forms/input-placeholder-paint-order-expected.png
new file mode 100644 (file)
index 0000000..9309af3
Binary files /dev/null and b/LayoutTests/platform/chromium-mac-snowleopard/fast/forms/input-placeholder-paint-order-expected.png differ
diff --git a/LayoutTests/platform/chromium-mac-snowleopard/fast/forms/input-placeholder-paint-order-expected.txt b/LayoutTests/platform/chromium-mac-snowleopard/fast/forms/input-placeholder-paint-order-expected.txt
new file mode 100644 (file)
index 0000000..e5252fc
--- /dev/null
@@ -0,0 +1,5 @@
+This test is meaningfull only if RenderTheme::shouldShowPlaceholderWhenFocused() returns true.
+
+The green caret in the following text box should painted over the red placeholder text.
+
+
diff --git a/LayoutTests/platform/chromium-mac-snowleopard/fast/forms/textarea/textarea-placeholder-paint-order-expected.png b/LayoutTests/platform/chromium-mac-snowleopard/fast/forms/textarea/textarea-placeholder-paint-order-expected.png
new file mode 100644 (file)
index 0000000..1b0aef1
Binary files /dev/null and b/LayoutTests/platform/chromium-mac-snowleopard/fast/forms/textarea/textarea-placeholder-paint-order-expected.png differ
diff --git a/LayoutTests/platform/chromium-mac-snowleopard/fast/forms/textarea/textarea-placeholder-paint-order-expected.txt b/LayoutTests/platform/chromium-mac-snowleopard/fast/forms/textarea/textarea-placeholder-paint-order-expected.txt
new file mode 100644 (file)
index 0000000..e5252fc
--- /dev/null
@@ -0,0 +1,5 @@
+This test is meaningfull only if RenderTheme::shouldShowPlaceholderWhenFocused() returns true.
+
+The green caret in the following text box should painted over the red placeholder text.
+
+
index ca26721..5ace83f 100644 (file)
@@ -3436,6 +3436,10 @@ BUGWK82122 LEOPARD SNOWLEOPARD : css3/selectors3/xml/css3-modsel-d4.xml = FAIL P
 BUGWK80531 WIN : fast/table/colspanMinWidth-vertical.html = IMAGE+TEXT
 BUGWK80531 MAC : fast/table/colspanMinWidth-vertical.html = IMAGE+TEXT
 
+// New tests.  Need expectations.
+BUGWK87155 : fast/forms/input-placeholder-paint-order.html = MISSING FAIL
+BUGWK87155 : fast/forms/textarea/textarea-placeholder-paint-order.html = MISSING FAIL
+
 // Flaky/crashing on a single platform every 30 tries or so
 BUGWK82230 LINUX DEBUG : http/tests/cache/post-redirect-get.php = PASS CRASH
 BUGWK82231 WIN DEBUG : fast/canvas/webgl/program-test.html = PASS CRASH
index 17e9739..c051635 100644 (file)
@@ -1,3 +1,47 @@
+2012-05-29  Kent Tamura  <tkent@chromium.org>
+
+        REGRESSION (r90971): the cursor is painted “behind” the placeholder text
+        https://bugs.webkit.org/show_bug.cgi?id=87155
+
+        Reviewed by Hajime Morita.
+
+        This regression happened only on platforms on which
+        RenderTheme::shouldShowPlaceholderWhenFocused() returns true.
+
+        Because the order of renderers for the editable node and the placeholder
+        node was:
+         - A renderer for the editable node
+         - A renderer for the placeholder node,
+        The text caret was painted, then the palceholder was painted.
+
+        We should not use z-index in the built-in shadow nodes. So the patch
+        fixes this bug by re-ordering these renderers.
+
+        Tests: fast/forms/input-placeholder-paint-order-2.html
+               fast/forms/input-placeholder-paint-order.html
+               fast/forms/textarea/textarea-placeholder-paint-order-2.html
+               fast/forms/textarea/textarea-placeholder-paint-order.html
+
+        * html/HTMLTextFormControlElement.cpp:
+        (WebCore::HTMLTextFormControlElement::fixPlaceholderRenderer):
+        Added. Reorder the order of renderers so that the placeholder renderer
+        precedes the inner text renderer.
+        * html/HTMLTextFormControlElement.h: Add fixPlaceholderRenderer() declaration.
+
+        * html/HTMLTextAreaElement.cpp:
+        (WebCore::HTMLTextAreaElement::attach): Calls fixPlaceholderRenderer().
+        (WebCore::HTMLTextAreaElement::updatePlaceholderText):
+        ditto. Also, use innerTextElement() to improvde code readability.
+        * html/HTMLTextAreaElement.h:
+        (HTMLTextAreaElement): Overrides attach().
+
+        * html/TextFieldInputType.cpp:
+        (WebCore::TextFieldInputType::updatePlaceholderText):
+        Calls fixPlaceholderRenderer().
+        (WebCore::TextFieldInputType::attach): ditto.
+        * html/TextFieldInputType.h:
+        (TextFieldInputType): Overrides attach().
+
 2012-05-28  Kentaro Hara  <haraken@chromium.org>
 
         [V8] Implement V8Binding::v8Null(isolate) and use it in CodeGeneratorV8.pm
index bc44e7b..6437257 100644 (file)
@@ -496,6 +496,12 @@ HTMLElement* HTMLTextAreaElement::placeholderElement() const
     return m_placeholder.get();
 }
 
+void HTMLTextAreaElement::attach()
+{
+    HTMLTextFormControlElement::attach();
+    fixPlaceholderRenderer(m_placeholder.get(), innerTextElement());
+}
+
 void HTMLTextAreaElement::updatePlaceholderText()
 {
     ExceptionCode ec = 0;
@@ -511,11 +517,12 @@ void HTMLTextAreaElement::updatePlaceholderText()
     if (!m_placeholder) {
         m_placeholder = HTMLDivElement::create(document());
         m_placeholder->setShadowPseudoId("-webkit-input-placeholder");
-        shadow()->oldestShadowRoot()->insertBefore(m_placeholder, shadow()->oldestShadowRoot()->firstChild()->nextSibling(), ec);
+        shadow()->oldestShadowRoot()->insertBefore(m_placeholder, innerTextElement()->nextSibling(), ec);
         ASSERT(!ec);
     }
     m_placeholder->setInnerText(placeholderText, ec);
     ASSERT(!ec);
+    fixPlaceholderRenderer(m_placeholder.get(), innerTextElement());
 }
 
 }
index c475c71..63ce9ea 100644 (file)
@@ -109,6 +109,7 @@ private:
     virtual void accessKeyAction(bool sendMouseEvents);
 
     virtual bool shouldUseInputMethod();
+    virtual void attach() OVERRIDE;
 
     bool valueMissing(const String& value) const { return isRequiredFormControl() && !disabled() && !readOnly() && value.isEmpty(); }
     bool tooLong(const String&, NeedsToCheckDirtyFlag) const;
index e00f1d4..9092de7 100644 (file)
@@ -166,6 +166,23 @@ void HTMLTextFormControlElement::updatePlaceholderVisibility(bool placeholderVal
     ASSERT(!ec);
 }
 
+void HTMLTextFormControlElement::fixPlaceholderRenderer(HTMLElement* placeholder, HTMLElement* siblingElement)
+{
+    // FIXME: We should change the order of DOM nodes. But it makes an assertion
+    // failure in editing code.
+    if (!placeholder || !placeholder->renderer())
+        return;
+    RenderObject* placeholderRenderer = placeholder->renderer();
+    RenderObject* siblingRenderer = siblingElement->renderer();
+    ASSERT(siblingRenderer);
+    if (placeholderRenderer->nextSibling() == siblingRenderer)
+        return;
+    RenderObject* parentRenderer = placeholderRenderer->parent();
+    ASSERT(parentRenderer == siblingRenderer->parent());
+    parentRenderer->removeChild(placeholderRenderer);
+    parentRenderer->addChild(placeholderRenderer, siblingRenderer);
+}
+
 RenderTextControl* HTMLTextFormControlElement::textRendererAfterUpdateLayout()
 {
     if (!isTextFormControl())
index 3fee3c4..8ebe66a 100644 (file)
@@ -54,6 +54,7 @@ public:
     bool placeholderShouldBeVisible() const;
     virtual HTMLElement* placeholderElement() const = 0;
     void updatePlaceholderVisibility(bool);
+    static void fixPlaceholderRenderer(HTMLElement* placeholder, HTMLElement* siblingElement);
 
     int indexForVisiblePosition(const VisiblePosition&) const;
     int selectionStart() const;
index 9a3b25a..8189144 100644 (file)
@@ -410,6 +410,13 @@ void TextFieldInputType::updatePlaceholderText()
     }
     m_placeholder->setInnerText(placeholderText, ec);
     ASSERT(!ec);
+    element()->fixPlaceholderRenderer(m_placeholder.get(), m_container ? m_container.get() : m_innerText.get());
+}
+
+void TextFieldInputType::attach()
+{
+    InputType::attach();
+    element()->fixPlaceholderRenderer(m_placeholder.get(), m_container ? m_container.get() : m_innerText.get());
 }
 
 bool TextFieldInputType::appendFormData(FormDataList& list, bool multipart) const
index 26b436e..559be25 100644 (file)
@@ -80,6 +80,7 @@ private:
     virtual HTMLElement* placeholderElement() const OVERRIDE;
     virtual void updatePlaceholderText() OVERRIDE;
     virtual bool appendFormData(FormDataList&, bool multipart) const OVERRIDE;
+    virtual void attach() OVERRIDE;
 
     RefPtr<HTMLElement> m_container;
     RefPtr<HTMLElement> m_innerBlock;