input[type=text]: Add a workaround to fix vertical position of Noto Sans CJK 1.004. (issue 1387783003 by tkent@chromium.org)

0 views
Skip to first unread message

tk...@chromium.org

unread,
Oct 5, 2015, 1:47:35 AM10/5/15
to ko...@chromium.org, asa...@chromium.org, benjhayd...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
Reviewers: kojii,

Message:
Koji-san, can you review this?


Description:
input[type=text]: Add a workaround to fix vertical position of Noto Sans CJK
1.004.

If an input element has the following style,
- line-height: Npx
- FontMetrics::height(): M (where M > N)

|line-height| for innerEditor was removed, and the innerEditor
and the input content box were aligned at the top edge. So, the
bottom part of the text was clipped if |height| is specified and
it's smaller than FontMetrics::height().

This behavior is required because all browsers ignore
|line-height| if |height| is not specified.

Typically an error of author style causes this
situation. However, with Noto Sans 1.004, FontMetrics::height()
is larger than font-size, and the bottom part of text was clipped
even if <input style="font-size:16px; line-height:16px;
height:16px;">. Authors don't expect this behavior.

This CL updates the code so that we don't remove |line-height|
from innerEditor if |height| is specified. This centers
innerEditor vertically.

BUG=519331

Please review this at https://codereview.chromium.org/1387783003/

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+28, -0 lines):
M third_party/WebKit/LayoutTests/TestExpectations
A
third_party/WebKit/LayoutTests/fast/forms/text/text-font-height-mismatch.html
A +
third_party/WebKit/LayoutTests/platform/mac/fast/forms/text/text-font-height-mismatch-expected.png
A
third_party/WebKit/LayoutTests/platform/mac/fast/forms/text/text-font-height-mismatch-expected.txt
M third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp


Index: third_party/WebKit/LayoutTests/TestExpectations
diff --git a/third_party/WebKit/LayoutTests/TestExpectations
b/third_party/WebKit/LayoutTests/TestExpectations
index
07f799fb11f3624c04a47f640b67ce55329742b4..640f9f0d257796daa9bc51f4b5062b576d4c43e3
100644
--- a/third_party/WebKit/LayoutTests/TestExpectations
+++ b/third_party/WebKit/LayoutTests/TestExpectations
@@ -67,6 +67,7 @@ crbug.com/463358 [ Mac Linux Debug ]
svg/W3C-SVG-1.1/paths-data-02-t.svg [ Image
crbug.com/463358 [ Mac Linux Debug ] css3/masking/clip-path-polygon.html [
ImageOnlyFailure ]

crbug.com/267206 [ Mac ]
virtual/rootlayerscrolls/fast/scrolling/scrollbar-tickmarks-hittest.html [
Timeout ]
+crbug.com/519331 fast/forms/text/text-font-height-mismatch.html [
NeedsRebaseline ]

crbug.com/280342 [ Linux Win ]
http/tests/media/progress-events-generated-correctly.html [ Failure ]

Index:
third_party/WebKit/LayoutTests/fast/forms/text/text-font-height-mismatch.html
diff --git
a/third_party/WebKit/LayoutTests/fast/forms/text/text-font-height-mismatch.html
b/third_party/WebKit/LayoutTests/fast/forms/text/text-font-height-mismatch.html
new file mode 100644
index
0000000000000000000000000000000000000000..a8133400ecd2d9c7fb40a71cd177c27451259617
--- /dev/null
+++
b/third_party/WebKit/LayoutTests/fast/forms/text/text-font-height-mismatch.html
@@ -0,0 +1,4 @@
+<!DOCTYPE html>
+<p>Editable text should be centered vertically.<p>
+<input style="border: solid 1px black; line-height:16px; height:16px;
font-size:24px;" value="ABCgjy"></div>
+<input style="border: solid 1px black; height:32px; font-size:16px; "
value="ABCgjy"></div>
Index:
third_party/WebKit/LayoutTests/platform/mac/fast/forms/text/text-font-height-mismatch-expected.png
diff --git
a/third_party/WebKit/LayoutTests/platform/linux/tables/mozilla/bugs/bug53690-1-expected.png
b/third_party/WebKit/LayoutTests/platform/mac/fast/forms/text/text-font-height-mismatch-expected.png
similarity index 59%
copy from
third_party/WebKit/LayoutTests/platform/linux/tables/mozilla/bugs/bug53690-1-expected.png
copy to
third_party/WebKit/LayoutTests/platform/mac/fast/forms/text/text-font-height-mismatch-expected.png
index
1c044860b8848528715c5f57f237c84693ca55a6..6eeda2a0c89954e90dd312442c64e01f7d0977a9
100644
Binary files
a/third_party/WebKit/LayoutTests/platform/linux/tables/mozilla/bugs/bug53690-1-expected.png
and
b/third_party/WebKit/LayoutTests/platform/mac/fast/forms/text/text-font-height-mismatch-expected.png
differ
Index:
third_party/WebKit/LayoutTests/platform/mac/fast/forms/text/text-font-height-mismatch-expected.txt
diff --git
a/third_party/WebKit/LayoutTests/platform/mac/fast/forms/text/text-font-height-mismatch-expected.txt
b/third_party/WebKit/LayoutTests/platform/mac/fast/forms/text/text-font-height-mismatch-expected.txt
new file mode 100644
index
0000000000000000000000000000000000000000..b0fddf86ca2170183e873fd7351685d69461e733
--- /dev/null
+++
b/third_party/WebKit/LayoutTests/platform/mac/fast/forms/text/text-font-height-mismatch-expected.txt
@@ -0,0 +1,22 @@
+layer at (0,0) size 800x600
+ LayoutView at (0,0) size 800x600
+layer at (0,0) size 800x600
+ LayoutBlockFlow {HTML} at (0,0) size 800x600
+ LayoutBlockFlow {BODY} at (8,8) size 784x576
+ LayoutBlockFlow {P} at (0,0) size 784x18
+ LayoutText {#text} at (0,0) size 274x18
+ text run at (0,0) width 274: "Editable text should be centered
vertically."
+ LayoutBlockFlow {P} at (0,34) size 784x32
+ LayoutTextControl {INPUT} at (0,5) size 274x16 [bgcolor=#FFFFFF]
[border: (1px solid #000000)]
+ LayoutText {#text} at (274,8) size 4x18
+ text run at (274,8) width 4: " "
+ LayoutTextControl {INPUT} at (278,0) size 171x32 [bgcolor=#FFFFFF]
[border: (1px solid #000000)]
+ LayoutText {#text} at (0,0) size 0x0
+layer at (10,47) size 270x16 scrollHeight 22
+ LayoutBlockFlow {DIV} at (2,0) size 270x16
+ LayoutText {#text} at (0,-6) size 82x28
+ text run at (0,-6) width 82: "ABCgjy"
+layer at (288,49) size 167x18
+ LayoutBlockFlow {DIV} at (2,7) size 167x18
+ LayoutText {#text} at (0,0) size 55x18
+ text run at (0,0) width 55: "ABCgjy"
Index: third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp
diff --git
a/third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp
b/third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp
index
b6ce8e2460743a1b824206b091d30e72d05b81ad..8c62e0ee232afc526df56380b916c92fc4fb7c64
100644
--- a/third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutTextControlSingleLine.cpp
@@ -365,8 +365,9 @@ PassRefPtr<ComputedStyle>
LayoutTextControlSingleLine::createInnerEditorStyle(co

if (m_desiredInnerEditorLogicalHeight >= 0)

textBlockStyle->setLogicalHeight(Length(m_desiredInnerEditorLogicalHeight,
Fixed));
+
// Do not allow line-height to be smaller than our default.
- if (textBlockStyle->fontMetrics().lineSpacing() > lineHeight(true,
HorizontalLine, PositionOfInteriorLineBoxes))
+ if (textBlockStyle->fontMetrics().lineSpacing() > lineHeight(true,
HorizontalLine, PositionOfInteriorLineBoxes) &&
startStyle.height().isIntrinsicOrAuto())
textBlockStyle->setLineHeight(ComputedStyle::initialLineHeight());

textBlockStyle->setDisplay(BLOCK);


ko...@chromium.org

unread,
Oct 5, 2015, 4:30:25 AM10/5/15
to tk...@chromium.org, asa...@chromium.org, benjhayd...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
lgtm



https://codereview.chromium.org/1387783003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 5, 2015, 6:15:43 AM10/5/15
to tk...@chromium.org, ko...@chromium.org, commi...@chromium.org, asa...@chromium.org, benjhayd...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1387783003/40001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1387783003/40001


https://codereview.chromium.org/1387783003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 5, 2015, 6:20:19 AM10/5/15
to tk...@chromium.org, ko...@chromium.org, commi...@chromium.org, asa...@chromium.org, benjhayd...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
Committed patchset #1 (id:40001)

https://codereview.chromium.org/1387783003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 5, 2015, 6:21:49 AM10/5/15
to tk...@chromium.org, ko...@chromium.org, commi...@chromium.org, asa...@chromium.org, benjhayd...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
Patchset 1 (id:??) landed as
https://crrev.com/bfa2f33f86fd947ce27dafbde21b3557b6d990fc
Cr-Commit-Position: refs/heads/master@{#352313}

https://codereview.chromium.org/1387783003/

tk...@chromium.org

unread,
Oct 6, 2015, 7:56:43 PM10/6/15
to ko...@chromium.org, asa...@chromium.org, benjhayd...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org
A revert of this CL (patchset #1 id:40001) has been created in
https://codereview.chromium.org/1386343002/ by tk...@chromium.org.

The reason for reverting is: Caused a regression on github,
crbug.com/539858.

https://codereview.chromium.org/1387783003/
Reply all
Reply to author
Forward
0 new messages