Ignore the percentage value to calculate the logical height of input control

Ignore the percentage value to calculate the logical height of input control

When the form doesn't have the height value, we should ignore the percentage
value of the input control height to calculate the logical height properly.

BUG=607526
TEST=fast/forms/input-height.html

[joone, 2016-11-04 08:29:39]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[joone, 2016-11-04 08:30:01]

Description was changed from

==========
Keep the line-height for single text control when it has a pecentage value for
the height

BUG=607526
TEST=
==========

to

==========
Keep the line-height for single text control when it has a pecentage value for
the height

BUG=607526
TEST=fast/forms/input-keep-line-height.html
==========

[commit-bot: I haz the power, 2016-11-04 08:30:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 08:32:34]

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

[commit-bot: I haz the power, 2016-11-04 08:32:35]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[joone, 2016-11-04 08:43:04]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 08:43:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 08:45:59]

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

[commit-bot: I haz the power, 2016-11-04 08:46:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[joone, 2016-11-04 08:52:51]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 08:53:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 10:25:59]

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

[commit-bot: I haz the power, 2016-11-04 10:26:00]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[joone, 2016-11-04 18:32:44]

Description was changed from

==========
Keep the line-height for single text control when it has a pecentage value for
the height

BUG=607526
TEST=fast/forms/input-keep-line-height.html
==========

to

==========
Don't check the pecentage or calculated value for the height of innerEditor

BUG=607526
TEST=fast/forms/input-keep-line-height.html
==========

[joone, 2016-11-04 19:56:03]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 19:56:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joone, 2016-11-04 21:13:51]

Description was changed from

==========
Don't check the pecentage or calculated value for the height of innerEditor

BUG=607526
TEST=fast/forms/input-keep-line-height.html
==========

to

==========
Don't check the pecentage value to consider the line-height of innerEditor

We don't need to check the percentage value to consider the line-height
of innerEditor so that we can get the right height of form.

If so, the caret will be rendered in the middle of the input box.

BUG=607526
TEST=fast/forms/input-keep-line-height.html
==========

[joone, 2016-11-04 21:15:52]

Description was changed from

==========
Don't check the pecentage value to consider the line-height of innerEditor

We don't need to check the percentage value to consider the line-height
of innerEditor so that we can get the right height of form.

If so, the caret will be rendered in the middle of the input box.

BUG=607526
TEST=fast/forms/input-keep-line-height.html
==========

to

==========
Don't check the percentage value to consider the line-height of innerEditor

We don't need to check the percentage value to consider the line-height
of innerEditor so that we can get the right height of form.

If so, the caret will be rendered in the middle of the input box.

BUG=607526
TEST=fast/forms/input-keep-line-height.html
==========

[joone, 2016-11-04 21:17:13]

Description was changed from

==========
Don't check the percentage value to consider the line-height of innerEditor

We don't need to check the percentage value to consider the line-height
of innerEditor so that we can get the right height of form.

If so, the caret will be rendered in the middle of the input box.

BUG=607526
TEST=fast/forms/input-keep-line-height.html
==========

to

==========
Don't check the percentage value to consider the line-height of innerEditor

We don't need to check the percentage value to consider the line-height
of innerEditor so that we can get the right height of form.

If so, the caret will be rendered in the middle of the form.

BUG=607526
TEST=fast/forms/input-keep-line-height.html
==========

[joone, 2016-11-04 21:18:16]

joone.hur@intel.com changed reviewers:
+ tkent@chromium.org

[joone, 2016-11-04 21:19:40]

Hi, tkent@ could you take a look at this CL?

[commit-bot: I haz the power, 2016-11-04 22:54:43]

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

[commit-bot: I haz the power, 2016-11-04 22:54:44]

Dry run: This issue passed the CQ dry run.

[tkent, 2016-11-07 01:50:28]

What happens if the percent-specified INPUT height is smaller than the
line-height?  Is it rendered correctly?

[joone, 2016-11-07 19:25:18]

On 2016/11/07 at 01:50:28, tkent wrote:
> What happens if the percent-specified INPUT height is smaller than the
line-height?  Is it rendered correctly?

 #component-search input.query {
	background: blue;
	border: none;
	float: left;
	height: 80%;
	line-height: 24px;
	padding: 0 0 0 5px;
	width: 143px;
}

I changed the height value from 100% to 80%, there is no change: it has the same
rendering result as height = 100%.

[tkent, 2016-11-08 08:29:15]

On 2016/11/07 at 19:25:18, joone.hur wrote:
> On 2016/11/07 at 01:50:28, tkent wrote:
> > What happens if the percent-specified INPUT height is smaller than the
line-height?  Is it rendered correctly?
> 
>  #component-search input.query {
> 	background: blue;
> 	border: none;
> 	float: left;
> 	height: 80%;
> 	line-height: 24px;
> 	padding: 0 0 0 5px;
> 	width: 143px;
> }
> 
> I changed the height value from 100% to 80%, there is no change: it has the
same rendering result as height = 100%.

Please test with
 - Adding a specific height to <form>.  e.g. <form style="height:32px">, and
 - Make the computed height of the INPUT smaller than line-height; e.g. height:
30% (== 9.6px)


Excluding percent height when the percent height doesn't work (the parent
ComputedStyle doesn't have a concrete height) makes sense.

[joone, 2016-11-08 18:35:19]

On 2016/11/08 at 08:29:15, tkent wrote:
> On 2016/11/07 at 19:25:18, joone.hur wrote:
> > On 2016/11/07 at 01:50:28, tkent wrote:
> > > What happens if the percent-specified INPUT height is smaller than the
line-height?  Is it rendered correctly?
> > 
> >  #component-search input.query {
> > 	background: blue;
> > 	border: none;
> > 	float: left;
> > 	height: 80%;
> > 	line-height: 24px;
> > 	padding: 0 0 0 5px;
> > 	width: 143px;
> > }
> > 
> > I changed the height value from 100% to 80%, there is no change: it has the
same rendering result as height = 100%.
> 
> Please test with
>  - Adding a specific height to <form>.  e.g. <form style="height:32px">, and
>  - Make the computed height of the INPUT smaller than line-height; e.g.
height: 30% (== 9.6px)
> 
> 
> Excluding percent height when the percent height doesn't work (the parent
ComputedStyle doesn't have a concrete height) makes sense.

Here is a screenshot with this CL:
https://bugs.chromium.org/p/chromium/issues/detail?id=607526#c9

The percent height of the input element doesn't work.

[tkent, 2016-11-08 23:18:17]

On 2016/11/08 at 18:35:19, joone.hur wrote:
> On 2016/11/08 at 08:29:15, tkent wrote:
> > On 2016/11/07 at 19:25:18, joone.hur wrote:
> > > On 2016/11/07 at 01:50:28, tkent wrote:
> > > > What happens if the percent-specified INPUT height is smaller than the
line-height?  Is it rendered correctly?
> > > 
> > >  #component-search input.query {
> > > 	background: blue;
> > > 	border: none;
> > > 	float: left;
> > > 	height: 80%;
> > > 	line-height: 24px;
> > > 	padding: 0 0 0 5px;
> > > 	width: 143px;
> > > }
> > > 
> > > I changed the height value from 100% to 80%, there is no change: it has
the same rendering result as height = 100%.
> > 
> > Please test with
> >  - Adding a specific height to <form>.  e.g. <form style="height:32px">, and
> >  - Make the computed height of the INPUT smaller than line-height; e.g.
height: 30% (== 9.6px)
> > 
> > 
> > Excluding percent height when the percent height doesn't work (the parent
ComputedStyle doesn't have a concrete height) makes sense.
> 
> Here is a screenshot with this CL:
https://bugs.chromium.org/p/chromium/issues/detail?id=607526#c9
> 
> The percent height of the input element doesn't work.

It means this CL is wrong.  In that case, height:30% and height:9.6px should
have identical rendering.

[joone, 2016-11-08 23:23:29]

On 2016/11/08 at 23:18:17, tkent wrote:
> On 2016/11/08 at 18:35:19, joone.hur wrote:
> > On 2016/11/08 at 08:29:15, tkent wrote:
> > > On 2016/11/07 at 19:25:18, joone.hur wrote:
> > > > On 2016/11/07 at 01:50:28, tkent wrote:
> > > > > What happens if the percent-specified INPUT height is smaller than the
line-height?  Is it rendered correctly?
> > > > 
> > > >  #component-search input.query {
> > > > 	background: blue;
> > > > 	border: none;
> > > > 	float: left;
> > > > 	height: 80%;
> > > > 	line-height: 24px;
> > > > 	padding: 0 0 0 5px;
> > > > 	width: 143px;
> > > > }
> > > > 
> > > > I changed the height value from 100% to 80%, there is no change: it has
the same rendering result as height = 100%.
> > > 
> > > Please test with
> > >  - Adding a specific height to <form>.  e.g. <form style="height:32px">,
and
> > >  - Make the computed height of the INPUT smaller than line-height; e.g.
height: 30% (== 9.6px)
> > > 
> > > 
> > > Excluding percent height when the percent height doesn't work (the parent
ComputedStyle doesn't have a concrete height) makes sense.
> > 
> > Here is a screenshot with this CL:
https://bugs.chromium.org/p/chromium/issues/detail?id=607526#c9
> > 
> > The percent height of the input element doesn't work.
> 
> It means this CL is wrong.  In that case, height:30% and height:9.6px should
have identical rendering.

The height of the input element is the same, but the height of the form is
increased because the height is 32px.
Isn't it normal?

[joone, 2016-11-08 23:38:22]

On 2016/11/08 at 23:23:29, joone wrote:
> On 2016/11/08 at 23:18:17, tkent wrote:
> > > > Please test with
> > > >  - Adding a specific height to <form>.  e.g. <form style="height:32px">,
and
> > > >  - Make the computed height of the INPUT smaller than line-height; e.g.
height: 30% (== 9.6px)
> > > > 
> > > > 
> > > > Excluding percent height when the percent height doesn't work (the
parent ComputedStyle doesn't have a concrete height) makes sense.
> > > 
> > > Here is a screenshot with this CL:
https://bugs.chromium.org/p/chromium/issues/detail?id=607526#c9
> > > 
> > > The percent height of the input element doesn't work.
> > 
> > It means this CL is wrong.  In that case, height:30% and height:9.6px should
have identical rendering.
> 
> The height of the input element is the same, but the height of the form is
increased because the height is 32px.
> Isn't it normal?

Firefox works like my CL.

[tkent, 2016-11-08 23:40:02]

On 2016/11/08 at 23:23:29, joone.hur wrote:
> > > Here is a screenshot with this CL:
https://bugs.chromium.org/p/chromium/issues/detail?id=607526#c9
> > > 
> > > The percent height of the input element doesn't work.
> > 
> > It means this CL is wrong.  In that case, height:30% and height:9.6px should
have identical rendering.
> 
> The height of the input element is the same, but the height of the form is
increased because the height is 32px.
> Isn't it normal?

Same as what?

* With FORM height:32px + INPUT height:30%, rendering with this CL and rendering
without this CL should be identical.
* Rendering with FORM height:32px + INPUT height:30% and rendering with FORM
height:32px + INPUT height:9.6px should be identical.
We should reset line-height as ever.

* Rendering with FORM without height + INPUT height:100% and rendering with FORM
without height + INPUT without height should be identical.
I understand you're trying to fix this.  We should not reset line-height in this
case because the percent height is used for layout.

[tkent, 2016-11-08 23:41:08]

> I understand you're trying to fix this.  We should not reset line-height in
this case because the percent height is used for layout.

is used -> is not used

[joone, 2016-11-09 00:02:26]

On 2016/11/08 at 23:41:08, tkent wrote:
> > I understand you're trying to fix this.  We should not reset line-height in
this case because the percent height is used for layout.
> 
> is used -> is not used

Firefox and Edge honor the line-height of input element in case that the
percentage height is used.
It means the percentage height is ignored.

[joone, 2016-11-15 00:02:44]

Description was changed from

==========
Don't check the percentage value to consider the line-height of innerEditor

We don't need to check the percentage value to consider the line-height
of innerEditor so that we can get the right height of form.

If so, the caret will be rendered in the middle of the form.

BUG=607526
TEST=fast/forms/input-keep-line-height.html
==========

to

==========
Consider the percentage value of the input contorl to calculate the height

We need to consider the percentage value of input control to calculate
the logical height. In this case, the caret will be rendered in the middle
of the form.

BUG=607526
TEST=fast/forms/input-height.html
==========

[joone, 2016-11-15 00:05:41]

Description was changed from

==========
Consider the percentage value of the input contorl to calculate the height

We need to consider the percentage value of input control to calculate
the logical height. In this case, the caret will be rendered in the middle
of the form.

BUG=607526
TEST=fast/forms/input-height.html
==========

to

==========
Consider the percentage value of the input contorl to calculate the height

Currently, the pixel value is only honored so we need to consider the percentage

value of input control to calculate the logical height. In this case, the caret
will be rendered in the middle of the form.

BUG=607526
TEST=fast/forms/input-height.html
==========

[joone, 2016-11-15 01:11:15]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 01:11:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joone, 2016-11-15 01:32:57]

tkent@ could you take a look at the updated CL?

Currently, the percentage value doesn't work well so the update CL can fix this
problem.
However, the height can't be reduced more than the font size by changing the
percentage value. Is this normal?

[joone, 2016-11-15 01:34:03]

Description was changed from

==========
Consider the percentage value of the input contorl to calculate the height

Currently, the pixel value is only honored so we need to consider the percentage

value of input control to calculate the logical height. In this case, the caret
will be rendered in the middle of the form.

BUG=607526
TEST=fast/forms/input-height.html
==========

to

==========
Consider the percentage value of the input control to calculate the height

Currently, the pixel value is only honored so we need to consider the percentage

value of input control to calculate the logical height. In this case, the caret
will be rendered in the middle of the form.

BUG=607526
TEST=fast/forms/input-height.html
==========

[commit-bot: I haz the power, 2016-11-15 03:30:09]

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

[commit-bot: I haz the power, 2016-11-15 03:30:10]

Dry run: This issue passed the CQ dry run.

[joone, 2016-11-15 19:45:39]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 19:46:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joone, 2016-11-15 19:47:04]

On 2016/11/15 at 01:32:57, joone wrote:
> tkent@ could you take a look at the updated CL?
> 
> Currently, the percentage value doesn't work well so the update CL can fix
this problem.
> However, the height can't be reduced more than the font size by changing the
percentage value. Is this normal?

Now, the percentage height and pixel height have the same rendering result.

[commit-bot: I haz the power, 2016-11-15 20:00:46]

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

[commit-bot: I haz the power, 2016-11-15 20:00:47]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[joone, 2016-11-18 00:44:15]

Description was changed from

==========
Consider the percentage value of the input control to calculate the height

Currently, the pixel value is only honored so we need to consider the percentage

value of input control to calculate the logical height. In this case, the caret
will be rendered in the middle of the form.

BUG=607526
TEST=fast/forms/input-height.html
==========

to

==========
Consider the percentage value of the input control to calculate the height

Currently, the pixel value is only honored so we need to consider the percentage

value of input control to calculate the logical height. In this case, the caret
will be rendered in the middle of the form.

BUG=607526
TEST=fast/forms/input-height.html
==========

[tkent, 2016-11-18 08:59:12]

On 2016/11/15 at 19:47:04, joone.hur wrote:
> On 2016/11/15 at 01:32:57, joone wrote:
> > tkent@ could you take a look at the updated CL?
> > 
> > Currently, the percentage value doesn't work well so the update CL can fix
this problem.
> > However, the height can't be reduced more than the font size by changing the
percentage value. Is this normal?
> 
> Now, the percentage height and pixel height have the same rendering result.

I haven't checked the code closely yet, however,
 - An existing test fails.
 - Does computeControlLogicalHeight() work with CSS variable?
 - Does the logic in computeControlLogicalHeight() match to the standard
percent-length computation?

[joone, 2016-11-18 23:35:27]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 23:36:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-19 01:09:56]

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

[commit-bot: I haz the power, 2016-11-19 01:09:57]

Dry run: This issue passed the CQ dry run.

[joone, 2016-11-19 01:41:35]

On 2016/11/18 08:59:12, tkent wrote:
> On 2016/11/15 at 19:47:04, joone.hur wrote:
> > On 2016/11/15 at 01:32:57, joone wrote:
> > > tkent@ could you take a look at the updated CL?
> > > 
> > > Currently, the percentage value doesn't work well so the update CL can fix
> this problem.
> > > However, the height can't be reduced more than the font size by changing
the
> percentage value. Is this normal?
> > 
> > Now, the percentage height and pixel height have the same rendering result.
> 
> I haven't checked the code closely yet, however,
>  - An existing test fails.
>  - Does computeControlLogicalHeight() work with CSS variable?
>  - Does the logic in computeControlLogicalHeight() match to the standard
> percent-length computation?

I fixed the test fail, but how can I check the others?

[tkent, 2016-11-29 08:51:36]

On 2016/11/19 at 01:41:35, joone.hur wrote:
> > I haven't checked the code closely yet, however,
> >  - An existing test fails.
> >  - Does computeControlLogicalHeight() work with CSS variable?
> >  - Does the logic in computeControlLogicalHeight() match to the standard
> > percent-length computation?
> 
> I fixed the test fail, but how can I check the others?

CSS variable: Just make a test
Match to standard: Please read the CSS specifications, and find Blink
implementation for it.

I'm afraid re-implement percent-length computation logic here causes
regressions.

[joone, 2016-11-30 01:10:45]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 01:11:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joone, 2016-11-30 01:55:25]

On 2016/11/29 08:51:36, tkent wrote:
> On 2016/11/19 at 01:41:35, joone.hur wrote:
> > > I haven't checked the code closely yet, however,
> > >  - An existing test fails.
> > >  - Does computeControlLogicalHeight() work with CSS variable?
> > >  - Does the logic in computeControlLogicalHeight() match to the standard
> > > percent-length computation?
> > 
> > I fixed the test fail, but how can I check the others?
> 
> CSS variable: Just make a test
> Match to standard: Please read the CSS specifications, and find Blink
> implementation for it.

CSS variable was applied to the test case.

> I'm afraid re-implement percent-length computation logic here causes
> regressions.

Do you mean LayoutBox::computePercentageLogicalHeight() or
LayoutBlock::availableLogicalHeightForPercentageComputation()?
They don't work with this case.

[commit-bot: I haz the power, 2016-11-30 03:14:03]

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

[commit-bot: I haz the power, 2016-11-30 03:14:04]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[tkent, 2017-01-16 08:05:25]

On 2016/11/30 at 01:55:25, joone.hur wrote:
> > I'm afraid re-implement percent-length computation logic here causes
> > regressions.
> 
> Do you mean LayoutBox::computePercentageLogicalHeight() or
LayoutBlock::availableLogicalHeightForPercentageComputation()?
> They don't work with this case.

The former is the right one.  Why doesn't it work?

[tkent, 2017-01-18 05:54:01]

I think the simplest solution is to compute intrinsic height with
this->lineHeight() instead of innerEditorBox->lineHeight() in
LayoutTextControl::computeLogicalHeight().

[joone, 2017-01-18 23:06:56]

On 2017/01/18 05:54:01, tkent wrote:
> I think the simplest solution is to compute intrinsic height with
> this->lineHeight() instead of innerEditorBox->lineHeight() in
> LayoutTextControl::computeLogicalHeight().

It doesn't work:
https://bugs.chromium.org/p/chromium/issues/detail?id=607526#c12

[tkent, 2017-01-18 23:15:24]

On 2017/01/18 at 23:06:56, joone.hur wrote:
> On 2017/01/18 05:54:01, tkent wrote:
> > I think the simplest solution is to compute intrinsic height with
> > this->lineHeight() instead of innerEditorBox->lineHeight() in
> > LayoutTextControl::computeLogicalHeight().
> 
> It doesn't work:
https://bugs.chromium.org/p/chromium/issues/detail?id=607526#c12

input-height-actual.png is the result of my suggestion, right?  It looks correct
to me.  height:30% in the second INPUT should be ignored according to the CSS
standard.

[joone, 2017-01-18 23:35:47]

On 2017/01/18 23:15:24, tkent wrote:
> On 2017/01/18 at 23:06:56, joone.hur wrote:
> > On 2017/01/18 05:54:01, tkent wrote:
> > > I think the simplest solution is to compute intrinsic height with
> > > this->lineHeight() instead of innerEditorBox->lineHeight() in
> > > LayoutTextControl::computeLogicalHeight().
> > 
> > It doesn't work:
> https://bugs.chromium.org/p/chromium/issues/detail?id=607526#c12
> 
> input-height-actual.png is the result of my suggestion, right?  It looks
correct
> to me.  height:30% in the second INPUT should be ignored according to the CSS
> standard.

Yes, but, you mentioned that height:30% and height:9.6px should
have identical rendering:
https://codereview.chromium.org/2478483003/#msg28

[tkent, 2017-01-18 23:42:45]

On 2017/01/18 at 23:35:47, joone.hur wrote:
> On 2017/01/18 23:15:24, tkent wrote:
> > On 2017/01/18 at 23:06:56, joone.hur wrote:
> > > On 2017/01/18 05:54:01, tkent wrote:
> > > > I think the simplest solution is to compute intrinsic height with
> > > > this->lineHeight() instead of innerEditorBox->lineHeight() in
> > > > LayoutTextControl::computeLogicalHeight().
> > > 
> > > It doesn't work:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=607526#c12
> > 
> > input-height-actual.png is the result of my suggestion, right?  It looks
correct
> > to me.  height:30% in the second INPUT should be ignored according to the
CSS
> > standard.
> 
> Yes, but, you mentioned that height:30% and height:9.6px should
> have identical rendering:
> https://codereview.chromium.org/2478483003/#msg28

Please remember the context.  I wrote:

> > Please test with
> >  - Adding a specific height to <form>.  e.g. <form style="height:32px">, and
> >  - Make the computed height of the INPUT smaller than line-height; e.g.
height: 30% (== 9.6px)

<form> in input-height.html doesn't have height property.

[joone, 2017-01-18 23:49:40]

On 2017/01/18 23:42:45, tkent wrote:
> On 2017/01/18 at 23:35:47, joone.hur wrote:
> > On 2017/01/18 23:15:24, tkent wrote:
> > > On 2017/01/18 at 23:06:56, joone.hur wrote:
> > > > On 2017/01/18 05:54:01, tkent wrote:
> > > > > I think the simplest solution is to compute intrinsic height with
> > > > > this->lineHeight() instead of innerEditorBox->lineHeight() in
> > > > > LayoutTextControl::computeLogicalHeight().
> > > > 
> > > > It doesn't work:
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=607526#c12
> > > 
> > > input-height-actual.png is the result of my suggestion, right?  It looks
> correct
> > > to me.  height:30% in the second INPUT should be ignored according to the
> CSS
> > > standard.
> > 
> > Yes, but, you mentioned that height:30% and height:9.6px should
> > have identical rendering:
> > https://codereview.chromium.org/2478483003/#msg28
> 
> Please remember the context.  I wrote:
> 
> > > Please test with
> > >  - Adding a specific height to <form>.  e.g. <form style="height:32px">,
and
> > >  - Make the computed height of the INPUT smaller than line-height; e.g.
> height: 30% (== 9.6px)
> 
> <form> in input-height.html doesn't have height property.

Okay, I will update the CL.

[joone, 2017-01-19 00:52:48]

Description was changed from

==========
Consider the percentage value of the input control to calculate the height

Currently, the pixel value is only honored so we need to consider the percentage

value of input control to calculate the logical height. In this case, the caret
will be rendered in the middle of the form.

BUG=607526
TEST=fast/forms/input-height.html
==========

to

==========
Ignore the percentage value to calculate the logical height of input control

When the form doesn't have the height value, we should Ignore the percentage
value of the input control height to calculate the logical height properly.

BUG=607526
TEST=fast/forms/input-height.html
==========

[joone, 2017-01-19 00:53:10]

Description was changed from

==========
Ignore the percentage value to calculate the logical height of input control

When the form doesn't have the height value, we should Ignore the percentage
value of the input control height to calculate the logical height properly.

BUG=607526
TEST=fast/forms/input-height.html
==========

to

==========
Ignore the percentage value to calculate the logical height of input control

When the form doesn't have the height value, we should ignore the percentage
value of the input control height to calculate the logical height properly.

BUG=607526
TEST=fast/forms/input-height.html
==========

[joone, 2017-01-19 00:53:33]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 00:54:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 00:56:58]

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

[commit-bot: I haz the power, 2017-01-19 00:56:59]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[joone, 2017-01-19 01:02:34]

Patchset #9 (id:160001) has been deleted

[joone, 2017-01-19 01:02:42]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 01:03:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joone, 2017-01-19 01:54:44]

tkent@ can you review the updated CL?

[tkent, 2017-01-19 02:13:04]

https://codereview.chromium.org/2478483003/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/forms/input-height.html (right):

https://codereview.chromium.org/2478483003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/forms/input-height.html:1: <!DOCTYPE html>
I recommend to make this a reference test, not a pixel test.

input-height.html should contain
 * The second DIV of this version; FORM without height + INPUT with height:30%,
and
 * FORM with height + INPUT with height:30%

input-height-expected.html should contain:
 * The first DIV of this version; FORM without height + INPUT without height
 * The third DIV of this version; FORM without height + INPUT with height:9.6px

https://codereview.chromium.org/2478483003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/forms/input-height.html:29: <title>input
caret size by cubix</title>
Now caret size is not a problem.

https://codereview.chromium.org/2478483003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/forms/input-height.html:41: <div
id="component-search" class="header-component">
ID attribute value 'component-search' conflicts with other elements.

https://codereview.chromium.org/2478483003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/forms/input-height.html:43: <input
type="text" class="query" name="q" id="s" size="31" placeholder="Search"
autocomplete="off" style="color: black; height: 30%">
ID attribute value 's' conflicts with other elements.

https://codereview.chromium.org/2478483003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/forms/input-height.html:59: 
unnecessary blank lines.

https://codereview.chromium.org/2478483003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTextControl.cpp (right):

https://codereview.chromium.org/2478483003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTextControl.cpp:124:
this->lineHeight(true, HorizontalLine, PositionOfInteriorLineBoxes),
nit: |this->| is unnecessary.

[commit-bot: I haz the power, 2017-01-19 02:29:14]

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

[commit-bot: I haz the power, 2017-01-19 02:29:15]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[joone, 2017-01-21 00:15:18]

Patchset #10 (id:200001) has been deleted

[joone, 2017-01-21 00:15:49]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-21 00:16:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-21 02:01:17]

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

[commit-bot: I haz the power, 2017-01-21 02:01:20]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[joone, 2017-01-21 07:32:50]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-21 07:33:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-21 09:18:31]

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

[commit-bot: I haz the power, 2017-01-21 09:18:32]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[joone, 2017-01-23 01:07:08]

tkent@ thanks for review.

https://codereview.chromium.org/2478483003/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/forms/input-height.html (right):

https://codereview.chromium.org/2478483003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/forms/input-height.html:1: <!DOCTYPE html>
On 2017/01/19 02:13:03, tkent wrote:
> I recommend to make this a reference test, not a pixel test.
> 
> input-height.html should contain
>  * The second DIV of this version; FORM without height + INPUT with
height:30%,
> and
>  * FORM with height + INPUT with height:30%
> 
> input-height-expected.html should contain:
>  * The first DIV of this version; FORM without height + INPUT without height
>  * The third DIV of this version; FORM without height + INPUT with
height:9.6px

Done.

https://codereview.chromium.org/2478483003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/forms/input-height.html:29: <title>input
caret size by cubix</title>
On 2017/01/19 02:13:03, tkent wrote:
> Now caret size is not a problem.

Done.

https://codereview.chromium.org/2478483003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/forms/input-height.html:41: <div
id="component-search" class="header-component">
On 2017/01/19 02:13:03, tkent wrote:
> ID attribute value 'component-search' conflicts with other elements.

Done.

https://codereview.chromium.org/2478483003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/forms/input-height.html:43: <input
type="text" class="query" name="q" id="s" size="31" placeholder="Search"
autocomplete="off" style="color: black; height: 30%">
On 2017/01/19 02:13:03, tkent wrote:
> ID attribute value 's' conflicts with other elements.
> 

Done.

https://codereview.chromium.org/2478483003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/forms/input-height.html:59: 
On 2017/01/19 02:13:03, tkent wrote:
> unnecessary blank lines.

Done.

[tkent, 2017-01-23 04:15:50]

https://codereview.chromium.org/2478483003/diff/240001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2478483003/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:2202: crbug.com/607526
fast/css/line-height.html [ NeedsRebaseline ]
These behavior change don't look acceptable.

My wild guess is we need to port the first condition to clear line-height in
LayoutTextControlSingleLine::createInnerEditorStyle() to
LayoutTextControl(SingleLine?)::computeLogicalHeight().