Bug 29084 - getComputedStyle returns percentage values for left / right / top / bottom
: getComputedStyle returns percentage values for left / right / top / bottom
Status: ASSIGNED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To: Tim 'mithro' Ansell
:
Depends on:
Blocks: 106632 110007
  Show dependency treegraph
 
Reported: 2009-09-09 07:07 PDT by Jake Archibald
Modified: 2014-06-11 02:20 PDT (History)
24 users (show)

See Also:


Attachments
test (3.64 KB, text/html)
2012-07-10 14:17 PDT, Ryosuke Niwa
no flags Details
work in progress (4.55 KB, patch)
2012-07-16 14:37 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (5.07 KB, patch)
2013-02-12 00:33 PST, Tim 'mithro' Ansell
no flags Details | Formatted Diff | Diff
Patch (7.29 KB, patch)
2013-02-13 01:01 PST, Tim 'mithro' Ansell
no flags Details | Formatted Diff | Diff
Patch (7.57 KB, patch)
2013-02-13 18:01 PST, Tim 'mithro' Ansell
no flags Details | Formatted Diff | Diff
Patch (21.32 KB, patch)
2013-02-14 21:03 PST, Tim 'mithro' Ansell
no flags Details | Formatted Diff | Diff
Patch (5.67 KB, patch)
2013-02-17 17:58 PST, Tim 'mithro' Ansell
no flags Details | Formatted Diff | Diff
Patch (23.27 KB, patch)
2013-02-18 04:19 PST, Tim 'mithro' Ansell
no flags Details | Formatted Diff | Diff
Patch (24.91 KB, patch)
2013-02-21 01:27 PST, Tim 'mithro' Ansell
no flags Details | Formatted Diff | Diff
Patch (23.85 KB, patch)
2013-02-21 02:21 PST, Tim 'mithro' Ansell
no flags Details | Formatted Diff | Diff
Patch (23.85 KB, patch)
2013-02-21 02:45 PST, Tim 'mithro' Ansell
no flags Details | Formatted Diff | Diff
Patch (47.46 KB, patch)
2013-02-21 20:07 PST, Tim 'mithro' Ansell
no flags Details | Formatted Diff | Diff
Patch (47.80 KB, patch)
2013-02-25 18:05 PST, Tim 'mithro' Ansell
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jake Archibald 2009-09-09 07:07:17 PDT
var div = document.createElement('div');
div.style.position = 'absolute';
div.style.top = '10%';
document.body.appendChild(div);
alert( window.getComputedStyle(div, null).top );

The above will alert '10%' rather than the pixel value.

Gecko / Presto will give the pixel value.

Webkit gives the pixel value for other properties (widht, height etc) and will give the pixel value for left / right / top / bottom if the set value isn't a percent, eg 10em.

The above bug exists in Safari, Chrome and Nightly r48199, although I have only tested on Windows.

Jake.
Comment 1 Tab Atkins 2011-10-19 20:52:52 PDT
'10%' is the correct computed value - percentages are resolved at "used value" time.  However, the getComputedStyle function is a bit of a misnomer, and actually returns a mix of computed and used values.

Which properties return what is underdefined right now.  Ideally we should wait for the CSSOM spec to define this, but in the absence of that, we can look at what other browsers do.

In particular, Firefox returns used values for these properties.  I think that's useful, and we should probably match them.
Comment 2 Mike Sherov 2011-11-21 14:59:27 PST
> In particular, Firefox returns used values for these properties.

Yes, FF, IE9, and Opera all use the "used value" instead of the "computed value".  

> I think that's useful, and we should probably match them.

Yes, it's immensely useful, and in particular, several popular javascript libraries have bugs open for it:
 * http://bugs.jquery.com/ticket/10639
 * http://yuilibrary.com/projects/yui3/ticket/2529799
Comment 3 Ryosuke Niwa 2011-11-21 15:48:08 PST
What does Trident do?
Comment 4 Tab Atkins 2011-11-21 15:56:10 PST
IE9 returns pixels, as Mike says in #2.
Comment 5 Ryosuke Niwa 2011-11-21 16:00:32 PST
Okay, then it appears that this is a no-brainer. We should just fix it.
Comment 6 Mike Sherov 2011-11-21 16:01:03 PST
And IE<9 doesn't even have getComputedStyle... But the nonstandard currentStyle returns % instead of pixels (computed vs. used)
Comment 7 Ryosuke Niwa 2011-11-21 16:04:00 PST
(In reply to comment #6)
> And IE<9 doesn't even have getComputedStyle... But the nonstandard currentStyle returns % instead of pixels (computed vs. used)

But that's a different API so it doesn't seem like a problem for us. I guess my only worry is that there might be some WebKit-only content that depends on this behavior but that's probably unlikely.
Comment 8 Mike Sherov 2011-11-21 16:13:01 PST
Sure, currentStyle is non-standard and is a different API, I was just trying to give the full Trident story :).

So I'm new to this... If I want to fix this myself, do I just go right to it in the webkit source code, paying attention to the guidelines according to the webkit.org site, or is there something else I should do first?
Comment 9 Xianzhu Wang 2011-11-21 18:51:50 PST
Bug 42799 is about the problem of pixelXXX CSS properties which seems a workaround of this bug. I think after we fix this bug we can drop the pixelXXX properties.

The current draft CSSOM says getComputedStyle() should return the resolved values, and for width, height etc., "if the property applies to the element or pseudo-element and the resolved value of the 'display' property is not none, the resolved value is the used value. Otherwise the resolved value is the computed value."
Comment 10 Mike Sherov 2011-11-21 19:11:35 PST
That makes sense. If it's display none or disconnected, pixel values (used values) aren't possible nor easy to correctly resolve, and so therefore should report computed values (cascaded values or whatever you want to call it).
Comment 11 Xianzhu Wang 2011-11-21 19:56:38 PST
(In reply to comment #10)
> That makes sense. If it's display none or disconnected, pixel values (used values) aren't possible nor easy to correctly resolve, and so therefore should report computed values (cascaded values or whatever you want to call it).

Sorry I made a mistake that thought the list of properties that getComputedStyle() may return the used value included left/right/top/bottom. Actually the draft (http://dev.w3.org/csswg/cssom/#resolved-value) implies for left/right/top/bottom getComputedStyle() should return the computed style.
Comment 12 Mike Sherov 2011-11-21 20:07:00 PST
I'm sorry for causing any confusion. This is perhaps the correct behavior, but then this exposes the fact that although top/left/right/bottom follow the draft spec, margin*, padding*, border* do not and FF, IE9, and Opera do follow the spec for margin*, padding*, and border*.  

Can we at least then bring those properties in line with the other browsers, as it would also match the draft?

Shall I create another ticket for those properties separately?
Comment 13 Mike Sherov 2011-11-21 20:19:38 PST
(In reply to comment #12)
> I'm sorry for causing any confusion. This is perhaps the correct behavior, but then this exposes the fact that although top/left/right/bottom follow the draft spec, margin*, padding*, border* do not and FF, IE9, and Opera do follow the spec for margin*, padding*, and border*.  

I meant padding and margin, not border. 

> 
> Can we at least then bring those properties in line with the other browsers, as it would also match the draft?
> 
> Shall I create another ticket for those properties separately?
Comment 14 Xianzhu Wang 2011-11-21 20:31:20 PST
(In reply to comment #12)
> Shall I create another ticket for those properties separately?

I prefer another bug for margin, padding, width/height because they should have no controversy.
Comment 15 Ryosuke Niwa 2011-11-22 11:07:58 PST
If IE, FF, & Opera all behave the same, then that must be the behavior we match, not the spec. In fact, we should amend the spec to match the reality.
Comment 16 Mike Sherov 2011-11-22 16:58:16 PST
Just to be complete here, this fiddle shows it all:

http://jsfiddle.net/u4F8m/5/embedded/result/

IE9, O, FF all have "used values" for the properties listed.

Webkit has "computed values" for margin-*,top/left/right/bottom

margin-* is clearly a bug, as it neither matches the other browsers nor the spec.

top/left/right/bottom match the spec, and not the other browsers, and I believe the spec should be amended.
Comment 17 Mike Sherov 2011-11-23 17:38:52 PST
Looks like this behavior is intentional, and was "fixed" to do this behavior in: https://bugs.webkit.org/show_bug.cgi?id=13343

Can someone please weigh in here? It seems as if CSS2.1 and CSS3 are in disagreement.
Comment 18 Mike Sherov 2012-03-17 14:02:18 PDT
So I posted on www-style about this, and it seemed as if "used value" is the way to go here. I submitted a bug to the editor of CSSOM about amending the spec: https://www.w3.org/Bugs/Public/show_bug.cgi?id=16389

Although the spec currently says computed value for top/left/bottom/right, I think changing to used value is the right move, for the reasons I mention in this thread, on www-style, and in the CSSOM bug report. 

This change would be a big win for interopability and for JS libs that attempt to provide interoperability like jQuery and YUI. 

Thoughts?
Comment 19 Tab Atkins 2012-03-19 08:37:04 PDT
(In reply to comment #18)
> So I posted on www-style about this, and it seemed as if "used value" is the way to go here. I submitted a bug to the editor of CSSOM about amending the spec: https://www.w3.org/Bugs/Public/show_bug.cgi?id=16389
> 
> Although the spec currently says computed value for top/left/bottom/right, I think changing to used value is the right move, for the reasons I mention in this thread, on www-style, and in the CSSOM bug report. 
> 
> This change would be a big win for interopability and for JS libs that attempt to provide interoperability like jQuery and YUI. 
> 
> Thoughts?

I say do it.  You've interacted on the list and submitted a bug report against the spec, which has an active editor who will take care of it.  It seems clear that the right solution is to fix the spec to match browsers here, and we should adjust ourselves to be in line with that.
Comment 20 Mike Sherov 2012-06-22 05:33:14 PDT
I know I'd previously said I'd like to take a shot on this bug, but it's a bit over my head at the moment. 

In the meantime, I'd just like to ping this ticket with an update on the frequency with which this is reported as a bug to the jQuery bug tracker, in an attempt to get the priority ticket of this raised, and to get someone more familiar with the source to work on it:

http://bugs.jquery.com/ticket/11932
http://bugs.jquery.com/ticket/11954
http://bugs.jquery.com/ticket/9505
Comment 21 Ryosuke Niwa 2012-06-22 14:05:46 PDT
I'm sorry, what we're supposed to do here? Is the percentage values correct or incorrect?
Comment 22 Mike Sherov 2012-06-22 14:11:35 PDT
top/left/right/bottom should return pixel values, even if percentage is specified, to match the other browsers. Yes, this is against the current spec, but there has been significant discussion on the W3C ML about it, and a ticket filed against the spec to amend.

Thanks for your reply!
Comment 23 Ryosuke Niwa 2012-07-06 22:55:36 PDT
I think we just need to tweak getPositionOffsetValue in http://trac.webkit.org/browser/trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp

I'll get to get to it next week.
Comment 24 Mike Sherov 2012-07-09 18:28:29 PDT
(In reply to comment #23)
> I think we just need to tweak getPositionOffsetValue in http://trac.webkit.org/browser/trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp
> 
> I'll get to get to it next week.

Looks that way. That's so much for getting to this!
Comment 25 Ryosuke Niwa 2012-07-10 14:17:03 PDT
Created attachment 151525 [details]
test
Comment 26 Mike Sherov 2012-07-11 18:20:10 PDT
Those tests look good to me.

If you allow me to digress for a moment, what's interesting is that running them in the different browsers show just how divergent the implementation is amongst browsers:

Opera 11.6: Passes all tests as written
FF13: Converts "auto" to pixels so fails when expecting "auto".
IE9/IE10: incorrectly accounts for padding so fails on the padding tests.
Webkit: doesn't convert to pixels so fails when expecting pixels.

Now, if we really wanted to go all the way with this, I personally believe that the FF behavior is most useful, and most accurately fits the idea of returning used value (that is, converting "auto" to actual pixels, even if not specified"). 

However, to achieve the most consistency is to expect "auto", as you have written here.
Comment 27 Ryosuke Niwa 2012-07-11 20:01:06 PDT
(In reply to comment #26)
> Opera 11.6: Passes all tests as written
> FF13: Converts "auto" to pixels so fails when expecting "auto".
> IE9/IE10: incorrectly accounts for padding so fails on the padding tests.
> Webkit: doesn't convert to pixels so fails when expecting pixels.
> 
> Now, if we really wanted to go all the way with this, I personally believe that the FF behavior is most useful, and most accurately fits the idea of returning used value (that is, converting "auto" to actual pixels, even if not specified"). 
> 
> However, to achieve the most consistency is to expect "auto", as you have written here.

Yeah, I'm not sure what the correct "expected" behavior is. Intuitively FF or Opera seem to do well but given that IE and Opera both return "auto" for unspecified values, returning "auto" here seems like a good idea.

We need some tests for vertical writing mode and RTL pages.
Comment 28 Mike Sherov 2012-07-11 20:17:06 PDT
According to http://www.w3.org/TR/CSS2/cascade.html#used-value, " The used value is the result of taking the computed value and resolving any remaining dependencies into an absolute value." and then subsequently "A used value is in principle the value used for rendering,"

Is "auto" an absolute value or the value used for rendering? I don't personally believe so.
Is "auto" a useful value to return? I personally don't believe so.
Is "auto" consistent with MOST of the other browsers? yes.

What I really care about here is that the percentage stuff gets fixed. Perhaps we can save any decision about "auto" for another ticket?
Comment 29 Ryosuke Niwa 2012-07-11 20:34:05 PDT
(In reply to comment #28)
> According to http://www.w3.org/TR/CSS2/cascade.html#used-value, " The used value is the result of taking the computed value and resolving any remaining dependencies into an absolute value." and then subsequently "A used value is in principle the value used for rendering,"
> 
> Is "auto" an absolute value or the value used for rendering? I don't personally believe so.
> Is "auto" a useful value to return? I personally don't believe so.
> Is "auto" consistent with MOST of the other browsers? yes.
> 
> What I really care about here is that the percentage stuff gets fixed. Perhaps we can save any decision about "auto" for another ticket?

Let's add this information on https://www.w3.org/Bugs/Public/show_bug.cgi?id=16389 and see what others think. We should at least provide the relevant information there before deciding what to do. e.g. other browser vendors might be in the process of changing their behaviors.
Comment 30 Mike Sherov 2012-07-11 20:40:38 PDT
I added the info to the ticket. I'll also post about this on www-style to see if I can get an answer.
Comment 31 Mike Sherov 2012-07-11 23:41:40 PDT
posted on the ML about this: http://lists.w3.org/Archives/Public/www-style/2012Jul/0277.html
Comment 32 Mike Sherov 2012-07-12 05:30:00 PDT
This pretty much says it all: http://lists.w3.org/Archives/Public/www-style/2012Jul/0284.html

That is: FF13 is the correct behavior, "auto" should be converted to pixel.
Comment 33 Ryosuke Niwa 2012-07-16 14:37:46 PDT
Created attachment 152616 [details]
work in progress
Comment 34 Mike Sherov 2012-08-01 09:00:20 PDT
by the way, this was added to the CSSOM spec on 7/17/12: http://dev.w3.org/csswg/cssom/#resolved-values
Comment 35 WebKit Review Bot 2012-11-24 20:38:04 PST
Comment on attachment 152616 [details]
work in progress

Attachment 152616 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14991111
Comment 36 EFL EWS Bot 2012-11-24 20:38:27 PST
Comment on attachment 152616 [details]
work in progress

Attachment 152616 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14981219
Comment 37 Peter Beverloo (cr-android ews) 2012-11-24 20:43:24 PST
Comment on attachment 152616 [details]
work in progress

Attachment 152616 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14975800
Comment 38 Build Bot 2012-11-24 20:48:36 PST
Comment on attachment 152616 [details]
work in progress

Attachment 152616 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14970948
Comment 39 Early Warning System Bot 2012-11-24 20:55:06 PST
Comment on attachment 152616 [details]
work in progress

Attachment 152616 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14989147
Comment 40 Build Bot 2012-11-24 21:03:31 PST
Comment on attachment 152616 [details]
work in progress

Attachment 152616 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14986153
Comment 41 Early Warning System Bot 2012-11-24 21:14:15 PST
Comment on attachment 152616 [details]
work in progress

Attachment 152616 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14983188
Comment 42 Mike Sherov 2012-12-28 11:03:43 PST
Is there anything I can do to help move this along besides actually authoring a patch myself? I'd like to get involved but don't know per se where to start.
Comment 43 Tim 'mithro' Ansell 2013-02-12 00:33:21 PST
Created attachment 187794 [details]
Patch
Comment 44 WebKit Review Bot 2013-02-12 01:40:13 PST
Comment on attachment 187794 [details]
Patch

Attachment 187794 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16493740

New failing tests:
css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html
fast/css/getComputedStyle/computed-style.html
svg/css/getComputedStyle-basic.xhtml
jquery/offset.html
fast/css/getComputedStyle/computed-style-without-renderer.html
fast/css/hover-affects-child.html
Comment 45 Build Bot 2013-02-12 15:22:40 PST
Comment on attachment 187794 [details]
Patch

Attachment 187794 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16517398

New failing tests:
css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html
jquery/offset.html
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html
fast/css/hover-affects-child.html
Comment 46 Build Bot 2013-02-12 23:56:23 PST
Comment on attachment 187794 [details]
Patch

Attachment 187794 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16531400

New failing tests:
css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html
jquery/offset.html
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html
fast/css/hover-affects-child.html
Comment 47 Tim 'mithro' Ansell 2013-02-13 01:01:38 PST
Created attachment 188032 [details]
Patch
Comment 48 WebKit Review Bot 2013-02-13 01:05:43 PST
Attachment 188032 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSCalculationValue.cpp', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSPrimitiveValue.cpp']" exit_code: 1
Source/WebCore/css/CSSCalculationValue.cpp:206:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1533:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 49 WebKit Review Bot 2013-02-13 01:28:31 PST
Comment on attachment 188032 [details]
Patch

Attachment 188032 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16530034

New failing tests:
animations/duplicated-keyframes-name.html
animations/keyframes-infinite-iterations.html
animations/animation-direction.html
animations/longhand-timing-function.html
animations/missing-from-to.html
animations/keyframe-timing-functions2.html
animations/fill-mode-removed.html
animations/fill-mode-multiple-keyframes.html
animations/missing-keyframe-properties-repeating.html
animations/animation-direction-alternate-reverse.html
animations/keyframes.html
animations/change-keyframes.html
animations/fill-mode.html
animations/fill-mode-reverse.html
animations/animation-direction-reverse-timing-functions.html
animations/change-keyframes-name.html
animations/fill-mode-iteration-count-non-integer.html
compositing/animation/animated-composited-inside-hidden.html
animations/keyframe-timing-functions.html
animations/animation-direction-reverse-non-hardware.html
animations/missing-keyframe-properties-timing-function.html
animations/missing-values-first-keyframe.html
animations/import.html
animations/fill-mode-missing-from-to-keyframes.html
animations/missing-keyframe-properties.html
animations/keyframes-out-of-order.html
animations/keyframes-comma-separated.html
animations/animation-direction-reverse-fill-mode.html
animations/change-one-anim.html
animations/generic-from-to.html
Comment 50 Build Bot 2013-02-13 02:21:06 PST
Comment on attachment 188032 [details]
Patch

Attachment 188032 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16536050

New failing tests:
css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html
animations/duplicated-keyframes-name.html
animations/keyframes-infinite-iterations.html
animations/animation-direction.html
animations/longhand-timing-function.html
animations/missing-from-to.html
animations/keyframe-timing-functions2.html
animations/fill-mode-removed.html
animations/fill-mode-multiple-keyframes.html
animations/missing-keyframe-properties-repeating.html
animations/animation-direction-alternate-reverse.html
animations/keyframes.html
animations/change-keyframes.html
animations/fill-mode.html
animations/fill-mode-reverse.html
animations/animation-direction-reverse-timing-functions.html
animations/change-keyframes-name.html
animations/fill-mode-iteration-count-non-integer.html
compositing/animation/animated-composited-inside-hidden.html
animations/keyframe-timing-functions.html
animations/animation-direction-reverse-non-hardware.html
animations/missing-keyframe-properties-timing-function.html
animations/import.html
animations/fill-mode-missing-from-to-keyframes.html
animations/missing-keyframe-properties.html
animations/keyframes-out-of-order.html
animations/keyframes-comma-separated.html
animations/animation-direction-reverse-fill-mode.html
animations/change-one-anim.html
animations/generic-from-to.html
Comment 51 Build Bot 2013-02-13 06:06:41 PST
Comment on attachment 188032 [details]
Patch

Attachment 188032 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16527146

New failing tests:
animations/duplicated-keyframes-name.html
animations/keyframes-infinite-iterations.html
animations/animation-direction.html
animations/longhand-timing-function.html
animations/missing-from-to.html
animations/keyframe-timing-functions2.html
animations/fill-mode-removed.html
animations/fill-mode-multiple-keyframes.html
animations/missing-keyframe-properties-repeating.html
animations/animation-direction-alternate-reverse.html
animations/keyframes.html
animations/change-keyframes.html
animations/fill-mode.html
animations/fill-mode-reverse.html
animations/animation-direction-reverse-timing-functions.html
animations/change-keyframes-name.html
animations/fill-mode-iteration-count-non-integer.html
compositing/animation/animated-composited-inside-hidden.html
animations/keyframe-timing-functions.html
animations/animation-direction-reverse-non-hardware.html
animations/import.html
animations/fill-mode-missing-from-to-keyframes.html
animations/keyframes-out-of-order.html
animations/keyframes-comma-separated.html
animations/animation-direction-reverse-fill-mode.html
animations/change-one-anim.html
animations/generic-from-to.html
Comment 52 Tim 'mithro' Ansell 2013-02-13 18:01:11 PST
Created attachment 188242 [details]
Patch
Comment 53 WebKit Review Bot 2013-02-13 18:07:11 PST
Attachment 188242 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSCalculationValue.cpp', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSPrimitiveValue.cpp']" exit_code: 1
Source/WebCore/css/CSSCalculationValue.cpp:206:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1533:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 54 WebKit Review Bot 2013-02-13 19:04:04 PST
Comment on attachment 188242 [details]
Patch

Attachment 188242 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16570144

New failing tests:
animations/duplicated-keyframes-name.html
animations/multiple-animations.html
animations/keyframes-infinite-iterations.html
animations/animation-direction.html
animations/longhand-timing-function.html
animations/missing-from-to.html
animations/keyframe-timing-functions2.html
animations/fill-mode-removed.html
animations/multiple-animations-timing-function.html
animations/fill-mode-multiple-keyframes.html
animations/animation-direction-alternate-reverse.html
animations/keyframes.html
animations/change-keyframes.html
animations/multiple-keyframes.html
animations/simultaneous-start-left.html
animations/fill-mode.html
animations/fill-mode-reverse.html
animations/fill-mode-iteration-count-non-integer.html
compositing/animation/animated-composited-inside-hidden.html
animations/keyframe-timing-functions.html
animations/missing-keyframe-properties-timing-function.html
animations/import.html
animations/fill-mode-missing-from-to-keyframes.html
animations/missing-keyframe-properties.html
animations/keyframes-out-of-order.html
animations/keyframes-comma-separated.html
animations/animation-direction-reverse-fill-mode.html
animations/negative-delay.html
animations/change-one-anim.html
animations/generic-from-to.html
Comment 55 Tim 'mithro' Ansell 2013-02-14 21:03:28 PST
Created attachment 188473 [details]
Patch
Comment 56 WebKit Review Bot 2013-02-14 22:39:09 PST
Comment on attachment 188473 [details]
Patch

Attachment 188473 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16559807

New failing tests:
css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html
editing/pasteboard/drag-drop-input-textarea.html
editing/deleting/delete-all-text-in-text-field-assertion.html
editing/shadow/insertorderedlist-crash.html
css3/flexbox/css-properties.html
editing/pasteboard/drop-text-events.html
editing/pasteboard/copy-crash.html
fast/css-grid-layout/grid-columns-rows-get-set.html
animations/animation-border-overflow.html
editing/selection/delete-word-granularity-text-control.html
editing/input/set-value-on-input-and-delete.html
editing/selection/drag-text-delay.html
editing/pasteboard/drag-drop-url-text.html
editing/pasteboard/paste-plaintext-nowrap.html
editing/input/ime-composition-clearpreedit.html
editing/style/style-3998892-fix.html
fast/css/getComputedStyle/computed-style-display-none.html
editing/style/apple-style-editable-mix.html
editing/pasteboard/drop-inputtext-acquires-style.html
editing/style/apply-style-join-child-text-nodes-crash.html
editing/style/iframe-onload-crash-mac.html
editing/shadow/bold-twice-in-shadow.html
editing/pasteboard/copy-paste-first-line-in-textarea.html
fast/css-grid-layout/grid-columns-rows-get-set-multiple.html
fast/css/getComputedStyle/computed-style-without-renderer.html
editing/pasteboard/paste-placeholder-input.html
editing/deleting/5290534.html
editing/pasteboard/drag-drop-input-in-svg.svg
editing/style/table-selection.html
fast/css/counters/counter-after-style-crash.html
Comment 57 Build Bot 2013-02-14 23:38:09 PST
Comment on attachment 188473 [details]
Patch

Attachment 188473 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16538990

New failing tests:
css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html
editing/pasteboard/4944770-2.html
editing/deleting/delete-all-text-in-text-field-assertion.html
editing/style/apply-style-join-child-text-nodes-crash.html
css3/flexbox/css-properties.html
fast/css/lang-selector-empty-attribute.xhtml
fast/css/hover-affects-child.html
animations/animation-add-events-in-handler.html
editing/style/table-selection.html
fast/css-grid-layout/grid-columns-rows-get-set.html
editing/selection/delete-word-granularity-text-control.html
editing/input/set-value-on-input-and-delete.html
editing/input/ime-composition-clearpreedit.html
fast/css/getComputedStyle/computed-style-display-none.html
editing/pasteboard/copy-display-none.html
editing/style/iframe-onload-crash-mac.html
http/tests/misc/acid3.html
canvas/philip/tests/2d.text.draw.fontface.notinpage.html
fast/css-grid-layout/grid-columns-rows-get-set-multiple.html
editing/pasteboard/4944770-1.html
editing/deleting/5290534.html
editing/pasteboard/copy-crash.html
editing/pasteboard/4641033.html
Comment 58 Robert Hogan 2013-02-15 07:22:35 PST
Hi Tim,

There are a couple of things I think you are doing wrong on this bug.


Firstly, you need to run the test suite locally rather than relying on the bots to find failures. It is possible to get chromium-linux to build and run the tests cleanly on your own machine if you run linux. I believe the same is true of the Mac build. You need to invest a bit of time in this upfront it will mean you can be productive on this and other bugs.

Secondly, your patch needs to have tests of its own. I haven't looked closely at the change you're making but it definitely needs tests.

We remove code rather than comment it out. 

Your change needs a detailed changelog as it is substantial and requires explanation, both overall and for particular changes like commenting out asserts!

Finally unnecessary whitespace changes can often result in a r- on their own.
Comment 59 WebKit Review Bot 2013-02-15 12:21:01 PST
Attachment 188473 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSCalculationValue.cpp', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSPrimitiveValue.cpp']" exit_code: 1
Source/WebCore/css/CSSCalculationValue.cpp:207:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/CSSComputedStyleDeclaration.cpp:597:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/css/CSSComputedStyleDeclaration.cpp:598:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1531:  max_width is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1532:  max_height is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1558:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 6 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 60 Darin Adler 2013-02-15 15:57:19 PST
Comment on attachment 188473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188473&action=review

Thanks for tackling this!

The change log for this patch is unacceptably empty. You need to explain what you are doing and why, not just cite the name of the bug.

Also, patches that fix bugs need to include test cases that show the bug is fixed and that would fail without the bug fix.

> Source/WebCore/css/CSSCalculationValue.cpp:188
> +//            ASSERT_NOT_REACHED();

I assume this was included with the patch by accident, since the change log doesn’t mention it. If not, we should discuss why you made this change. And it’s definitely incorrect to change it by commenting out the assertion.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:582
> -static PassRefPtr<CSSValue> zoomAdjustedPixelValueForLength(const Length& length, const RenderStyle* style)
> +static PassRefPtr<CSSValue> zoomAdjustedPixelValueForLengthOld(const Length& length, const RenderStyle* style)

You can’t just add the word old to the end of the function without explaining it, at least in change log. Please explain your rationale here.
Comment 61 Darin Adler 2013-02-15 16:01:20 PST
Comment on attachment 188473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188473&action=review

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:582
>> +static PassRefPtr<CSSValue> zoomAdjustedPixelValueForLengthOld(const Length& length, const RenderStyle* style)
> 
> You can’t just add the word old to the end of the function without explaining it, at least in change log. Please explain your rationale here.

Once we can clearly explain and understand the difference between the old and new versions of this function, we can presumably choose an appropriate name for the old one, other than “old”. Unless the plan is to eliminate the old function entirely, in which case we need to at least explain that direction in this patch here.
Comment 62 Tim 'mithro' Ansell 2013-02-17 17:57:56 PST
Sorry, this patch wasn't yet ready for review. I'll mark if review? when it's ready.
Comment 63 Tim 'mithro' Ansell 2013-02-17 17:58:15 PST
Created attachment 188785 [details]
Patch
Comment 64 Ryosuke Niwa 2013-02-17 18:02:06 PST
Comment on attachment 188785 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188785&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

We definitely need a new layout test for this. r- due to lack of tests.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:669
> +                maxWidth = renderer->containingBlock()->availableWidth();
> +                break;
> +            case CSSPropertyTop:
> +            case CSSPropertyBottom:
> +                maxWidth = renderer->containingBlock()->availableHeight();

Does this work in vertical writing mode? We need a test.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:672
> +            default: {
> +            }}

Please replace {} with a break.
Comment 65 Tim 'mithro' Ansell 2013-02-17 18:03:02 PST
Totally reworked the patch to be much less invasive. Still need to fix the remaining tests which fail because getComputedStyle now always returns pixels.
Comment 66 WebKit Review Bot 2013-02-18 02:54:16 PST
Comment on attachment 188785 [details]
Patch

Attachment 188785 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16613098

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/hover-affects-child.html
jquery/offset.html
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html
Comment 67 Tim 'mithro' Ansell 2013-02-18 04:19:29 PST
Created attachment 188845 [details]
Patch
Comment 68 WebKit Review Bot 2013-02-18 17:55:32 PST
Comment on attachment 188845 [details]
Patch

Attachment 188845 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16614449

New failing tests:
editing/pasteboard/paste-match-style-002.html
animations/multiple-animations.html
fast/css-generated-content/pseudo-animation.html
editing/execCommand/query-font-size-with-typing-style.html
animations/longhand-timing-function.html
animations/missing-from-to.html
editing/execCommand/infinite-recursion-computeRectForRepaint.html
animations/fill-mode-removed.html
animations/multiple-animations-timing-function.html
animations/fill-mode-multiple-keyframes.html
fast/css-generated-content/pseudo-transition.html
animations/change-keyframes.html
animations/multiple-keyframes.html
editing/pasteboard/paste-match-style-001.html
animations/simultaneous-start-left.html
animations/timing-functions.html
animations/fill-mode.html
animations/fill-mode-reverse.html
animations/fill-mode-iteration-count-non-integer.html
compositing/animation/animated-composited-inside-hidden.html
editing/style/remove-underline-after-paragraph.html
animations/missing-keyframe-properties-timing-function.html
animations/fill-mode-missing-from-to-keyframes.html
editing/style/remove-underline-after-paragraph-in-bold.html
animations/missing-keyframe-properties.html
editing/inserting/insert-div-024.html
animations/animation-direction-reverse-fill-mode.html
editing/inserting/insert-div-026.html
animations/change-one-anim.html
animations/generic-from-to.html
Comment 69 Build Bot 2013-02-19 10:57:01 PST
Comment on attachment 188845 [details]
Patch

Attachment 188845 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16612895

New failing tests:
animations/multiple-animations.html
fast/css-generated-content/pseudo-animation.html
animations/longhand-timing-function.html
animations/missing-from-to.html
animations/fill-mode-reverse.html
animations/fill-mode-removed.html
animations/multiple-animations-timing-function.html
animations/fill-mode-multiple-keyframes.html
fast/css-generated-content/pseudo-transition.html
animations/change-keyframes.html
animations/multiple-keyframes.html
animations/simultaneous-start-left.html
animations/timing-functions.html
animations/fill-mode.html
fast/css/getComputedStyle/computed-style-negative-top.html
animations/fill-mode-iteration-count-non-integer.html
compositing/animation/animated-composited-inside-hidden.html
animations/missing-keyframe-properties-timing-function.html
canvas/philip/tests/2d.text.draw.fontface.notinpage.html
animations/fill-mode-missing-from-to-keyframes.html
animations/missing-keyframe-properties.html
animations/animation-direction-reverse-fill-mode.html
animations/generic-from-to.html
Comment 70 Tim 'mithro' Ansell 2013-02-21 01:27:30 PST
Created attachment 189471 [details]
Patch
Comment 71 Tim 'mithro' Ansell 2013-02-21 01:30:20 PST
Third attempt at trying to get this right.
Comment 72 EFL EWS Bot 2013-02-21 01:36:21 PST
Comment on attachment 189471 [details]
Patch

Attachment 189471 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16652878
Comment 73 Early Warning System Bot 2013-02-21 01:37:48 PST
Comment on attachment 189471 [details]
Patch

Attachment 189471 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16653906
Comment 74 Early Warning System Bot 2013-02-21 01:39:35 PST
Comment on attachment 189471 [details]
Patch

Attachment 189471 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16650929
Comment 75 WebKit Review Bot 2013-02-21 01:43:07 PST
Comment on attachment 189471 [details]
Patch

Attachment 189471 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16647933
Comment 76 WebKit Review Bot 2013-02-21 01:52:15 PST
Comment on attachment 189471 [details]
Patch

Attachment 189471 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16651904
Comment 77 Build Bot 2013-02-21 01:56:38 PST
Comment on attachment 189471 [details]
Patch

Attachment 189471 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16653907
Comment 78 Tim 'mithro' Ansell 2013-02-21 02:21:16 PST
Created attachment 189487 [details]
Patch
Comment 79 Tim 'mithro' Ansell 2013-02-21 02:45:44 PST
Created attachment 189490 [details]
Patch
Comment 80 Tim 'mithro' Ansell 2013-02-21 02:47:14 PST
Less crashy version of the patch.
Comment 81 WebKit Review Bot 2013-02-21 03:22:21 PST
Comment on attachment 189490 [details]
Patch

Attachment 189490 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16688051

New failing tests:
fast/css/getComputedStyle/getComputedStyle-zoom-and-background-size.html
jquery/offset.html
Comment 82 Build Bot 2013-02-21 03:30:57 PST
Comment on attachment 189490 [details]
Patch

Attachment 189490 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16656954

New failing tests:
fast/css/getComputedStyle/getComputedStyle-zoom-and-background-size.html
jquery/offset.html
Comment 83 Alexis Menard (darktears) 2013-02-21 03:35:49 PST
Comment on attachment 189490 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189490&action=review

> Source/WebCore/ChangeLog:7
> +

You need to explain the change here. You also need to give the link to the spec where this is specified.
Comment 84 Tim 'mithro' Ansell 2013-02-21 15:55:42 PST
Added the following description; will upload again once I get the final two tests to pass.

         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.
+
Comment 85 Tim 'mithro' Ansell 2013-02-21 20:07:56 PST
Created attachment 189665 [details]
Patch
Comment 86 Tim 'mithro' Ansell 2013-02-21 20:09:41 PST
This patch should finally be ready for review. The tests have been extended quite a bit and now they should also all pass.

Could you guys please take a look?
Comment 87 Elliott Sprehn 2013-02-21 22:33:52 PST
Comment on attachment 189665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189665&action=review

Looks pretty good, just a couple things. I take it you tested this in Firefox and got the same results?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:640
> +    // If the element is not displayed; return the "computed value" rather then the "used value".

Can you put a link to the spec here? That's super useful in the code. :)

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:657
> +    renderView->layout();

This is wrong, you just want to change isLayoutDependentProperty to include these properties.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:680
> +            return zoomAdjustedPixelValue(renderer->containingBlock()->clientHeight() - (box->offsetTop()+box->offsetHeight()) - box->marginBottom(), style);

containingBLock->clientHeight(), You already know the containing block from above, asking for it again is expensive. Also add a space around the +, I completely missed that there's addition in there at first.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:684
> +            return zoomAdjustedPixelValue(renderer->containingBlock()->clientWidth() - (box->offsetLeft()+box->offsetWidth()) - box->marginRight(), style);

Same
Comment 88 Benjamin Poulain 2013-02-22 01:46:25 PST
Comment on attachment 189665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189665&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:651
> +        switch (propertyID) {
>          case CSSPropertyLeft:
> -            l = style->left();
> -            break;
> +            return zoomAdjustedPixelValueForLength(style->left(), style);
>          case CSSPropertyRight:
> -            l = style->right();
> -            break;
> +            return zoomAdjustedPixelValueForLength(style->right(), style);
>          case CSSPropertyTop:
> -            l = style->top();
> -            break;
> +            return zoomAdjustedPixelValueForLength(style->top(), style);
>          case CSSPropertyBottom:
> -            l = style->bottom();
> -            break;
> +            return zoomAdjustedPixelValueForLength(style->bottom(), style);
>          default:

For the "in-code documentation" sake, it could be good to turn this into a static function.
E.g.: 
if (!renderView || !renderer || !renderer->isBox())
    return positionOffsetStyleValueForProperty(propertyID, style);

I think by copying zoomAdjustedPixelValueForLength() in every case:, you created less flexible code.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:673
> +        switch (propertyID) {
> +        case CSSPropertyTop:
> +            return zoomAdjustedPixelValue(box->relativePositionOffset().height(), style);
> +        case CSSPropertyBottom:
> +            return zoomAdjustedPixelValue(-(box->relativePositionOffset().height()), style);
> +        case CSSPropertyLeft:
> +            return zoomAdjustedPixelValue(box->relativePositionOffset().width(), style);
> +        case CSSPropertyRight:
> +            return zoomAdjustedPixelValue(-(box->relativePositionOffset().width()), style);
> +        default:
> +            ASSERT_NOT_REACHED();
> +        }

Ditto.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:678
> +        switch (propertyID) {
> +        case CSSPropertyTop:
> +            return zoomAdjustedPixelValue(box->offsetTop() - box->marginTop(), style);

Ditto.

> LayoutTests/fast/css/getComputedStyle/computed-style-negative-top.html:41
> -        result.appendChild(document.createTextNode("Test succeeded! Top is " + style.top + "."));
> +        result.appendChild(document.createTextNode("Test succeeded! top is " + style.top + "."));
> +    else
> +        result.appendChild(document.createTextNode("Test failed! top is " + style.top + "."));
> +
> +    result.appendChild(document.createElement('br'));
> +
> +    if (style.left == "-2px")
> +        result.appendChild(document.createTextNode("Test succeeded! left is " + style.left + "."));
> +    else
> +        result.appendChild(document.createTextNode("Test failed! left is " + style.left + "."));
> +
> +    result.appendChild(document.createElement('br'));
> +
> +    if (style.bottom == "1px")
> +        result.appendChild(document.createTextNode("Test succeeded! bottom is " + style.bottom + "."));
> +    else
> +        result.appendChild(document.createTextNode("Test failed! bottom is " + style.bottom + "."));
> +
> +    result.appendChild(document.createElement('br'));
> +
> +    if (style.right == "2px")
> +        result.appendChild(document.createTextNode("Test succeeded! right is " + style.right + "."));

It is a good opportunity to convert this test to js-test-pre.js
Comment 89 Tim 'mithro' Ansell 2013-02-24 15:53:27 PST
Comment on attachment 189665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189665&action=review

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:651
>>          default:
> 
> For the "in-code documentation" sake, it could be good to turn this into a static function.
> E.g.: 
> if (!renderView || !renderer || !renderer->isBox())
>     return positionOffsetStyleValueForProperty(propertyID, style);
> 
> I think by copying zoomAdjustedPixelValueForLength() in every case:, you created less flexible code.

I'm not sure what you are asking here? Changing the code to

switch(propertyID) {
case CSSPropertyLeft:
  if (!renderView || !renderer || !renderer->isBox()) {
    return zoomAdjustedPixelValueForLength(style->bottom(), style);
  } else if (box->isRelPositioned() || !containingBlock) {
    return zoomAdjustedPixelValue(box->relativePositionOffset().width(), style);
  } else {
    return zoomAdjustedPixelValue(box->offsetTop() - box->marginTop(), style);
  }
  ASSERT_NOT_REACHED();

case CSSPropertyRight:
  if (!renderView || !renderer || !renderer->isBox()) {
    return zoomAdjustedPixelValueForLength(style->right(), style);
  } else if (box->isRelPositioned() || !containingBlock) {
    return zoomAdjustedPixelValue(-(box->relativePositionOffset().width()), style);
  } else {
    return zoomAdjustedPixelValue(containingBlock()->clientWidth() - (box->offsetLeft()+box->offsetWidth()) - box->marginRight(), style);
  }
  ASSERT_NOT_REACHED();
....

??
Comment 90 Benjamin Poulain 2013-02-24 22:52:54 PST
No no no!! That'd be worse!

Which part was not clear?
Comment 91 Tim 'mithro' Ansell 2013-02-25 01:49:11 PST
You said;

> For the "in-code documentation" sake, it could be good to turn this into a static function.
> E.g.: 
> if (!renderView || !renderer || !renderer->isBox())
>     return positionOffsetStyleValueForProperty(propertyID, style);
> 
> I think by copying zoomAdjustedPixelValueForLength() in every case:, you created less flexible code.

I don't understand which code you are indicating to become the static function...
Comment 92 Benjamin Poulain 2013-02-25 02:42:20 PST
(In reply to comment #91)
> You said;
> 
> > For the "in-code documentation" sake, it could be good to turn this into a static function.
> > E.g.: 
> > if (!renderView || !renderer || !renderer->isBox())
> >     return positionOffsetStyleValueForProperty(propertyID, style);
> > 
> > I think by copying zoomAdjustedPixelValueForLength() in every case:, you created less flexible code.
> 
> I don't understand which code you are indicating to become the static function...

Okay. They were two separate comment.

1) If you move each switch-case into its own separate static function, you will make the code of getPositionOffsetValue() easier to follow. Even more so if you can find a good name for each group.

2) By copying zoomAdjustedPixelValueForLength to each case instead of applying it to a Length value, you make the code less flexible. Every future change will have to update 4 sites.

For example
    static inline PassRefPtr<CSSValue>positionOffsetStyleValueForProperty(CSSPropertyID propertyID, RenderStyle* style)
    {
        Length length;
        switch(propertyID) {
        case CSSPropertyLeft:
            length = style->left()
            break;
        [...]
        }
        return zoomAdjustedPixelValueForLength(length, style);
    }

Then, the code simply becomes
    if (!renderView || !renderer || !renderer->isBox())
        return positionOffsetStyleValueForProperty(propertyID, style);


This is only my opinion regarding readability of this section. I don't feel strongly about those changes so feel free to ignore the comments if you don't think this would improve the code.
Comment 93 Tim 'mithro' Ansell 2013-02-25 18:05:13 PST
Created attachment 190170 [details]
Patch
Comment 94 Tim 'mithro' Ansell 2013-02-25 18:26:14 PST
Please take another look.
Comment 95 Tim 'mithro' Ansell 2013-03-04 17:10:48 PST
Ping?
Comment 96 Eric Seidel 2013-03-08 13:17:27 PST
Comment on attachment 190170 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190170&action=review

OK.  This change looks great.  My only concern is about testing.  There are a lot of cases here.  I'm not sure you covered position: sticky.  You shoudl be aware of the many types of lenghts:
enum LengthType {
    Auto, Relative, Percent, Fixed,
    Intrinsic, MinIntrinsic,
    MinContent, MaxContent, FillAvailable, FitContent,
    Calculated,
    ViewportPercentageWidth, ViewportPercentageHeight, ViewportPercentageMin, ViewportPercentageMax,
    Undefined
};

You shouldn't need to care about those, but its good to knwo it exists.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:641
> +    // See http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSview-getComputedStyle

Maybe you meant this?
http://dev.w3.org/csswg/cssom/#widl-Window-getComputedStyle-CSSStyleDeclaration-Element-elt

If the property applies to a positioned element and the resolved value of the 'display' property is not none, the resolved value is the used value. Otherwise the resolved value is the computed value.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:645
> +            return zoomAdjustedPixelValueForLength(style->left(), style);

I might have moved these switch statements into helper functions.  Then this code looks like:

if (!renderView || !renderer || !renderer->isBox())
    return zoomAdjustedPixelValueForLength(lengthForProperty(propertyId), style);

One could do similar for the other switches, presumably:
relativePositionForProperty(propertyId, box);
and
positionForProperty(propertyId, box, containingBlock);

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:-661
> -        else if (l.isViewportPercentage())

Do we have test coverage for this behavior?  You appear to be removing it in the display: none case.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:659
> +    if (box->isRelPositioned() || !containingBlock) {

I don't believe we can hit this case?  You will always have a containing block if you're in the DOM tree, I think?  Unless we're talking about <html style='position: relative'>?  If the element isn't in the tree, it can't have a renderer.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:687
> +    return 0;

We need tests requesting top/right/bottom/left on elements which are not positioned.
Comment 97 Eric Seidel 2013-03-08 13:20:40 PST
Comment on attachment 190170 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190170&action=review

> Source/WebCore/ChangeLog:13
> +        This is now consistent with both release Firefox and IE9.

You might add more links/documetnation here.  Your ChangeLog is sorta your "advertisement" for your change.  Tells all the "whys" and how we'd be fools to not accept your amazing change. :)
Comment 98 Tim 'mithro' Ansell 2013-04-08 21:51:30 PDT
This issue has been migrated to blink at http://code.google.com/p/chromium/issues/detail?id=178030

We will look at back porting the changes once we have committed to change to blink.
Comment 99 Michał Z. Gołębiowski 2013-04-09 01:47:29 PDT
It doesn't seem to be http://code.google.com/p/chromium/issues/detail?id=178030, did you make a typo in the number?
Comment 100 Tim 'mithro' Ansell 2013-04-09 02:13:39 PDT
Sorry - you are right. The correct bug is 
https://code.google.com/p/chromium/issues/detail?id=229280
Comment 101 Peter Mouland 2014-06-11 02:20:28 PDT
Is this bug going to be fixed?

It now works in chrome as described by the previous comment but still exists in Safari 7.0.4 (OS X 10.9.3) + Safari iOS 7.1