Fixing getComputedStyle to return pixel values for left / right / top / bottom

Fixing getComputedStyle to return pixel values for left / right / top / bottom

The returned object in CSS 2.1 "used values" (while CSS 2.0 used the
"computed values").
http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSview-getComputedStyle

The "used value" of any CSS property is the final value of that property
after all calculations have been performed. Dimensions are all in pixels.
http://www.w3.org/TR/CSS2/cascade.html#used-value

This is now consistent with both release Firefox and IE9.

BUG=229280
R=eseidel

[mithro-old, 2013-04-12 04:33:13]

Hi Eric,

I updated the change to have the refactoring you requested plus added a bunch of
comments which explains why something so simple is complicated.

Could you take another look and tell me what else needs to happen?

Tim

[eseidel, 2013-04-12 07:33:50]

other than the style nit, lgtm.

https://codereview.chromium.org/13871003/diff/4001/Source/WebCore/css/CSSComp...
File Source/WebCore/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/13871003/diff/4001/Source/WebCore/css/CSSComp...
Source/WebCore/css/CSSComputedStyleDeclaration.cpp:638: static Length
getOffsetComputedStyle(RenderStyle* style, CSSPropertyID propertyID) {
Blink style moves { to the next line.

[mithro-old, 2013-04-26 05:30:38]

Fixed.

Can you PTAL? If it's good can you commit?

https://codereview.chromium.org/13871003/diff/4001/Source/WebCore/css/CSSComp...
File Source/WebCore/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/13871003/diff/4001/Source/WebCore/css/CSSComp...
Source/WebCore/css/CSSComputedStyleDeclaration.cpp:638: static Length
getOffsetComputedStyle(RenderStyle* style, CSSPropertyID propertyID) {
On 2013/04/12 07:33:50, Eric Seidel (Google) wrote:
> Blink style moves { to the next line.

Done.

[commit-bot: I haz the power, 2013-04-26 05:34:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mithro@mithis.com/13871003/11001

[commit-bot: I haz the power, 2013-04-26 05:36:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mithro@mithis.com/13871003/11001

[commit-bot: I haz the power, 2013-04-26 05:40:08]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_layout_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-26 05:42:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mithro@mithis.com/13871003/11001

[commit-bot: I haz the power, 2013-04-26 05:44:52]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_layout_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-26 09:19:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mithro@mithis.com/13871003/16003

[commit-bot: I haz the power, 2013-04-26 09:43:55]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[mithro-old, 2013-07-23 12:52:20]

On 2013/04/26 09:43:55, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_layout_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

This change was never committed for complicated reasons, trying to document them
here for prosperity.

[mithro-old, 2013-07-23 12:55:19]

From: Timothy Robert Ansell <tansell@google.com>
Date: 29 April 2013 12:46
To: Tab Atkins <tabatkins@google.com>

Hi Tab,

Mike suggested you would be a good person to talk to about this.

I'm trying to get the fast/css/computed-offset-with-zoom.html test to work with
my fixes to getComputedStyle (see change
https://chromiumcodereview.appspot.com/13871003/ ). Our implementation has been
lagging the spec and there has been a bug open since 2009.

The test sets a relative box with the following;
            #test-fixed  {
                position: relative;
                left: 100px;
                right: 20px;
                top: 100px;
                bottom: 100px;
            }

Which according to the layout spec is overconstrained; and should ignore the
right value;
See http://www.w3.org/TR/CSS2/visuren.html#relative-positioning
>
> For relatively positioned elements, 'left' and 'right' move the box(es)
horizontally, without changing their size. 'Left' moves the boxes to the right,
and 'right' moves them to the left. Since boxes are not split or stretched as a
result of 'left' or 'right', the used values are always: left = -right. 

<snip> 
>
> If neither 'left' nor 'right' is 'auto', the position is over-constrained, and
one of them has to be ignored. If the 'direction' property of the containing
block is 'ltr', the value of 'left' wins and 'right' becomes -'left'. If
'direction' of the containing block is 'rtl', 'right' wins and 'left' is
ignored.

The spec also says that getComputedStyle should returned the "used value"
http://www.w3.org/TR/CSS2/cascade.html#used-value
>
> 6.1.3 Used values
> Computed values are processed as far as possible without formatting the
document. Some values, however, can only be determined when the document is
being laid out. For example, if the width of an element is set to be a certain
percentage of its containing block, the width cannot be determined until the
width of the containing block has been determined. The used value is the result
of taking the computed value and resolving any remaining dependencies into an
absolute value.


My reading of the spec is that the getComputedValue value for the right property
in the above should be -100px (as we ignore the right: 20px).

The problem is that Chrome, FireFox and IE all return 20px, IE the value which
was set.

I'm not sure how to resolve this problem, Mike suggested you might know better
what I should be doing.

Tim

----------
From: Tab Atkins <tabatkins@google.com>
Date: 30 April 2013 03:56
To: Timothy Robert Ansell <tansell@google.com>


Note - that paragraph only applies if the width is non-auto as well.
If 'width' is "auto", then non-auto 'left' and 'right' aren't
overconstrained, they're exactly constrained enough. ^_^

> The spec also says that getComputedStyle should returned the "used value"
> http://www.w3.org/TR/CSS2/cascade.html#used-value
>>
>> 6.1.3 Used values
>> Computed values are processed as far as possible without formatting the
>> document. Some values, however, can only be determined when the document is
>> being laid out. For example, if the width of an element is set to be a
>> certain percentage of its containing block, the width cannot be determined
>> until the width of the containing block has been determined. The used value
>> is the result of taking the computed value and resolving any remaining
>> dependencies into an absolute value.
>
> My reading of the spec is that the getComputedValue value for the right
> property in the above should be -100px (as we ignore the right: 20px).
>
> The problem is that Chrome, FireFox and IE all return 20px, IE the value
> which was set.
>
> I'm not sure how to resolve this problem, Mike suggested you might know
> better what I should be doing.

Oh jeez, the spec is *hilariously* underdefined here.  It never
actually mentions "used values" there; it just says "ignore the value
for 'left' and solve for it using the other values instead".  Good
job, past 2.1 editors.

I'll need to ask the WG for input.  I'll get back to you.

~TJ

----------
From: Tab Atkins <tabatkins@google.com>
Date: 30 April 2013 04:01
To: Timothy Robert Ansell <tansell@google.com>


On Mon, Apr 29, 2013 at 10:56 AM, Tab Atkins <tabatkins@google.com> wrote:
> Note - that paragraph only applies if the width is non-auto as well.
> If 'width' is "auto", then non-auto 'left' and 'right' aren't
> overconstrained, they're exactly constrained enough. ^_^

Hurp durp, I didn't read your email well enough.  You're talking about
relpos, not abspos.  Better response incoming.

~TJ

----------
From: Tab Atkins <tabatkins@google.com>
Date: 30 April 2013 04:05
To: Timothy Robert Ansell <tansell@google.com>


Okay, same level of undefinedness and weasel-wordiness.  Joy.  Back to
waiting for a WG response.

~TJ

----------
From: Timothy Robert Ansell <tansell@google.com>
Date: 30 April 2013 11:22
To: Tab Atkins <tabatkins@google.com>


So what should I do?

I believe my way is correct as specified, but maybe we should just do what
everyone else does?

Tim

----------
From: Tab Atkins <tabatkins@google.com>
Date: 3 May 2013 09:49
To: Timothy Robert Ansell <tansell@google.com>


On Mon, Apr 29, 2013 at 6:22 PM, Timothy Robert Ansell
<tansell@google.com> wrote:
> So what should I do?
>
> I believe my way is correct as specified, but maybe we should just do what
> everyone else does?

Sorry, I was discussing the issue in the mailing list.

It looks like, for bugwards compat, the way forward is:

If the *computed* value was "auto" and the computed value of the
opposite property was non-"auto", return the negation of the opposite
property's used value.  Otherwise, return the used value.

~TJ

----------
From: Timothy Robert Ansell <tansell@google.com>
Date: 31 May 2013 14:36
To: Tab Atkins <tabatkins@google.com>


Hi Tab,

I've been away for 3 weeks on holidays and want to get this committed. Is this
the final say on this?

If the *computed* value was "auto" and the computed value of the
opposite property was non-"auto", return the negation of the opposite
property's used value.  Otherwise, return the used value.

It sounds really confusing :(

Tim



----------
From: Tab Atkins <tabatkins@google.com>
Date: 1 June 2013 04:16
To: Timothy Robert Ansell <tansell@google.com>


On Thu, May 30, 2013 at 9:36 PM, Timothy Robert Ansell
<tansell@google.com> wrote:
> Hi Tab,
>
> I've been away for 3 weeks on holidays and want to get this committed. Is
> this the final say on this?
>
> If the *computed* value was "auto" and the computed value of the
> opposite property was non-"auto", return the negation of the opposite
> property's used value.  Otherwise, return the used value.
>
> It sounds really confusing :(

Unless we want to play chicken with compat bugs, yes, that's the final say.

~TJ