Bug 89697

Summary:

[JSC] CSSStyleDeclaration report incorrect descriptor

Product:

WebKit

Reporter:

Erik Arvidsson <arv>

Component:

DOM

Assignee:

Manuel Rego Casasnovas <rego>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, barraclough, benjamin, commit-queue, eric, ggaren, kling, koivisto, mitz, oliver, rego, rniwa, sam

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

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	Details Formatted Diff Diff
Patch (4.76 KB, patch) 2013-08-05 01:48 PDT, Manuel Rego Casasnovas	no flags	Details Formatted Diff Diff
Patch (4.84 KB, patch) 2014-04-14 02:49 PDT, Manuel Rego Casasnovas	no flags	Details Formatted Diff Diff
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.