input's placeholder is inconsistently laid out
The issue was that the code relies on overriding the placeholder's
style's logical height. However the logic wasn't tight and we would
miss some cases (where styleDidChange cleared the overriden style
and layout would not know about it), thus not relaying out the
placeholder when we should have. This situation turned layout
into a 2-states state machine.
This fix removes the hackish style's logical height override and
use the override logical height machinery for fun and profit.
BUG=529252
Review URL: https://codereview.chromium.org/1499063002
Cr-Commit-Position: refs/heads/master@{#363324}
diff --git a/third_party/WebKit/LayoutTests/TestExpectations b/third_party/WebKit/LayoutTests/TestExpectations
index 4238e835..91a45e9 100644
--- a/third_party/WebKit/LayoutTests/TestExpectations
+++ b/third_party/WebKit/LayoutTests/TestExpectations
@@ -772,6 +772,13 @@
crbug.com/396941 ietestcenter/css3/multicolumn/column-width-applies-to-015.htm [ Failure ]
crbug.com/378610 svg/custom/focus-ring-text.svg [ Failure ]
+crbug.com/529252 fast/repaint/change-text-content-and-background-color.html [ NeedsRebaseline ]
+crbug.com/529252 fast/repaint/multi-layout-one-frame.html [ NeedsRebaseline ]
+crbug.com/529252 fast/repaint/subtree-root-skipped.html [ NeedsRebaseline ]
+crbug.com/529252 virtual/syncpaint/fast/repaint/change-text-content-and-background-color.html [ NeedsRebaseline ]
+crbug.com/529252 virtual/syncpaint/fast/repaint/multi-layout-one-frame.html [ NeedsRebaseline ]
+crbug.com/529252 virtual/syncpaint/fast/repaint/subtree-root-skipped.html [ NeedsRebaseline ]
+
crbug.com/515454 [ Mac Win7 Win10 Android Linux ] css3/fonts/font-style-matching.html [ Slow ]
crbug.com/523021 [ XP ] css3/fonts/font-style-matching.html [ Failure Slow ]
diff --git a/third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall-expected.txt b/third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall-expected.txt
new file mode 100644
index 0000000..43431cf
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall-expected.txt
@@ -0,0 +1,7 @@
+Both inputs should be of the same size and the placeholder should be at the same place.
+PASS rect0.top is rect1.top
+PASS rect0.height is rect1.height
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall.html b/third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall.html
new file mode 100644
index 0000000..523f5691
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall.html
@@ -0,0 +1,35 @@
+<!DOCTYPE html>
+<style>
+input {
+ height: 26px;
+ line-height: 29px;
+ padding: 0 5px;
+ border: none;
+ border-bottom: 1px solid #888;
+ outline: none;
+}
+</style>
+<div>Both inputs should be of the same size and the placeholder should be at the same place.</div>
+<div id="console"></div>
+<input type="text" placeholder="placeholder">
+<input type="text" placeholder="placeholder">
+<script>
+window.jsTestIsAsync = true;
+
+var inputs = document.getElementsByTagName("input");
+
+function checkInputs()
+{
+ rect0 = inputs[0].getBoundingClientRect();
+ rect1 = inputs[1].getBoundingClientRect();
+ shouldBe("rect0.top", "rect1.top");
+ shouldBe("rect0.height", "rect1.height");
+
+ finishJSTest();
+}
+
+inputs[1].focus();
+// Forcing a layout in this frame makes the issue disappear.
+window.requestAnimationFrame(checkInputs);
+</script>
+<script src="../../resources/js-test.js"></script>
diff --git a/third_party/WebKit/LayoutTests/platform/linux/fast/repaint/change-text-content-and-background-color-expected.txt b/third_party/WebKit/LayoutTests/platform/linux/fast/repaint/change-text-content-and-background-color-expected.txt
index fd98e29..99e65e6e 100644
--- a/third_party/WebKit/LayoutTests/platform/linux/fast/repaint/change-text-content-and-background-color-expected.txt
+++ b/third_party/WebKit/LayoutTests/platform/linux/fast/repaint/change-text-content-and-background-color-expected.txt
@@ -6,14 +6,14 @@
"contentsOpaque": true,
"drawsContent": true,
"repaintRects": [
- [30, 30, 200, 23],
+ [30, 30, 200, 24],
[30, 30, 45, 23],
[30, 30, 41, 23],
[8, 8, 244, 68]
],
"paintInvalidationClients": [
- "InlineTextBox ''",
"RootInlineBox",
+ "InlineTextBox ''",
"LayoutTextControl (positioned) INPUT id='input'",
"LayoutBlockFlow DIV id='inner-editor'",
"LayoutText #text",
diff --git a/third_party/WebKit/LayoutTests/platform/linux/fast/repaint/multi-layout-one-frame-expected.txt b/third_party/WebKit/LayoutTests/platform/linux/fast/repaint/multi-layout-one-frame-expected.txt
index 346aab1..53a8e030 100644
--- a/third_party/WebKit/LayoutTests/platform/linux/fast/repaint/multi-layout-one-frame-expected.txt
+++ b/third_party/WebKit/LayoutTests/platform/linux/fast/repaint/multi-layout-one-frame-expected.txt
@@ -14,10 +14,10 @@
[10, 11, 45, 16]
],
"paintInvalidationClients": [
- "InlineTextBox ''",
"RootInlineBox",
"InlineTextBox ''",
"RootInlineBox",
+ "InlineTextBox ''",
"LayoutBlockFlow DIV id='inner-editor'",
"LayoutText #text",
"InlineTextBox 'PASSED'",
diff --git a/third_party/WebKit/LayoutTests/platform/linux/fast/repaint/subtree-root-skipped-expected.txt b/third_party/WebKit/LayoutTests/platform/linux/fast/repaint/subtree-root-skipped-expected.txt
index d3ffcb7..cf76b15 100644
--- a/third_party/WebKit/LayoutTests/platform/linux/fast/repaint/subtree-root-skipped-expected.txt
+++ b/third_party/WebKit/LayoutTests/platform/linux/fast/repaint/subtree-root-skipped-expected.txt
@@ -13,8 +13,8 @@
[8, 288, 10, 20]
],
"paintInvalidationClients": [
- "InlineTextBox ''",
"RootInlineBox",
+ "InlineTextBox ''",
"LayoutBlockFlow DIV id='inner-editor'",
"LayoutText #text",
"InlineTextBox 'PASS'",
diff --git a/third_party/WebKit/Source/core/layout/LayoutBox.cpp b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
index 029dc87c..4a51e69 100644
--- a/third_party/WebKit/Source/core/layout/LayoutBox.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
@@ -2362,7 +2362,7 @@
// grab our cached flexible height.
// FIXME: Account for writing-mode in flexible boxes.
// https://bugs.webkit.org/show_bug.cgi?id=46418
- if (hasOverrideLogicalContentHeight() && (parent()->isFlexibleBoxIncludingDeprecated() || parent()->isLayoutGrid())) {
+ if (hasOverrideLogicalContentHeight()) {
LayoutUnit contentHeight = overrideLogicalContentHeight();
if (parent()->isLayoutGrid() && style()->logicalMinHeight().isAuto() && style()->overflowY() == OVISIBLE) {
ASSERT(style()->logicalHeight().isAuto());
diff --git a/third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp b/third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp
index 73bc95d9..d23c798 100644
--- a/third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp
@@ -121,19 +121,26 @@
LayoutBox* viewPortLayoutObject = editingViewPortElement() ? editingViewPortElement()->layoutBox() : 0;
// To ensure consistency between layouts, we need to reset any conditionally overriden height.
- if (innerEditorLayoutObject && !innerEditorLayoutObject->styleRef().logicalHeight().isAuto()) {
- innerEditorLayoutObject->mutableStyleRef().setLogicalHeight(Length(Auto));
+ if (innerEditorLayoutObject) {
+ innerEditorLayoutObject->clearOverrideLogicalContentHeight();
+ // TODO(jchaffraix): We could probably skip some of these due to
+ // forcing children relayout below but keeping them for safety for now.
layoutScope.setNeedsLayout(innerEditorLayoutObject, LayoutInvalidationReason::TextControlChanged);
HTMLElement* placeholderElement = inputElement()->placeholderElement();
if (LayoutBox* placeholderBox = placeholderElement ? placeholderElement->layoutBox() : 0)
layoutScope.setNeedsLayout(placeholderBox, LayoutInvalidationReason::TextControlChanged);
}
+ // TODO(jchaffraix): This logic is not correct and will yield to bugs such
+ // as crbug.com/529252. The fix is similar to what is done with
+ // innerEditorLayoutObject above.
if (viewPortLayoutObject && !viewPortLayoutObject->styleRef().logicalHeight().isAuto()) {
viewPortLayoutObject->mutableStyleRef().setLogicalHeight(Length(Auto));
layoutScope.setNeedsLayout(viewPortLayoutObject, LayoutInvalidationReason::TextControlChanged);
}
- LayoutBlockFlow::layoutBlock(false);
+ // This is the measuring phase. Thus we force children to be relayout so
+ // that the checks below are executed consistently.
+ LayoutBlockFlow::layoutBlock(true);
Element* container = containerElement();
LayoutBox* containerLayoutObject = container ? container->layoutBox() : 0;
@@ -147,7 +154,7 @@
m_desiredInnerEditorLogicalHeight = desiredLogicalHeight;
- innerEditorLayoutObject->mutableStyleRef().setLogicalHeight(Length(desiredLogicalHeight, Fixed));
+ innerEditorLayoutObject->setOverrideLogicalContentHeight(desiredLogicalHeight);
layoutScope.setNeedsLayout(innerEditorLayoutObject, LayoutInvalidationReason::TextControlChanged);
if (viewPortLayoutObject) {
viewPortLayoutObject->mutableStyleRef().setLogicalHeight(Length(desiredLogicalHeight, Fixed));
@@ -252,9 +259,6 @@
containerLayoutObject->mutableStyleRef().setHeight(Length());
containerLayoutObject->mutableStyleRef().setWidth(Length());
}
- LayoutObject* innerEditorLayoutObject = innerEditorElement()->layoutObject();
- if (innerEditorLayoutObject && diff.needsFullLayout())
- innerEditorLayoutObject->setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::StyleChange);
if (HTMLElement* placeholder = inputElement()->placeholderElement())
placeholder->setInlineStyleProperty(CSSPropertyTextOverflow, textShouldBeTruncated() ? CSSValueEllipsis : CSSValueClip);
setHasOverflowClip(false);
