RESOLVED FIXED Bug 89697
[JSC] CSSStyleDeclaration report incorrect descriptor 
https://bugs.webkit.org/show_bug.cgi?id=89697
Summary [JSC] CSSStyleDeclaration report incorrect descriptor
Erik Arvidsson
Reported 2012-06-21 15:40:58 PDT
The properties of the CSSStyleDeclaration are enumerable by the for in loop but the property descriptor reports them as non enumerable. var style = document.getComputedStyle(document.body); var descr = Object.getOwnPropertyDescriptor(style, 'color'); shouldBeTrue('descr.enumerable');
Attachments
Patch (4.66 KB, patch)
2013-06-07 13:24 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Patch (4.76 KB, patch)
2013-08-05 01:48 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Patch (4.84 KB, patch)
2014-04-14 02:49 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Manuel Rego Casasnovas
Comment 1 2013-06-07 13:15:05 PDT
(In reply to comment #0) > The properties of the CSSStyleDeclaration are enumerable by the for in loop but the property descriptor reports them as non enumerable. > > var style = document.getComputedStyle(document.body); > var descr = Object.getOwnPropertyDescriptor(style, 'color'); > shouldBeTrue('descr.enumerable'); There's also more weird stuff regarding CSS2Properties in CSSStyleDeclaration. For example if you get the descriptor of "color" property with Object.getOwnPropertyDescriptor() it returns: Object {value: "", writable: false, enumerable: false, configurable: false} So it appears as "writable: false" however things like the next line works properly: style.color = "blue"; According to the spec (http://www.ecma-international.org/publications/files/ECMA-ST/Ecma-262.pdf) if writable is false you shouldn't be allowed to modify the value. So it's clear that the descriptor is wrong. BTW, the descriptor in Chrome is: Object {value: "", writable: true, enumerable: true, configurable: true}
Manuel Rego Casasnovas
Comment 2 2013-06-07 13:24:30 PDT
Created attachment 204064 [details] Patch
Manuel Rego Casasnovas
Comment 3 2013-06-07 13:28:50 PDT
Comment on attachment 204064 [details] Patch This patch changes the property descriptor. However there's still an issue, as even when the descriptor now say "configurable: true" (like in Chrome), if you use Object.defineProperty() to change the descriptor the change is not working. For example with the following code you don't get any message in the JavaScript console: Object.defineProperty(style, "color", { get : function(){ console.log("get color"); }, set : function(x){ console.log("set color: " + x); }, }); Anyway I think that we could report this as a different bug as it seems clear that current descriptor is not accurate with the behavior of CSS2Properties in CSSStyleDeclaration object.
Geoffrey Garen
Comment 4 2013-08-01 21:20:31 PDT
Comment on attachment 204064 [details] Patch Removing ReadOnly and DontEnum here is good. But let's keep DontDelete (configurable: true) for now, Since the property isn't delete-able, that's the most honest thing to report.
Manuel Rego Casasnovas
Comment 5 2013-08-05 01:48:48 PDT
Created attachment 208111 [details] Patch
Benjamin Poulain
Comment 6 2014-04-13 23:19:01 PDT
Comment on attachment 208111 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208111&action=review The patch looks good. Some issue with the test. This will need a rebaseline. > LayoutTests/ChangeLog:12 > + * fast/js/cssstyledeclaration-properties-descriptor-expected.txt: Added. > + * fast/js/cssstyledeclaration-properties-descriptor.html: Added. > + * fast/js/script-tests/cssstyledeclaration-properties-descriptor.js: Added. The test is at the wrong place, it should be in LayoutTests//fast/dom/CSSStyleDeclaration > LayoutTests/fast/js/cssstyledeclaration-properties-descriptor.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> Please use the HTML5 doctype. > LayoutTests/fast/js/cssstyledeclaration-properties-descriptor.html:4 > +<script src="resources/js-test-pre.js"></script> This won't work anymore, the path has changed. :( > LayoutTests/fast/js/cssstyledeclaration-properties-descriptor.html:7 > +<script src="script-tests/cssstyledeclaration-properties-descriptor.js"></script> Please put the test code here instead of using a separate file. The separate file thingy is the old style of WebKit. It makes it more painful to work on tests. > LayoutTests/fast/js/script-tests/cssstyledeclaration-properties-descriptor.js:3 > +description( > +"This tests the descriptor of CSSStyleDeclaration properties." > +); This should be on a single line.
Manuel Rego Casasnovas
Comment 7 2014-04-14 02:49:16 PDT
Created attachment 229275 [details] Patch Thanks for the review. Patch for landing applying review suggestions.
WebKit Commit Bot
Comment 8 2014-04-14 04:22:16 PDT
Comment on attachment 229275 [details] Patch Clearing flags on attachment: 229275 Committed r167240: <http://trac.webkit.org/changeset/167240>
WebKit Commit Bot
Comment 9 2014-04-14 04:22:21 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.