Bug 55111 - getComputedStyle() returns unitless values for some properties that require units
Summary: getComputedStyle() returns unitless values for some properties that require u...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-23 19:47 PST by Rob Brackett
Modified: 2011-05-01 10:30 PDT (History)
3 users (show)

See Also:


Attachments
Test case for unit-less properties in getComputedStyle() (6.37 KB, text/html)
2011-02-23 19:47 PST, Rob Brackett
no flags Details
Patch (13.82 KB, patch)
2011-04-18 15:29 PDT, Emil A Eklund
ojan: review+
ojan: commit-queue-
Details | Formatted Diff | Diff
Patch (13.83 KB, patch)
2011-04-29 12:30 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.