Fix getComputedStyle background-position to output calc values

Fix getComputedStyle background-position to output calc values

BUG=260097
Review URL: https://codereview.chromium.org/23651010

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

[Srini, 2013-09-03 07:38:58]

Hi,

Can you review this patch? I will rework if any changes required. 

Thanks,
-Srini.

[alancutter (OOO until 2018), 2013-09-16 07:54:27]

On 2013/09/03 07:38:58, Srini wrote:
> Hi,
> 
> Can you review this patch? I will rework if any changes required. 
> 
> Thanks,
> -Srini.

Thanks for contributing the patch, though there seems to be something strange
going on in the conversion. It should not return calc(10% + 20) as this is not
the same as calc(10% + 20px).
I've posted another patch to address this misconversion:
https://codereview.chromium.org/23439004/

[Srini, 2013-09-20 12:49:08]

On 2013/09/16 07:54:27, alancutter wrote:
> On 2013/09/03 07:38:58, Srini wrote:
> > Hi,
> > 
> > Can you review this patch? I will rework if any changes required. 
> > 
> > Thanks,
> > -Srini.
> 
> Thanks for contributing the patch, though there seems to be something strange
> going on in the conversion. It should not return calc(10% + 20) as this is not
> the same as calc(10% + 20px).
> I've posted another patch to address this misconversion:
> https://codereview.chromium.org/23439004/

Mike Lawther: Can you check up this patch? Thanks!

[Srini, 2013-09-27 11:32:26]

Hi, can you review this patch? Thanks!.

[eae, 2013-09-27 17:35:47]

What is the motivation behind this change? Does the spec say anything about how
the values should be outputed? What does the other browsers do?

[Srini, 2013-09-27 18:04:35]

On 2013/09/27 17:35:47, eae wrote:
> What is the motivation behind this change? Does the spec say anything about
how
> the values should be outputed? What does the other browsers do?

Sorry I didn't the spec. I saw that what was displayed was incorrect and the bug
260097 suggested this output in the test case and sorry I just went with that.
Firefox & IE displays nothing not even "left bottom" as chromium displays today
without my patch. I'm just reading the spec and to me it isn't very clear with
respect to how to deal with Calc in this case.

[eae, 2013-09-27 18:05:42]

On 2013/09/27 18:04:35, Srini wrote:
> On 2013/09/27 17:35:47, eae wrote:
> > What is the motivation behind this change? Does the spec say anything about
> how
> > the values should be outputed? What does the other browsers do?
> 
> Sorry I didn't the spec. I saw that what was displayed was incorrect and the
bug
> 260097 suggested this output in the test case and sorry I just went with that.
> Firefox & IE displays nothing not even "left bottom" as chromium displays
today
> without my patch. I'm just reading the spec and to me it isn't very clear with
> respect to how to deal with Calc in this case.

Yeah it isn't well specified. getComputedStyle is generally supposed to return
resolved/computed values so having it return the formula rather than the values
is a bit odd.

[Srini, 2013-09-27 18:53:20]

On 2013/09/27 18:05:42, eae wrote:
> On 2013/09/27 18:04:35, Srini wrote:
> > On 2013/09/27 17:35:47, eae wrote:
> > > What is the motivation behind this change? Does the spec say anything
about
> > how
> > > the values should be outputed? What does the other browsers do?
> > 
> > Sorry I didn't the spec. I saw that what was displayed was incorrect and the
> bug
> > 260097 suggested this output in the test case and sorry I just went with
that.
> > Firefox & IE displays nothing not even "left bottom" as chromium displays
> today
> > without my patch. I'm just reading the spec and to me it isn't very clear
with
> > respect to how to deal with Calc in this case.
> 
> Yeah it isn't well specified. getComputedStyle is generally supposed to return
> resolved/computed values so having it return the formula rather than the
values
> is a bit odd.

Should I change it to return just computed/final px values then?

[eae, 2013-09-27 18:57:17]

On 2013/09/27 18:53:20, Srini wrote:
> Should I change it to return just computed/final px values then?

Not sure. Given that no other browser seems to implement this and that the spec
is unclear I'd rather hold off until we can either get a clarification or
someone actually needs this.

[Srini, 2013-09-27 19:23:42]

On 2013/09/27 18:57:17, eae wrote:
> On 2013/09/27 18:53:20, Srini wrote:
> > Should I change it to return just computed/final px values then?
> 
> Not sure. Given that no other browser seems to implement this and that the
spec
> is unclear I'd rather hold off until we can either get a clarification or
> someone actually needs this.

Sure. I'll hold this patch till we get more clarity. Thanks for the review in
anycase.

[eae, 2013-09-27 19:57:22]

Thanks. Any time.

[alancutter (OOO until 2018), 2013-10-07 21:53:21]

On 2013/09/27 18:57:17, eae wrote:
> On 2013/09/27 18:53:20, Srini wrote:
> > Should I change it to return just computed/final px values then?
> 
> Not sure. Given that no other browser seems to implement this and that the
spec
> is unclear I'd rather hold off until we can either get a clarification or
> someone actually needs this.

Firefox implements this as returning calc, see http://jsfiddle.net/UZ3US/ in
Firefox.
However the spec suggests that absolute lengths should be used for the computed
value: http://dev.w3.org/csswg/css-backgrounds/#the-background-position

We will need the functionality of computed calc styles on background position to
test animation of the property between px and % values.

[mithro-old, 2013-10-08 05:08:11]

https://codereview.chromium.org/23651010/diff/1/LayoutTests/fast/css/getCompu...
File
LayoutTests/fast/css/getComputedStyle/getComputedStyle-background-position-calc-expected.txt
(right):

https://codereview.chromium.org/23651010/diff/1/LayoutTests/fast/css/getCompu...
LayoutTests/fast/css/getComputedStyle/getComputedStyle-background-position-calc-expected.txt:3:
left calc(10% + 20) bottom calc(30 + 40%)
You actually need:
left calc(10% + 20px) bottom calc(30px + 40%)

It appears.

[Srini, 2013-10-08 05:44:52]

On 2013/10/08 05:08:11, mithro wrote:
>
https://codereview.chromium.org/23651010/diff/1/LayoutTests/fast/css/getCompu...
> File
>
LayoutTests/fast/css/getComputedStyle/getComputedStyle-background-position-calc-expected.txt
> (right):
> 
>
https://codereview.chromium.org/23651010/diff/1/LayoutTests/fast/css/getCompu...
>
LayoutTests/fast/css/getComputedStyle/getComputedStyle-background-position-calc-expected.txt:3:
> left calc(10% + 20) bottom calc(30 + 40%)
> You actually need:
> left calc(10% + 20px) bottom calc(30px + 40%)
> 
> It appears.

Yes. Once I run this test again with Alan Cutter's patch this would be fixed.

[Srini, 2013-10-08 05:50:41]

On 2013/10/07 21:53:21, alancutter wrote:
> On 2013/09/27 18:57:17, eae wrote:
> > On 2013/09/27 18:53:20, Srini wrote:
> > > Should I change it to return just computed/final px values then?
> > 
> > Not sure. Given that no other browser seems to implement this and that the
> spec
> > is unclear I'd rather hold off until we can either get a clarification or
> > someone actually needs this.
> 
> Firefox implements this as returning calc, see http://jsfiddle.net/UZ3US/ in
> Firefox.

Wierd, my earlier test from the bug returned nothing useful but this test works.
So Firefox implements as what I had tried with the patch.

> However the spec suggests that absolute lengths should be used for the
computed
> value: http://dev.w3.org/csswg/css-backgrounds/#the-background-position
> 
> We will need the functionality of computed calc styles on background position
to
> test animation of the property between px and % values.

I'm sorry Alan Cutter, Im not sure if I understood your point correctly. What
test should I write here, wrt animation of property betn px & %?

[alancutter (OOO until 2018), 2013-10-08 11:20:08]

On 2013/10/08 05:50:41, Srini wrote:
> On 2013/10/07 21:53:21, alancutter wrote:
> > We will need the functionality of computed calc styles on background
position
> to
> > test animation of the property between px and % values.
> 
> I'm sorry Alan Cutter, Im not sure if I understood your point correctly. What
> test should I write here, wrt animation of property betn px & %?

Ah my bad, I meant we in the general Blink sense rather than this patch. Your
patch here will enable us to write animation tests that use getComputedStyle for
mixed unit types.

https://codereview.chromium.org/23651010/diff/1/Source/core/css/CSSComputedSt...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/23651010/diff/1/Source/core/css/CSSComputedSt...
Source/core/css/CSSComputedStyleDeclaration.cpp:567: return
cssValuePool().createValue(length);
The "if (length.isCalculated())" is not required here as
CSSPrimitiveValue::create(length, style) handles calc already.
This can just become "cssValuePool().createValue(length, style);".

[alancutter (OOO until 2018), 2013-10-08 11:23:27]

On 2013/10/08 11:20:08, alancutter wrote:
> This can just become "cssValuePool().createValue(length, style);".

I meant to say "return cssValuePool().createValue(length, style);".

[Srini, 2013-10-08 16:12:12]

On 2013/10/08 11:23:27, alancutter wrote:
> On 2013/10/08 11:20:08, alancutter wrote:
> > This can just become "cssValuePool().createValue(length, style);".
> 
> I meant to say "return cssValuePool().createValue(length, style);".

I'll fix that and submit. Thx Alan Cutter for the clarification.

[Srini, 2013-10-10 07:07:40]

eae: Can you check this patch up again? This patch makes us in par with Firefox.

[eae, 2013-10-10 15:31:25]

LGTM

[commit-bot: I haz the power, 2013-10-10 16:28:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....

[commit-bot: I haz the power, 2013-10-10 17:36:40]

Change committed as 159351