Changeset 18320 in webkit

Timestamp:

Dec 19, 2006 10:56:45 AM (17 years ago)

Author:

ap

Message:

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

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/block/positioning/relayout-on-position-change.html (modified) (1 diff)
• LayoutTests/fast/dom/css-set-property-exception-expected.txt (modified) (1 diff)
• LayoutTests/fast/dom/css-set-property-exception.html (modified) (3 diffs)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/bindings/js/kjs_css.cpp (modified) (3 diffs)
• WebCore/css/CSSMutableStyleDeclaration.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/LayoutTests/fast/block/positioning/relayout-on-position-change.html

@@ -5 +5 @@
 {
     document.body.offsetTop;    // force layout
-    document.getElementById('change').style.position = "none";
+    document.getElementById('change').style.position = "";
 }
 </script>

trunk/LayoutTests/fast/dom/css-set-property-exception-expected.txt

@@ -1 @@
-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; ".
+

trunk/LayoutTests/fast/dom/css-set-property-exception.html

@@ -8 +8 @@
     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()
 {
@@ -15 +24 @@
     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 + "\".");
     }
 }
@@ -66 +91 @@
 </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>

trunk/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/WebCore/bindings/js/kjs_css.cpp

@@ -35 +35 @@
 #include "CSSValueList.h"
 #include "Document.h"
+#include "Frame.h"
 #include "HTMLNames.h"
 #include "HTMLStyleElement.h"
@@ -43 +44 @@
 #include "JSCSSValueList.h"
 #include "MediaList.h"
+#include "Settings.h"
 #include "StyleSheetList.h"
 #include "kjs_dom.h"
@@ -212 +214 @@
       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);

trunk/WebCore/css/CSSMutableStyleDeclaration.cpp

@@ -298 +298 @@
     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);