getComputedStyle() should return used value for 'line-height'

getComputedStyle() should return used value for 'line-height'

As per the spec https://drafts.csswg.org/cssom/#resolved-value
getComputedStyle() should return used value for 'line-height' which
Chrome currently doesn't do. This patch fixes that to make it more
interoperable with FF.

BUG=383409

[nainar, 2016-03-21 05:54:33]

nainar@chromium.org changed reviewers:
+ meade@chromium.org

[nainar, 2016-03-21 05:54:34]

Hi Eddy!

PTAL?

Thanks!

[Timothy Loh, 2016-03-21 05:56:20]

https://codereview.chromium.org/1820773002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
(right):

https://codereview.chromium.org/1820773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:468: return
zoomAdjustedPixelValue(floatValueForLength(length,
style.getFontDescription().computedSize()), style);
floatValueForLength resolves percentages but if you create a Length(Fixed) then
there are no percentages. Do we need to create a Length at all?

[nainar, 2016-03-21 06:21:44]

nainar@chromium.org changed reviewers:
+ timloh@chromium.org

[nainar, 2016-03-21 06:21:45]

Hi!

Responded to Tim's comment. PTAL?

Thanks!

https://codereview.chromium.org/1820773002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
(right):

https://codereview.chromium.org/1820773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:468: return
zoomAdjustedPixelValue(floatValueForLength(length,
style.getFontDescription().computedSize()), style);
No. You're right we can also omit the check for isNegative() in that case.
computedLineHeight deals with the negative case.

[Timothy Loh, 2016-03-21 23:54:12]

On 2016/03/21 06:21:45, nainar wrote:
> Hi!
> 
> Responded to Tim's comment. PTAL?
> 
> Thanks!
> 
>
https://codereview.chromium.org/1820773002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
> (right):
> 
>
https://codereview.chromium.org/1820773002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:468:
return
> zoomAdjustedPixelValue(floatValueForLength(length,
> style.getFontDescription().computedSize()), style);
> No. You're right we can also omit the check for isNegative() in that case.
> computedLineHeight deals with the negative case.

bots are all red?

[nainar, 2016-03-22 07:53:31]

Fixed the failing tests issue. Also changed computedLineHeight to return a float
instead of an int. 

PTAL?

Thanks!

[nainar, 2016-03-23 03:53:37]

@meade, 

Changed the needed unit tests. PTAL?

Thanks!

[meade_UTC10, 2016-03-23 23:18:16]

On 2016/03/23 03:53:37, nainar wrote:
> @meade, 
> 
> Changed the needed unit tests. PTAL?
> 
> Thanks!

Code looks good but some of the bots are still red...

[nainar, 2016-04-06 06:39:51]

@timloh,

Tim, could you take a look at the code change. I still have a few tests that are
failing that I need to take a look at. However, to get those tests passing I
would have to change the expectation files to platform specific expectation
file. Which makes me think the change I am making is wrong. Please let me know
if there is something I am missing.

Thanks!

[Timothy Loh, 2016-04-11 03:32:16]

https://codereview.chromium.org/1820773002/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1820773002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:636: float
computedLineHeight() const;
Why is this changing?

[nainar, 2016-04-11 04:42:15]

https://codereview.chromium.org/1820773002/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1820773002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:636: float
computedLineHeight() const;
If I retain it as int we lose out on information. getComputedstyle(e).lineHeight
will always be an int. FF reports getComputedstyle(e).lineHeight as a float.

[Timothy Loh, 2016-04-11 04:44:43]

https://codereview.chromium.org/1820773002/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1820773002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:636: float
computedLineHeight() const;
On 2016/04/11 04:42:15, nainar wrote:
> If I retain it as int we lose out on information.
getComputedstyle(e).lineHeight
> will always be an int. FF reports getComputedstyle(e).lineHeight as a float.

Doesn't this mean we're changing layout behaviour though?

[nainar, 2016-04-11 04:53:57]

On 2016/04/11 at 04:42:15, nainar wrote:
>
https://codereview.chromium.org/1820773002/diff/90001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/style/ComputedStyle.h (right):
> 
>
https://codereview.chromium.org/1820773002/diff/90001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/style/ComputedStyle.h:636: float
computedLineHeight() const;
> If I retain it as int we lose out on information.
getComputedstyle(e).lineHeight will always be an int. FF reports
getComputedstyle(e).lineHeight as a float.

Yes this does change the layout behaviour. Would making a separate function in
this case that does the same job as computedLineHeight but returns a float be
appropriate?

[Timothy Loh, 2016-04-11 04:55:03]

On 2016/04/11 04:53:57, nainar wrote:
> On 2016/04/11 at 04:42:15, nainar wrote:
> >
>
https://codereview.chromium.org/1820773002/diff/90001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/style/ComputedStyle.h (right):
> > 
> >
>
https://codereview.chromium.org/1820773002/diff/90001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/style/ComputedStyle.h:636: float
> computedLineHeight() const;
> > If I retain it as int we lose out on information.
> getComputedstyle(e).lineHeight will always be an int. FF reports
> getComputedstyle(e).lineHeight as a float.
> 
> Yes this does change the layout behaviour. Would making a separate function in
> this case that does the same job as computedLineHeight but returns a float be
> appropriate?

If we currently use an int value of line height to layout, why do we want to use
a float? Isn't the used value in this case an int?

[nainar, 2016-04-11 05:41:58]

On 2016/04/11 at 04:55:03, timloh wrote:
> On 2016/04/11 04:53:57, nainar wrote:
> > On 2016/04/11 at 04:42:15, nainar wrote:
> > >
> >
https://codereview.chromium.org/1820773002/diff/90001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/style/ComputedStyle.h (right):
> > > 
> > >
> >
https://codereview.chromium.org/1820773002/diff/90001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/style/ComputedStyle.h:636: float
> > computedLineHeight() const;
> > > If I retain it as int we lose out on information.
> > getComputedstyle(e).lineHeight will always be an int. FF reports
> > getComputedstyle(e).lineHeight as a float.
> > 
> > Yes this does change the layout behaviour. Would making a separate function
in
> > this case that does the same job as computedLineHeight but returns a float
be
> > appropriate?
> 
> If we currently use an int value of line height to layout, why do we want to
use a float? Isn't the used value in this case an int?

Yes the used value is an int. I was looking at compatibility with FF and other
browsers.

For example in the below case:
{input: "2cm", expected: "75px"}, 
{input: "20mm", expected: "75px"},

FF would return 75.5833px in both cases.

[Timothy Loh, 2016-04-12 13:25:40]

ok I refreshed myself on the relevant specs, in particular

getComputedStyle line-height -> resolved value -> used value -> no rounding
actual value -> used value with (e.g.) rounding

so let's add a function (floatComputedLineHeight?) and add a comment like
// This function only exists for getComputedStyle, layout is only performed with
the int value

[meade_UTC10, 2016-08-15 01:04:44]

I'm going to remove myself from the R line if this CL isn't needed any more.

[meade_UTC10, 2016-08-15 01:05:00]

Description was changed from

==========
getComputedStyle() should return used value for 'line-height'

As per the spec https://drafts.csswg.org/cssom/#resolved-value
getComputedStyle() should return used value for 'line-height' which
Chrome currently doesn't do. This patch fixes that to make it more
interoperable with FF.

BUG=383409
==========

to

==========
getComputedStyle() should return used value for 'line-height'

As per the spec https://drafts.csswg.org/cssom/#resolved-value
getComputedStyle() should return used value for 'line-height' which
Chrome currently doesn't do. This patch fixes that to make it more
interoperable with FF.

BUG=383409
==========

[meade_UTC10, 2016-08-15 01:05:00]

meade@chromium.org changed reviewers:
- meade@chromium.org