Bug 29084

'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.

> 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

What does Trident do?

IE9 returns pixels, as Mike says in #2.

Okay, then it appears that this is a no-brainer. We should just fix it.

And IE<9 doesn't even have getComputedStyle... But the nonstandard currentStyle returns % instead of pixels (computed vs. used)

(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.

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?

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."

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).

(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.

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?

(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?

(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.

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.

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.

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.

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?

(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.

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

I'm sorry, what we're supposed to do here? Is the percentage values correct or incorrect?

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!

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.

(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!

Created attachment 151525 [details] test

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.

(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.

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?

(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.

I added the info to the ticket. I'll also post about this on www-style to see if I can get an answer.

posted on the ML about this: http://lists.w3.org/Archives/Public/www-style/2012Jul/0277.html

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.

Created attachment 152616 [details] work in progress

by the way, this was added to the CSSOM spec on 7/17/12: http://dev.w3.org/csswg/cssom/#resolved-values

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 on attachment 152616 [details] work in progress Attachment 152616 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14981219

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 on attachment 152616 [details] work in progress Attachment 152616 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14970948

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 on attachment 152616 [details] work in progress Attachment 152616 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14986153

Comment on attachment 152616 [details] work in progress Attachment 152616 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14983188

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.

Created attachment 187794 [details] Patch

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 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 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

Created attachment 188032 [details] Patch

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 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 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 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

Created attachment 188242 [details] Patch

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 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

Created attachment 188473 [details] Patch

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 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

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.

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 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 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.

Sorry, this patch wasn't yet ready for review. I'll mark if review? when it's ready.

Created attachment 188785 [details] Patch

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.

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 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

Created attachment 188845 [details] Patch

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 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

Created attachment 189471 [details] Patch

Third attempt at trying to get this right.

Comment on attachment 189471 [details] Patch Attachment 189471 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16652878

Comment on attachment 189471 [details] Patch Attachment 189471 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16653906

Comment on attachment 189471 [details] Patch Attachment 189471 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16650929

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 on attachment 189471 [details] Patch Attachment 189471 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16651904

Comment on attachment 189471 [details] Patch Attachment 189471 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16653907

Created attachment 189487 [details] Patch

Created attachment 189490 [details] Patch

Less crashy version of the patch.

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 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 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.

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. +

Created attachment 189665 [details] Patch

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 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 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 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(); .... ??

No no no!! That'd be worse! Which part was not clear?

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...

(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.

Created attachment 190170 [details] Patch

Please take another look.

Ping?

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 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. :)

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.

It doesn't seem to be http://code.google.com/p/chromium/issues/detail?id=178030, did you make a typo in the number?

Sorry - you are right. The correct bug is https://code.google.com/p/chromium/issues/detail?id=229280

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

Created attachment 311407 [details] Patch

<rdar://problem/32439966>

Comment on attachment 311407 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311407&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:796 > + RenderBlock* containingBlock = box.containingBlock(); auto* > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2261 > + return renderer && style && renderer->isBox(); I'd write it like this: return style && renderer && renderer->isBox();

*** Bug 105979 has been marked as a duplicate of this bug. ***

Comment on attachment 311407 [details] Patch Attachment 311407 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3825541 New failing tests: imported/w3c/web-platform-tests/css-timing-1/frames-timing-functions-output.html imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/centering.html fast/css/getComputedStyle/computed-style.html animations/trigger-container-scroll-empty.html transitions/transition-to-from-auto.html fast/css/hover-affects-child.html animations/trigger-container-scroll-simple.html animations/trigger-container-scroll-boundaries.html

Created attachment 311411 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 311407 [details] Patch Attachment 311407 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3825546 New failing tests: imported/w3c/web-platform-tests/css-timing-1/frames-timing-functions-output.html imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/centering.html fast/css/getComputedStyle/computed-style.html transitions/transition-to-from-auto.html fast/css/hover-affects-child.html animations/trigger-container-scroll-simple.html animations/trigger-container-scroll-boundaries.html

Created attachment 311412 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 311407 [details] Patch Attachment 311407 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3825550 New failing tests: imported/w3c/web-platform-tests/css-timing-1/frames-timing-functions-output.html imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/centering.html fast/css/getComputedStyle/computed-style.html animations/trigger-container-scroll-empty.html transitions/transition-to-from-auto.html fast/css/hover-affects-child.html animations/trigger-container-scroll-simple.html animations/trigger-container-scroll-boundaries.html

Created attachment 311413 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 311407 [details] Patch Attachment 311407 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3825558 New failing tests: transitions/transition-to-from-auto.html imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/centering.html animations/trigger-container-scroll-simple.html imported/w3c/web-platform-tests/css-timing-1/frames-timing-functions-output.html animations/trigger-container-scroll-empty.html

Created attachment 311414 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5

Created attachment 311417 [details] Patch

Comment on attachment 311417 [details] Patch Attachment 311417 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3827761 New failing tests: imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/centering.html http/tests/cache/cancel-during-revalidation-succeeded.html webrtc/peer-connection-audio-mute.html

Created attachment 311419 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5

Comment on attachment 151525 [details] test &gt;&lt;!DOCTYPE html&gt; &gt;&lt;html&gt; &gt;&lt;body&gt; &gt;&lt;style&gt; &gt; &gt;.relative100x100 { &gt; position: relative; &gt; width: 100px; &gt; height: 100px; &gt;} &gt; &gt;#test { &gt; position: absolute; &gt;} &gt; &gt;&lt;/style&gt; &gt;&lt;div id=&quot;tests&quot;&gt; &gt;&lt;div class=&quot;relative100x100&quot;&gt;&lt;div id=&quot;test&quot;&gt;b&lt;/div&gt;&lt;/div&gt; &gt;&lt;/div&gt; &gt;&lt;script&gt; &gt; &gt;function debug(s) { &gt; document.writeln(s + '&lt;br&gt;'); &gt;} &gt; &gt;function evalAndLog(s) { &gt; eval(s); &gt; debug(s); &gt;} &gt; &gt;function shouldBe(s1, s2) { &gt; var actual = eval(s1); &gt; var expected = eval(s2); &gt; var result = s1 + ' was ' + actual; &gt; debug(actual == expected ? 'PASS - ' + result : 'FAIL - ' + result + ' but expected ' + expected); &gt;} &gt; &gt;var test = document.getElementById('test'); &gt;evalAndLog(&quot;test.setAttribute('style', 'top: 10%; left: 20%; width: 50%; height: 90%;')&quot;); &gt;shouldBe(&quot;getComputedStyle(test).top&quot;, &quot;'10px'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).left&quot;, &quot;'20px'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).right&quot;, &quot;'auto'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).bottom&quot;, &quot;'auto'&quot;); &gt; &gt;debug(''); &gt;evalAndLog(&quot;test.parentNode.setAttribute('style', 'padding: 25px;')&quot;); &gt;shouldBe(&quot;getComputedStyle(test).top&quot;, &quot;'15px'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).left&quot;, &quot;'30px'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).right&quot;, &quot;'auto'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).bottom&quot;, &quot;'auto'&quot;); &gt; &gt;debug(''); &gt;evalAndLog(&quot;test.parentNode.setAttribute('style', 'border: solid 25px;')&quot;); &gt;shouldBe(&quot;getComputedStyle(test).top&quot;, &quot;'10px'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).left&quot;, &quot;'20px'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).right&quot;, &quot;'auto'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).bottom&quot;, &quot;'auto'&quot;); &gt; &gt;debug(''); &gt;evalAndLog(&quot;test.parentNode.setAttribute('style', 'margin: 25px;')&quot;); &gt;shouldBe(&quot;getComputedStyle(test).top&quot;, &quot;'10px'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).left&quot;, &quot;'20px'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).right&quot;, &quot;'auto'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).bottom&quot;, &quot;'auto'&quot;); &gt; &gt;debug(''); &gt;evalAndLog(&quot;test.parentNode.setAttribute('style', 'position: absolute; top: 100px; height: 100px;')&quot;); &gt;shouldBe(&quot;getComputedStyle(test).top&quot;, &quot;'10px'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).left&quot;, &quot;'20px'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).right&quot;, &quot;'auto'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).bottom&quot;, &quot;'auto'&quot;); &gt;evalAndLog(&quot;test.parentNode.removeAttribute('style')&quot;); &gt; &gt;debug(''); &gt;evalAndLog(&quot;test.setAttribute('style', 'font-size: 10px; top: 1em; left: 1em; width: 1em; height: 1em;')&quot;); &gt;shouldBe(&quot;getComputedStyle(test).top&quot;, &quot;'10px'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).left&quot;, &quot;'10px'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).right&quot;, &quot;'auto'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).bottom&quot;, &quot;'auto'&quot;); &gt; &gt;debug(''); &gt;evalAndLog(&quot;test.setAttribute('style', 'right: 10%; bottom: 10%; width: 90%; height: 90%;')&quot;); &gt;shouldBe(&quot;getComputedStyle(test).top&quot;, &quot;'auto'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).left&quot;, &quot;'auto'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).right&quot;, &quot;'10px'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).bottom&quot;, &quot;'10px'&quot;); &gt; &gt;debug(''); &gt;evalAndLog(&quot;test.parentNode.setAttribute('style', 'padding: 25px;')&quot;); &gt;shouldBe(&quot;getComputedStyle(test).top&quot;, &quot;'auto'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).left&quot;, &quot;'auto'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).right&quot;, &quot;'15px'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).bottom&quot;, &quot;'15px'&quot;); &gt; &gt;debug(''); &gt;evalAndLog(&quot;test.parentNode.setAttribute('style', 'border: solid 25px;')&quot;); &gt;shouldBe(&quot;getComputedStyle(test).top&quot;, &quot;'auto'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).left&quot;, &quot;'auto'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).right&quot;, &quot;'10px'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).bottom&quot;, &quot;'10px'&quot;); &gt; &gt;debug(''); &gt;evalAndLog(&quot;test.parentNode.setAttribute('style', 'margin: 25px;')&quot;); &gt;shouldBe(&quot;getComputedStyle(test).top&quot;, &quot;'auto'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).left&quot;, &quot;'auto'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).right&quot;, &quot;'10px'&quot;); &gt;shouldBe(&quot;getComputedStyle(test).bottom&quot;, &quot;'10px'&quot;); &gt; &gt;&lt;/script&gt; &gt;&lt;/body&gt; &gt;&lt;/html&gt;

Comment on attachment 187794 [details] Patch &gt;Subversion Revision: 142565 &gt;diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog &gt;index bababe31d1c148f44cf202ef65c96a5c84a88f5a..84839d49086dd3d2e70bb06095bf253f7bdd48d4 100644 &gt;--- a/Source/WebCore/ChangeLog &gt;+++ b/Source/WebCore/ChangeLog &gt;&#64;&#64; -1,3 +1,13 &#64;&#64; &gt;+2013-02-12 Tim 'mithro' Ansell &lt;mithro&#64;mithis.com&gt; &gt;+ &gt;+ Fixing getComputedStyle for left / right / top / bottom to return pixels. &gt;+ https://bugs.webkit.org/show_bug.cgi?id=29084 &gt;+ &gt;+ Reviewed by NOBODY (OOPS!). &gt;+ &gt;+ * css/CSSComputedStyleDeclaration.cpp: &gt;+ (WebCore::CSSComputedStyleDeclaration::getPropertyCSSValue): &gt;+ &gt; 2013-02-11 Kentaro Hara &lt;haraken&#64;chromium.org&gt; &gt; &gt; [V8] ScheduledAction::m_context can be empty, so we shouldn't &gt;diff --git a/Source/WebCore/css/CSSComputedStyleDeclaration.cpp b/Source/WebCore/css/CSSComputedStyleDeclaration.cpp &gt;index c43a48f0bcbf8593a98199bc32d095e82f90c25d..b6588a7d76aaae415cb8d6ee3e31009187196ca5 100644 &gt;--- a/Source/WebCore/css/CSSComputedStyleDeclaration.cpp &gt;+++ b/Source/WebCore/css/CSSComputedStyleDeclaration.cpp &gt;&#64;&#64; -632,47 +632,6 &#64;&#64; static PassRefPtr&lt;CSSValueList&gt; createPositionListForLayer(CSSPropertyID propert &gt; return positionList.release(); &gt; } &gt; &gt;-static PassRefPtr&lt;CSSValue&gt; getPositionOffsetValue(RenderStyle* style, CSSPropertyID propertyID, RenderView* renderView) &gt;-{ &gt;- if (!style) &gt;- return 0; &gt;- &gt;- Length l; &gt;- switch (propertyID) { &gt;- case CSSPropertyLeft: &gt;- l = style-&gt;left(); &gt;- break; &gt;- case CSSPropertyRight: &gt;- l = style-&gt;right(); &gt;- break; &gt;- case CSSPropertyTop: &gt;- l = style-&gt;top(); &gt;- break; &gt;- case CSSPropertyBottom: &gt;- l = style-&gt;bottom(); &gt;- break; &gt;- default: &gt;- return 0; &gt;- } &gt;- &gt;- if (style-&gt;hasOutOfFlowPosition()) { &gt;- if (l.type() == WebCore::Fixed) &gt;- return zoomAdjustedPixelValue(l.value(), style); &gt;- else if (l.isViewportPercentage()) &gt;- return zoomAdjustedPixelValue(valueForLength(l, 0, renderView), style); &gt;- return cssValuePool().createValue(l); &gt;- } &gt;- &gt;- if (style-&gt;hasInFlowPosition()) { &gt;- // FIXME: It's not enough to simply return &quot;auto&quot; values for one offset if the other side is defined. &gt;- // In other words if left is auto and right is not auto, then left's computed value is negative right(). &gt;- // So we should get the opposite length unit and see if it is auto. &gt;- return cssValuePool().createValue(l); &gt;- } &gt;- &gt;- return cssValuePool().createIdentifierValue(CSSValueAuto); &gt;-} &gt;- &gt; PassRefPtr&lt;CSSPrimitiveValue&gt; CSSComputedStyleDeclaration::currentColorOrValidColor(RenderStyle* style, const Color&amp; color) const &gt; { &gt; // This function does NOT look at visited information, so that computed style doesn't expose that. &gt;&#64;&#64; -1747,7 +1706,7 &#64;&#64; PassRefPtr&lt;CSSValue&gt; CSSComputedStyleDeclaration::getPropertyCSSValue(CSSPropert &gt; case CSSPropertyBorderLeftWidth: &gt; return zoomAdjustedPixelValue(style-&gt;borderLeftWidth(), style.get()); &gt; case CSSPropertyBottom: &gt;- return getPositionOffsetValue(style.get(), CSSPropertyBottom, m_node-&gt;document()-&gt;renderView()); &gt;+ return zoomAdjustedPixelValue(style-&gt;bottom().value(), style.get()); &gt; case CSSPropertyWebkitBoxAlign: &gt; return cssValuePool().createValue(style-&gt;boxAlign()); &gt; #if ENABLE(CSS_BOX_DECORATION_BREAK) &gt;&#64;&#64; -1975,7 +1934,7 &#64;&#64; PassRefPtr&lt;CSSValue&gt; CSSComputedStyleDeclaration::getPropertyCSSValue(CSSPropert &gt; return cssValuePool().createValue(style-&gt;imageResolution(), CSSPrimitiveValue::CSS_DPPX); &gt; #endif &gt; case CSSPropertyLeft: &gt;- return getPositionOffsetValue(style.get(), CSSPropertyLeft, m_node-&gt;document()-&gt;renderView()); &gt;+ return zoomAdjustedPixelValue(style-&gt;left().value(), style.get()); &gt; case CSSPropertyLetterSpacing: &gt; if (!style-&gt;letterSpacing()) &gt; return cssValuePool().createIdentifierValue(CSSValueNormal); &gt;&#64;&#64; -2118,7 +2077,7 &#64;&#64; PassRefPtr&lt;CSSValue&gt; CSSComputedStyleDeclaration::getPropertyCSSValue(CSSPropert &gt; case CSSPropertyPosition: &gt; return cssValuePool().createValue(style-&gt;position()); &gt; case CSSPropertyRight: &gt;- return getPositionOffsetValue(style.get(), CSSPropertyRight, m_node-&gt;document()-&gt;renderView()); &gt;+ return zoomAdjustedPixelValue(style-&gt;right().value(), style.get()); &gt; case CSSPropertyWebkitRubyPosition: &gt; return cssValuePool().createValue(style-&gt;rubyPosition()); &gt; case CSSPropertyTableLayout: &gt;&#64;&#64; -2186,7 +2145,7 &#64;&#64; PassRefPtr&lt;CSSValue&gt; CSSComputedStyleDeclaration::getPropertyCSSValue(CSSPropert &gt; case CSSPropertyTextTransform: &gt; return cssValuePool().createValue(style-&gt;textTransform()); &gt; case CSSPropertyTop: &gt;- return getPositionOffsetValue(style.get(), CSSPropertyTop, m_node-&gt;document()-&gt;renderView()); &gt;+ return zoomAdjustedPixelValue(style-&gt;top().value(), style.get()); &gt; case CSSPropertyUnicodeBidi: &gt; return cssValuePool().createValue(style-&gt;unicodeBidi()); &gt; case CSSPropertyVerticalAlign: