Bug 55111

Summary: getComputedStyle() returns unitless values for some properties that require units
Product: WebKit Reporter: Rob Brackett <rob.brackett>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, commit-queue, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Macintosh Intel   
OS: OS X 10.6   
Attachments:
Description Flags
Test case for unit-less properties in getComputedStyle()
none
Patch
ojan: review+, ojan: commit-queue-
Patch none

Description Rob Brackett 2011-02-23 19:47:40 PST
Created attachment 83598 [details]
Test case for unit-less properties in getComputedStyle()

The values returned by getComputedStyle() for some properties that require units do not include units. For example:

<div id="test" style="-webkit-column-gap: 20px"></div>
<script type="application/javascript">
  getComputedStyle(document.getElementById("test")).webkitColumnGap; // "20" (should be "20px")
</script>

The properties exhibiting the issue are:
-webkit-column-width
-webkit-column-gap
-webkit-perspective

A test case is attached. Properties in red do not include units. At current, there are only three buggy properties and they are equally buggy when display is "none" as when it is not, but experience with getComputedStyle() bugs has taught me the value of a comprehensive test ;)
Comment 1 Emil A Eklund 2011-04-18 15:29:26 PDT
Created attachment 90103 [details]
Patch
Comment 2 Ojan Vafai 2011-04-26 15:42:40 PDT
Comment on attachment 90103 [details]
Patch

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

> LayoutTests/fast/css/getComputedStyle/getComputedStyle-length-unit.html:93
> +        for (var propertyName in propertiesToCheck) {
> +            testElement.style[propertyName] = propertiesToCheck[propertyName];
> +        }
> +        
> +        function testPropertyUnit(className, propertyName)
> +        {
> +            if (testElement.className != className)
> +                testElement.className = className;
> +            var computedStyled = document.defaultView.getComputedStyle(testElement);
> +            var results = /\d+([a-z%]+)/.exec(computedStyled[propertyName]);
> +            return results ? results[1] : '';
> +        }
> +
> +        for (var propertyName in propertiesToCheck) {
> +            shouldBe("testPropertyUnit('visible', '" + propertyName + "')", "'px'");
> +        }
> +        for (var propertyName in propertiesToCheck) {
> +            shouldBe("testPropertyUnit('hidden', '" + propertyName + "')", "'px'");
> +        }
> +
> +        for (var propertyName in propertiesToCheck) {
> +            testElement.style[propertyName] = '';
> +        }

nit: although we don't enforce style for tests, webkit style is to not include curly braces for one-line if/else/for/while statements.
Comment 3 Emil A Eklund 2011-04-29 12:30:38 PDT
Created attachment 91719 [details]
Patch

Thanks Ojan. Fixed the style issues in the layout test, please take another look.
Comment 4 WebKit Commit Bot 2011-05-01 10:30:44 PDT
Comment on attachment 91719 [details]
Patch

Clearing flags on attachment: 91719

Committed r85418: <http://trac.webkit.org/changeset/85418>
Comment 5 WebKit Commit Bot 2011-05-01 10:30:49 PDT
All reviewed patches have been landed.  Closing bug.