Bug 7296 – JavaScript error not thrown when trying to set a CSS property to an invalid value

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Giovanni Intini]

The following two lines (full code attached) are used to dinamically change the position of a div:

67: innerDiv.style.top = top + (event.clientY - dragStartTop) + 'px';
68: innerDiv.style.left = left + (event.clientX - dragStartLeft);

Using an XHTML doctype the first line is correct while the 2nd one isn't because it's missing the + 'px' part. 

Safari doesn't work (as it should), but the javascript console doesn't return any error.

[Giovanni Intini]

Created attachment 6533 [details]
Full page code

[Alexey Proskuryakov]

I think this is known - a comment in the code says this problem cannot be fixed right away because that would cause backwards compatibility problems.

[Mark Rowe (bdash)]

I should point out that it only seems to occur when the document is in "standards" mode.  In quirks mode, the value is parsed even without the "px" and everything proceeds.  In standards mode, the value is not parsed with the "px" and therefore the line has zero effect.  I'm not clear on how changing this would be a compatibility problem - on the same code Firefox raises a JavaScript exception when setting the property.

[Alexey Proskuryakov]

I have now found this comment, it's in DOMCSSStyleDeclaration::put(). FWIW:
// 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.

[Mark Rowe (bdash)]

Thanks for the clarification.  Breaking Dashboard widgets is a pretty big no-no, so that would be reason enough not to "fix" this.  Is there any easy way to log a "warning" in the JS console without throwing an exception?  Having code that mysteriously doesn't work with no message as to why can be very frustrating.

[Alexey Proskuryakov]

Looks like this is done with Frame::addMessageToConsole(). I think it is a great idea!

[Alexey Proskuryakov]

Created attachment 11888 [details]
proposed fix

Now we do have a quirks mode for Dashboard!

The attached test now raises an exception even earlier than expected, that is, when trying to set the cursor:

innerDiv.style.cursor = "-moz-grab";

WinIE treats this as "url('-moz-grab')" and thus doesn't raise an exception. OTOH, Firefox ignores unknown cursor names. I'm going to file a followup bug for this.

See also bug 9687

[Alexey Proskuryakov]

(In reply to comment #7)
> OTOH, Firefox ignores unknown cursor names. 

More precisely, it treats such declarations as invalid, but it doesn't raise exceptions for invalid CSS anyway.

[Darin Adler]

Comment on attachment 11888 [details]
proposed fix

The mechanics of the change look good. But the bug report has conflicting information about Firefox's behavior.

Generally, fixes that cause us to raise more exceptions have been problematic in the past -- it's one of the easiest ways to stop sites from working.

+      if (Document* doc = static_cast<CSSStyleSheet*>(styleDecl.stylesheet())->doc())

No need to nil-check the document. A document can have a nil frame, but a stylesheet can't have a nil document.

+            styleDecl.removeProperty(prop, exception);
+            if (!exception) {
+              ExceptionCode ec = 0;
+              styleDecl.setProperty(prop, propValue, ec);
+              return;
+            }

I think it's confusing here to have one variable named exception and another named ec. The difference is that the second one is ignored, and so perhaps its name should reflect that.

+    // 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.

Should we have a test case or bug report for this?

I'm going to say review+ even though I think this is risky for compatibility.

[Alexey Proskuryakov]

Committed revision 18320. Filed bug 11878 for cursors.

(In reply to comment #10)

> +      if (Document* doc =
> static_cast<CSSStyleSheet*>(styleDecl.stylesheet())->doc())
> 
> No need to nil-check the document. A document can have a nil frame, but a
> stylesheet can't have a nil document.

Fixed.

> I think it's confusing here to have one variable named exception and another
> named ec. The difference is that the second one is ignored, and so perhaps its
> name should reflect that.

OK.

> +    // 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.
> 
> Should we have a test case or bug report for this?

The modified fast/dom/css-set-property-exception.html test will at least catch changes in behavior. I don't think there's anything for us to fix, since the behavior is unspecified, and other browsers behave inconsistently.

[Geoffrey Garen]

Hyatt and I think this change was a bad idea.

1. Small gain. It doesn't improve performance or compatibility.

2. It's a ticking compatibilty timebomb. IE6 is pretty lenient with invalid CSS values. It doesn't throw when setting to null, for example. Throwing an exception completely destroys a site / widget / app.

3. Dashboard quirks should be a last resort. There's no way for an author to turn them off, and they create a confusing gap between widgets and real web technology.

4. The proof is in the pudding: this change broke our very own Web Inspector and JavaScript debugger!

[Mark Rowe (bdash)]

For reference, bug 11888 covers the inspector/debugger breakage and includes a proposed patch which treats null as valid to allow its use in resetting a property to its initial value.  Of course, the patch is moot if this change gets reverted.

[Alexey Proskuryakov]

(In reply to comment #12)
> IE6 is pretty lenient with invalid CSS values. 
> It doesn't throw when setting to null, for example. 

  Interesting - in my tests, it does! See fast/dom/css-set-property-exception.html .

> 4. The proof is in the pudding: this change broke our very own Web Inspector
> and JavaScript debugger!

  Well, this sort of thing aways happens to code that was only tested in one browser. Thankfully, there isn't that much code that has only been tested with WebKit ;)

  One thing I want to point out is that this patch shouldn't be just reverted if it's decided that exceptions shouldn't be thrown. Besides enabling exceptions, it corrected the behavior is error case. 

[Geoffrey Garen]

>   Interesting - in my tests, it does! See
> fast/dom/css-set-property-exception.html .

I can see that IE throws in your test. But it doesn't throw for, e.g., div.style.width = null. And FF, whose code path we usually take on websites, never throws.

Can we just report the error in the console instead of throwing an exception? Generally, throwing an exception for bad CSS seems completely contrary to CSS, whose goal is gracefully to ignore unsupported syntax.

Also, I think this might be a bug:

+      if (Document* doc = static_cast<CSSStyleSheet*>(styleDecl.stylesheet())->doc())
+        if (Frame* frame = doc->frame())
+          if (frame->settings()->shouldUseDashboardBackwardCompatibilityMode()) {
+            styleDecl.removeProperty(prop, exception);
+            if (!exception) {
+              ExceptionCode ec = 0;
+              styleDecl.setProperty(prop, propValue, ec);
+              return;
+            }
+          }
+      styleDecl.setProperty(prop, propValue, exception);

If removeProperty throws an exception, this code will call setProperty, even though there's a branch that attempts to prevent that. I think the second call to setProperty needs to be in an 'else' clause.

[Alexey Proskuryakov]

(In reply to comment #15)
> I can see that IE throws in your test. But it doesn't throw for, e.g.,
> div.style.width = null. And FF, whose code path we usually take on websites,
> never throws.

  I've now added a special case for null, see bug 11919. Please note that previously, that didn't behave correctly in all cases anyway (because null was converted to "null"), and no one seemed to care :)

> Can we just report the error in the console instead of throwing an exception?
> Generally, throwing an exception for bad CSS seems completely contrary to CSS,
> whose goal is gracefully to ignore unsupported syntax.

  Of course we can! I just think it's humiliating not to be standards-compliant in an area where IE is compliant. Hiding mistakes in general may be good for compatibility in short term, but has a bunch of its own negative consequences.

  Your argument #2 (IE6 is pretty lenient with invalid CSS values) does make me rather nervous indeed, but maybe we should wait for a few compatibility problems before freaking out?

> Also, I think this might be a bug:

  Oops! I think "return" in Dashboard case should be moved down a bit.

[Geoffrey Garen]

>   I've now added a special case for null, see bug 11919.

Thanks!

>  Please note that previously, that didn't behave correctly in all cases anyway (because null was
> converted to "null"), and no one seemed to care :)

Sure, and that's great. My concern is limited to exceptions.

>   Of course we can! I just think it's humiliating not to be standards-compliant
> in an area where IE is compliant. Hiding mistakes in general may be good for
> compatibility in short term, but has a bunch of its own negative consequences.

I have mixed feelings about this. Standards help developers understand how a browser will behave. But this standard seems like more trouble than it's worth. CSS is meant to grow and change. If I develop CSS for a modern Safari, and it throws exceptions in an older Safari, I'm an unhappy developer.

>   Your argument #2 (IE6 is pretty lenient with invalid CSS values) does make me
> rather nervous indeed, but maybe we should wait for a few compatibility
> problems before freaking out?

OK, let's see how things turn out. Bug 11936 is our first.

[Alexey Proskuryakov]

(In reply to comment #17)

> But this standard seems like more trouble than it's worth.
> CSS is meant to grow and change.
<...>
> OK, let's see how things turn out. Bug 11936 is our first.

If we count bug 11878 (Mozilla cursors), it's already the second...

I guess I'm convinced now.

[Geoffrey Garen]

I wonder if we could honor the spirit of the spec by return false or an exception object from setProperty when parsing failed, without actually throwing an exception. The spec says that setProperty should have a void return value, so it would be a neat augmentation.

[Alexey Proskuryakov]

Created attachment 11992 [details]
disable exceptions

This doesn't implement logging (tracked as bug 9687).

[Geoffrey Garen]

Comment on attachment 11992 [details]
disable exceptions

I spoke with ap about this. He's going to remove the Dashboard quirk. We'd rather not have them unless we know they're needed.

[Alexey Proskuryakov]

Created attachment 12010 [details]
without a Dashboard quirk

We're ending up with throwing less than before - in shipping Safari, setProperty was allowed to throw exceptions.

[Geoffrey Garen]

Does bug 11993 mean that we need the quirk afterall?

[Darin Adler]

Comment on attachment 12010 [details]
without a Dashboard quirk

I'm going to wait and let Geoff review this.

[Darin Adler]

(In reply to comment #23)
> Does bug 11993 mean that we need the quirk afterall?

I don't think so! Alexey's basically saying we're going in the opposite direction -- we're going to throw exceptions even *less* than before, so bugs like that one will go away.

[Darin Adler]

Comment on attachment 12010 [details]
without a Dashboard quirk

ChangeLog is misleading since it mentions a Dashboard quirk and the patch has none.

In the case where the value is not parsable, the old code removes the property. But the new code does nothing. Is that change OK for Dashboard widgets? I'm not sure it is.

[Alexey Proskuryakov]

(In reply to comment #26)
> In the case where the value is not parsable, the old code removes the property.
> But the new code does nothing. Is that change OK for Dashboard widgets? I'm not
> sure it is.

I believe that's what Geoff is asking about in comment 23. I don't have an answer myself.

[David Kilzer (:ddkilzer)]

Comment on attachment 11888 [details]
proposed fix

Clearing darin's r+ on attachment 11888 [details] so this doesn't show up in the commit queue.

[Mark Rowe (bdash)]

It would be great to see some progress with Alexey's patch.  The original change in r18320 has caused quite a serious regression at mapquest.com (bug 11936) which I will need to roll out in the near future unless it is addressed.

It looks like the sticking point on the latest patch is the whether the property should be removed or left untouched when the value is not parseable?  For compatibility, I would suggest it should be removed as we  did prior to r18320.

[Geoffrey Garen]

Comment on attachment 12010 [details]
without a Dashboard quirk

I like the change to move this into the DOM!

I think Darin is right, though: We need to maintain the old behavior of unconditionally removing the property, then setting it only if it parses correctly.

[Alexey Proskuryakov]

(In reply to comment #30)
> I think Darin is right, though: We need to maintain the old behavior of
> unconditionally removing the property, then setting it only if it parses
> correctly.

Then the previous version of the patch is what you want, right?

[Geoffrey Garen]

Comment on attachment 12010 [details]
without a Dashboard quirk

I'm going to approve this patch.

ap mentioned on IRC that neither IE nor Firefox deletes the property when it fails to parse, so I think this is a compatibilty improvement. Let's not add a Dashboard quirk until it proves necessary.

Please remember to do these things before landing:
- Remove mention of the quirk from the ChangeLog
- Change to ASSERT(!ec), to match our style guidelines
- Add a test the verifies that a syntax error does not remove the previous property (i don't think that was covered by the original patch)

[Alexey Proskuryakov]

Committed revision 18618.

(In reply to comment #32)
> - Remove mention of the quirk from the ChangeLog

  Done.

> - Change to ASSERT(!ec), to match our style guidelines

  Done.

> - Add a test the verifies that a syntax error does not remove the previous
> property (i don't think that was covered by the original patch)

  Actually, it is already covered - the test dumps complete cssText after assignment.

[Geoffrey Garen]

Some fallout:

<rdar://problem/5111539> -- an Apple internal website relying on WebKit's old behavior, using an unparseable style rule to clear a style.

<rdar://problem/5011784> -- a WebKit app using undefined to clear a style rule.