input's placeholder is inconsistently laid out

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
Committed: https://crrev.com/2036c7d6d5438beefe656aa67d1d90af166f0960
Cr-Commit-Position: refs/heads/master@{#363324}

[Julien - ping for review, 2015-12-04 04:19:03]

jchaffraix@chromium.org changed reviewers:
+ leviw@chromium.org, mstensho@opera.com

[Julien - ping for review, 2015-12-04 04:19:04]

+Morten, as the master of 28-letters classes :)
+Levi, because nobody expects him!

[mstensho (USE GERRIT), 2015-12-04 12:55:54]

lgtm, but the test has to do some time in prison.

https://codereview.chromium.org/1499063002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall.html
(right):

https://codereview.chromium.org/1499063002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall.html:14:
<input type="text" placeholder="placholder" data-offset-y="8">
Who is Plach, and why is he older?

https://codereview.chromium.org/1499063002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall.html:16:
<script src="../../resources/check-layout.js"></script>
There are attributes in this document that check-layout.js would recognize, but
checkLayout() isn't called, so it's just a waste.

I suspect that you changed your mind at some point, and ended up with
checkInputs() instead.

https://codereview.chromium.org/1499063002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall.html:37:
log("PASSED, same top");
Couldn't you just use the shouldBe() thing in js-test.js instead?

https://codereview.chromium.org/1499063002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall.html:53:
// Forcing a layout in this frame makes the issue disappear.
Scary. But I guess there's no need to explain this weirdness, since you just
fixed it, right?

[Julien - ping for review, 2015-12-04 14:45:41]

The CQ bit was checked by jchaffraix@chromium.org

[Julien - ping for review, 2015-12-04 14:45:42]

Thanks Morten, I added some extra test expectations that were needed for Windows
and Mac (the diffs are similar to the ones I have already)

https://codereview.chromium.org/1499063002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall.html
(right):

https://codereview.chromium.org/1499063002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall.html:14:
<input type="text" placeholder="placholder" data-offset-y="8">
On 2015/12/04 at 12:55:54, mstensho wrote:
> Who is Plach, and why is he older?

http://theinfosphere.org/Claw-Plach !!!

https://codereview.chromium.org/1499063002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall.html:16:
<script src="../../resources/check-layout.js"></script>
On 2015/12/04 at 12:55:54, mstensho wrote:
> There are attributes in this document that check-layout.js would recognize,
but checkLayout() isn't called, so it's just a waste.
> 
> I suspect that you changed your mind at some point, and ended up with
checkInputs() instead.

Doh', you're right. I tried to make it a checkLayout test but checkLayout
removes the bug in a manner I couldn't understand through code inspection.

https://codereview.chromium.org/1499063002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall.html:37:
log("PASSED, same top");
On 2015/12/04 at 12:55:54, mstensho wrote:
> Couldn't you just use the shouldBe() thing in js-test.js instead?

Done!

https://codereview.chromium.org/1499063002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall.html:53:
// Forcing a layout in this frame makes the issue disappear.
On 2015/12/04 at 12:55:54, mstensho wrote:
> Scary. But I guess there's no need to explain this weirdness, since you just
fixed it, right?

It's more of a warning to reviewer. I still think it's important to mention this
weirdness.

[Julien - ping for review, 2015-12-04 14:45:43]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/1499063002/#ps40001
(title: "Added some extra NeedsRebaseline test expectations.")

[commit-bot: I haz the power, 2015-12-04 14:46:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1499063002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1499063002/40001

[mstensho (USE GERRIT), 2015-12-04 14:57:36]

https://codereview.chromium.org/1499063002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall.html
(right):

https://codereview.chromium.org/1499063002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall.html:53:
// Forcing a layout in this frame makes the issue disappear.
On 2015/12/04 14:45:42, Julien Chaffraix - PST wrote:
> On 2015/12/04 at 12:55:54, mstensho wrote:
> > Scary. But I guess there's no need to explain this weirdness, since you just
> fixed it, right?
> 
> It's more of a warning to reviewer. I still think it's important to mention
this
> weirdness.

Absolutely. Sorry about being unclear. What I didn't write in my previous
comment was that my initial thoughts were "WTF! Julien, please write an entire
book about this, please! I want to know everything about this!", but then I
realized that you were probably fixing this very piece of weirdness. And then I
thought that one sentence (which is what you have provided) is just enough, and
that a book on the topic would be TMI. :)

https://codereview.chromium.org/1499063002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall.html
(right):

https://codereview.chromium.org/1499063002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall.html:18:
window.jsTestIsAsync = true;
What's jsTestIsAsync, Gramps?

[Julien - ping for review, 2015-12-04 15:16:33]

You made me realize I forgot to update the test for what js-test.js gives me,
new patch brewing.

https://codereview.chromium.org/1499063002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall.html
(right):

https://codereview.chromium.org/1499063002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/input/placeholder-wrongly-placed-if-too-tall.html:18:
window.jsTestIsAsync = true;
On 2015/12/04 at 14:57:36, mstensho wrote:
> What's jsTestIsAsync, Gramps?

It's the new hotness in js-test land!!!

It is intended for tests like this one that are asynchronous (ie driven off rAF
or setTimeout). By default, js test registers an event listener for
DOMContentLoaded to output:

"PASS successfullyParsed is true

TEST COMPLETE"

which doesn't work with our test. That's why you have to set jsTestIsAsync so
that we have to explicitly call finishJSTest() when we're done.

[Julien - ping for review, 2015-12-04 16:44:36]

The CQ bit was checked by jchaffraix@chromium.org

[Julien - ping for review, 2015-12-04 16:44:36]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/1499063002/#ps60001
(title: "Upgraded the test to js-test.js for real!")

[commit-bot: I haz the power, 2015-12-04 16:45:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1499063002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1499063002/60001

[commit-bot: I haz the power, 2015-12-04 18:35:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-04 18:35:35]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Julien - ping for review, 2015-12-04 22:18:01]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2015-12-04 22:48:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1499063002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1499063002/60001

[commit-bot: I haz the power, 2015-12-05 00:48:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-05 00:49:39]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/2036c7d6d5438beefe656aa67d1d90af166f0960
Cr-Commit-Position: refs/heads/master@{#363324}
==========

[commit-bot: I haz the power, 2015-12-05 00:49:40]

Patchset 4 (id:??) landed as
https://crrev.com/2036c7d6d5438beefe656aa67d1d90af166f0960
Cr-Commit-Position: refs/heads/master@{#363324}