Reviewed by Darin.
authorap <ap@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Dec 2006 18:56:45 +0000 (18:56 +0000)
committerap <ap@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Dec 2006 18:56:45 +0000 (18:56 +0000)
        http://bugs.webkit.org/show_bug.cgi?id=7296
        JavaScript error not thrown when trying to set a CSS property to an invalid value

WebCore:
        * bindings/js/kjs_css.cpp:
        (KJS::DOMCSSStyleDeclaration::put): When not in Dashboard compatibility mode,
        raise exception for invalid values. Also removed an unnecessary call to
        removeProperty(), which prevented the property value from being preserved in
        error case.

        * css/CSSMutableStyleDeclaration.cpp:
        (WebCore::CSSMutableStyleDeclaration::setProperty): Moved the handling of
        empty property values here. Also removed an unnecessary call to removeProperty().

LayoutTests:
        * fast/block/positioning/relayout-on-position-change.html: This test was setting
        position property to an invalid value, expecting that it will be removed. Changed
        it to set the property to an empty value (now the test passes in Firefox, too).

        * fast/dom/css-set-property-exception-expected.txt:
        * fast/dom/css-set-property-exception.html:
        Updated the results, added a new case and made the output more verbose.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@18320 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/block/positioning/relayout-on-position-change.html
LayoutTests/fast/dom/css-set-property-exception-expected.txt
LayoutTests/fast/dom/css-set-property-exception.html
WebCore/ChangeLog
WebCore/bindings/js/kjs_css.cpp
WebCore/css/CSSMutableStyleDeclaration.cpp

index 47347d9..8053234 100644 (file)
@@ -1,3 +1,18 @@
+2006-12-19  Alexey Proskuryakov  <ap@webkit.org>
+
+        Reviewed by Darin.
+
+        http://bugs.webkit.org/show_bug.cgi?id=7296
+        JavaScript error not thrown when trying to set a CSS property to an invalid value
+
+        * fast/block/positioning/relayout-on-position-change.html: This test was setting
+        position property to an invalid value, expecting that it will be removed. Changed
+        it to set the property to an empty value (now the test passes in Firefox, too).
+
+        * fast/dom/css-set-property-exception-expected.txt:
+        * fast/dom/css-set-property-exception.html:
+        Updated the results, added a new case and made the output more verbose.
+
 2006-12-18  Nikolas Zimmermann  <zimmermann@kde.org>
 
         Reviewed by Sam. Previous attempts reviewed by Oliver & Eric.
index 10ae5e2..dee45ba 100644 (file)
@@ -4,7 +4,7 @@
 function test()
 {
     document.body.offsetTop;    // force layout
-    document.getElementById('change').style.position = "none";
+    document.getElementById('change').style.position = "";
 }
 </script>
 </head>
index ae6fd51..f90ead5 100644 (file)
@@ -1,13 +1,17 @@
-This test checks to see whether you get exceptions when setting a property with a "bad value". We preserve our historic behavior of never raising an exception when you set a CSS property using JavaScript property syntax. But we do raise exceptions when setting a property with setProperty.
+Test for bug 7296.
 
-The results below should show four successes, followed by two exceptions.
+This test checks to see whether you get exceptions when setting a property with a "bad value". Setting using JavaScript property syntax and with setProperty() should behave the same.
 
-This is the test element.
+The results below should show success in cases 1, 3, 5, and 7.
 
-Successfully set display to "block"; value is now "block".
-Successfully set display to ""; value is now "".
-Successfully set display to null; value is now "".
-Successfully set display to "block" with setProperty; value is now "block".
-Got exception trying to set display to "" with setProperty; value is now "".
-Got exception trying to set display to null with setProperty; value is now "".
+It is OK if the order of properties changes from the expected results - IE 6 and Firefox 2 don't agree on it anyway.
+
+Successfully set display to "block"; cssText is now: "top: 0px; bottom: 1px; display: block; ".
+Got exception trying to set display to "foobar"; cssText is now: "top: 0px; display: none; bottom: 1px; ".
+Successfully set display to ""; cssText is now: "top: 0px; bottom: 1px; ".
+Got exception trying to set display to null; cssText is now: "top: 0px; display: none; bottom: 1px; ".
+Successfully set display to "block" with setProperty; cssText is now: "top: 0px; bottom: 1px; display: block; ".
+Got exception trying to set display to "foobar" with setProperty; cssText is now: "top: 0px; display: none; bottom: 1px; ".
+Successfully set display to "" with setProperty; cssText is now: "top: 0px; bottom: 1px; ".
+Got exception trying to set display to null with setProperty; cssText is now: "top: 0px; display: none; bottom: 1px; ".
 
index 16c07f9..b6ff453 100644 (file)
@@ -7,6 +7,15 @@ function log(message)
     item.appendChild(document.createTextNode(message));
     document.getElementById("console").appendChild(item);
 }
+function setInitialStyleForE()
+{
+    var e = document.getElementById('e');
+
+    e.style.top = "0px";
+    e.style.bottom = "";
+    e.style.display = "none";
+    e.style.bottom = "1px";
+}
 function test()
 {
     if (window.layoutTestController)
@@ -14,61 +23,78 @@ function test()
 
     var e = document.getElementById('e');
 
-    e.style.display = "none";
+    setInitialStyleForE();
     try {
         e.style.display = "block";
-        log("Successfully set display to \"block\"; value is now \"" + e.style.display + "\".");
+        log("Successfully set display to \"block\"; cssText is now: \"" + e.style.cssText + "\".");
     } catch (exception) {
-        log("Got exception trying to set display to \"block\"; value is now \"" + e.style.display + "\".");
+        log("Got exception trying to set display to \"block\"; cssText is now: \"" + e.style.cssText + "\".");
     }
 
-    e.style.display = "none";
+    setInitialStyleForE();
+    try {
+        e.style.display = "foobar";
+        log("Successfully set display to \"foobar\"; cssText is now: \"" + e.style.cssText + "\".");
+    } catch (exception) {
+        log("Got exception trying to set display to \"foobar\"; cssText is now: \"" + e.style.cssText + "\".");
+    }
+
+    setInitialStyleForE();
     try {
         e.style.display = "";
-        log("Successfully set display to \"\"; value is now \"" + e.style.display + "\".");
+        log("Successfully set display to \"\"; cssText is now: \"" + e.style.cssText + "\".");
     } catch (exception) {
-        log("Got exception trying to set display to \"\"; value is now \"" + e.style.display + "\".");
+        log("Got exception trying to set display to \"\"; cssText is now: \"" + e.style.cssText + "\".");
     }
 
-    e.style.display = "none";
+    setInitialStyleForE();
     try {
         e.style.display = null;
-        log("Successfully set display to null; value is now \"" + e.style.display + "\".");
+        log("Successfully set display to null; cssText is now: \"" + e.style.cssText + "\".");
     } catch (exception) {
-        log("Got exception trying to set display to null; value is now \"" + e.style.display + "\".");
+        log("Got exception trying to set display to null; cssText is now: \"" + e.style.cssText + "\".");
     }
 
-    e.style.display = "none";
+    setInitialStyleForE();
     try {
         e.style.setProperty("display", "block", "");
-        log("Successfully set display to \"block\" with setProperty; value is now \"" + e.style.display + "\".");
+        log("Successfully set display to \"block\" with setProperty; cssText is now: \"" + e.style.cssText + "\".");
     } catch (exception) {
-        log("Got exception trying to set display to \"block\" with setProperty; value is now \"" + e.style.display + "\".");
+        log("Got exception trying to set display to \"block\" with setProperty; cssText is now: \"" + e.style.cssText + "\".");
     }
 
-    e.style.display = "none";
+    setInitialStyleForE();
+    try {
+        e.style.setProperty("display", "foobar", "");
+        log("Successfully set display to \"foobar\" with setProperty; cssText is now: \"" + e.style.cssText + "\".");
+    } catch (exception) {
+        log("Got exception trying to set display to \"foobar\" with setProperty; cssText is now: \"" + e.style.cssText + "\".");
+    }
+
+    setInitialStyleForE();
     try {
         e.style.setProperty("display", "", "");
-        log("Successfully set display to \"\" with setProperty; value is now \"" + e.style.display + "\".");
+        log("Successfully set display to \"\" with setProperty; cssText is now: \"" + e.style.cssText + "\".");
     } catch (exception) {
-        log("Got exception trying to set display to \"\" with setProperty; value is now \"" + e.style.display + "\".");
+        log("Got exception trying to set display to \"\" with setProperty; cssText is now: \"" + e.style.cssText + "\".");
     }
 
-    e.style.display = "none";
+    setInitialStyleForE();
     try {
         e.style.setProperty("display", null, "");
-        log("Successfully set display to null with setProperty; value is now \"" + e.style.display + "\".");
+        log("Successfully set display to null with setProperty; cssText is now: \"" + e.style.cssText + "\".");
     } catch (exception) {
-        log("Got exception trying to set display to null with setProperty; value is now \"" + e.style.display + "\".");
+        log("Got exception trying to set display to null with setProperty; cssText is now: \"" + e.style.cssText + "\".");
     }
 }
 </script>
 </head>
 <body onload="test();">
+<p>Test for <a href="http://bugs.webkit.org/show_bug.cgi?id=7296">bug 7296</a>.</p>
 <p>This test checks to see whether you get exceptions when setting a property with a "bad value".
-We preserve our historic behavior of never raising an exception when you set a CSS property using JavaScript property syntax.
-But we do raise exceptions when setting a property with setProperty.</p>
-<p>The results below should show four successes, followed by two exceptions.</p>
+Setting using JavaScript property syntax and with setProperty() should behave the same.</p>
+<p>The results below should show success in cases 1, 3, 5, and 7.</p>
+<P>It is OK if the order of properties changes from the expected results - IE 6 and Firefox 2 don't agree on it anyway.</p>
 <hr>
 <p id="e">This is the test element.</p>
 <hr>
index 19a6dd0..cffa569 100644 (file)
@@ -1,3 +1,20 @@
+2006-12-19  Alexey Proskuryakov  <ap@webkit.org>
+
+        Reviewed by Darin.
+
+        http://bugs.webkit.org/show_bug.cgi?id=7296
+        JavaScript error not thrown when trying to set a CSS property to an invalid value
+
+        * bindings/js/kjs_css.cpp:
+        (KJS::DOMCSSStyleDeclaration::put): When not in Dashboard compatibility mode,
+        raise exception for invalid values. Also removed an unnecessary call to
+        removeProperty(), which prevented the property value from being preserved in 
+        error case.
+
+        * css/CSSMutableStyleDeclaration.cpp:
+        (WebCore::CSSMutableStyleDeclaration::setProperty): Moved the handling of
+        empty property values here. Also removed an unnecessary call to removeProperty().
+
 2006-12-19  Anders Carlsson  <acarlsson@apple.com>
 
         Reviewed by Adam.
index 10d979e..0d4b948 100644 (file)
@@ -34,6 +34,7 @@
 #include "CSSStyleRule.h"
 #include "CSSValueList.h"
 #include "Document.h"
+#include "Frame.h"
 #include "HTMLNames.h"
 #include "HTMLStyleElement.h"
 #include "JSCSSPrimitiveValue.h"
@@ -42,6 +43,7 @@
 #include "JSCSSRuleList.h"
 #include "JSCSSValueList.h"
 #include "MediaList.h"
+#include "Settings.h"
 #include "StyleSheetList.h"
 #include "kjs_dom.h"
 
@@ -211,24 +213,23 @@ void DOMCSSStyleDeclaration::put(ExecState* exec, const Identifier &propertyName
     if (isCSSPropertyName(propertyName)) {
       bool pixelOrPos;
       String prop = cssPropertyName(propertyName, &pixelOrPos);
-      String propvalue = value->toString(exec);
+      String propValue = value->toString(exec);
       if (pixelOrPos)
-        propvalue += "px";
+        propValue += "px";
 #ifdef KJS_VERBOSE
       kdDebug(6070) << "DOMCSSStyleDeclaration: prop=" << prop << " propvalue=" << propvalue << endl;
 #endif
-      styleDecl.removeProperty(prop, exception);
-      if (!exception && !propvalue.isEmpty()) {
-        // We have to ignore exceptions here, because of the following unfortunate situation:
-        //   1) Older versions ignored exceptions here by accident, because the put function
-        //      that translated exceptions did not translate CSS exceptions.
-        //   2) Gecko does not raise an exception in this case, although WinIE does.
-        //   3) At least some Dashboard widgets are depending on this behavior.
-        // It would be nice to fix this some day, perhaps with some kind of "quirks mode",
-        // but it's likely that the Dashboard widgets are already using a strict mode DOCTYPE.
-        ExceptionCode ec = 0;
-        styleDecl.setProperty(prop, propvalue, ec);
-      }
+      ASSERT(styleDecl.stylesheet()->isCSSStyleSheet());
+      if (Frame* frame = static_cast<CSSStyleSheet*>(styleDecl.stylesheet())->doc()->frame())
+        if (frame->settings()->shouldUseDashboardBackwardCompatibilityMode()) {
+          styleDecl.removeProperty(prop, exception);
+          if (!exception) {
+            ExceptionCode exceptionIgnored = 0;
+            styleDecl.setProperty(prop, propValue, exceptionIgnored);
+            return;
+          }
+        }
+      styleDecl.setProperty(prop, propValue, exception);
     } else {
       DOMObject::put(exec, propertyName, value, attr);
     }
index 4b4f416..63c3419 100644 (file)
@@ -297,8 +297,14 @@ bool CSSMutableStyleDeclaration::setProperty(int propertyID, const String &value
 {
     ec = 0;
 
-    removeProperty(propertyID);
+    // Setting the value to an empty string just removes the property in both IE and Gecko.
+    if (value.isEmpty()) {
+        removeProperty(propertyID, notifyChanged, ec);
+        return ec == 0;
+    }
 
+    // When replacing an existing property value, this moves the property to the end of the list.
+    // Firefox preserves the position, and MSIE moves the property to the beginning.
     CSSParser parser(useStrictParsing());
     bool success = parser.parseValue(this, propertyID, value, important);
     if (!success) {