Text inside input field gets misaligned when relaying out

Text inside input field gets misaligned when relaying out

https://codereview.chromium.org/241113004 changed the way
we layout positioned inputs, causing
LayoutTextControlSingleLine::layout to read stale data as
we didn't relayout the inner text.

BUG=445330
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190853

[Julien - ping for review, 2015-02-23 20:45:55]

jchaffraix@chromium.org changed reviewers:
+ dsinclair@chromium.org, leviw@chromium.org

[Julien - ping for review, 2015-02-23 20:45:56]

Not necessarily the best fix but we don't want to always trigger a second
layout. Also this is for a stable blocker.

[dsinclair, 2015-02-23 21:00:44]

https://codereview.chromium.org/952663002/diff/1/Source/core/layout/LayoutTex...
File Source/core/layout/LayoutTextControlSingleLine.cpp (right):

https://codereview.chromium.org/952663002/diff/1/Source/core/layout/LayoutTex...
Source/core/layout/LayoutTextControlSingleLine.cpp:162: if (!container &&
innerEditorRenderer && innerEditorRenderer->size().height() !=
contentLogicalHeight())
We don't care about width() != contentLogicalWidth()?

[leviw_travelin_and_unemployed, 2015-02-23 21:02:01]

https://codereview.chromium.org/952663002/diff/1/Source/core/layout/LayoutTex...
File Source/core/layout/LayoutTextControlSingleLine.cpp (right):

https://codereview.chromium.org/952663002/diff/1/Source/core/layout/LayoutTex...
Source/core/layout/LayoutTextControlSingleLine.cpp:162: if (!container &&
innerEditorRenderer && innerEditorRenderer->size().height() !=
contentLogicalHeight())
The conditions of this "if" are a little weird, and duplicated *exactly* a few
lines later. Extract into a named function?

[Julien - ping for review, 2015-02-24 01:15:35]

Updated patch up for review!

https://codereview.chromium.org/952663002/diff/1/Source/core/layout/LayoutTex...
File Source/core/layout/LayoutTextControlSingleLine.cpp (right):

https://codereview.chromium.org/952663002/diff/1/Source/core/layout/LayoutTex...
Source/core/layout/LayoutTextControlSingleLine.cpp:162: if (!container &&
innerEditorRenderer && innerEditorRenderer->size().height() !=
contentLogicalHeight())
On 2015/02/23 21:02:00, leviw wrote:
> The conditions of this "if" are a little weird, and duplicated *exactly* a few
> lines later. Extract into a named function?

Spot on. I was trying to be smart about when to relayout but after further
discussion with Levi we decided to just force it regardless of the previous
condition (it's not the first time this logic is making assumption about the
children being relaid out).

[dsinclair, 2015-02-24 18:37:31]

lgtm

[leviw_travelin_and_unemployed, 2015-02-24 18:38:26]

https://codereview.chromium.org/952663002/diff/20001/Source/core/layout/Layou...
File Source/core/layout/LayoutTextControlSingleLine.cpp (right):

https://codereview.chromium.org/952663002/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutTextControlSingleLine.cpp:97: // FIXME: This code is
madness (https://crbug.com/461117)
Well said, sir!

https://codereview.chromium.org/952663002/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutTextControlSingleLine.cpp:110: bool
innerEditorRendererHadLayout = innerEditorRenderer &&
innerEditorRenderer->needsLayout();
innerEditorRendererHadNeedsLayout?

https://codereview.chromium.org/952663002/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutTextControlSingleLine.cpp:141:
layoutScope.setNeedsLayout(innerEditorRenderer);
Do we still need this?

[Julien - ping for review, 2015-02-24 18:48:02]

https://codereview.chromium.org/952663002/diff/20001/Source/core/layout/Layou...
File Source/core/layout/LayoutTextControlSingleLine.cpp (right):

https://codereview.chromium.org/952663002/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutTextControlSingleLine.cpp:110: bool
innerEditorRendererHadLayout = innerEditorRenderer &&
innerEditorRenderer->needsLayout();
On 2015/02/24 18:38:25, leviw wrote:
> innerEditorRendererHadNeedsLayout?

I prefer HadLayout as it corresponds to what's happening as we call layout
below.

https://codereview.chromium.org/952663002/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutTextControlSingleLine.cpp:141:
layoutScope.setNeedsLayout(innerEditorRenderer);
On 2015/02/24 18:38:25, leviw wrote:
> Do we still need this?

Yes. We won't force a relayout if you had a layout in the first pass above and
this requires an extra one.

[leviw_travelin_and_unemployed, 2015-02-24 19:00:21]

lgtm.

[Julien - ping for review, 2015-02-25 18:13:02]

The CQ bit was checked by jchaffraix@chromium.org

[Julien - ping for review, 2015-02-25 18:13:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org
Link to the patchset: https://codereview.chromium.org/952663002/#ps40001 (title:
"Rebaselined")

[commit-bot: I haz the power, 2015-02-25 18:14:25]

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

[commit-bot: I haz the power, 2015-02-25 20:23:36]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190853