383075 - "red;" (with a semicolon) should not be accepted as a color

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Setting elem.style.background to "red;" or even "red; yellow" turns the element red.  I think it should have no effect, like in WebKit trunk and Opera, and cause an error message to appear in the error console.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase #2

It affects all properties.

Comment 2

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 267138 [details] [diff] [review]
Patch rev. 1

>+    if (parsedOK) {
>+      // Junk at end of property value.
>+      REPORT_UNEXPECTED_TOKEN(PEExpectEndProperty);
>+      errorCode = NS_ERROR_FAILURE;
>+    }

This should come at the very beginning of the else to be consistent with other types of error reporting, I think.

With that, and if you add a mochitest for the bug, r+sr=dbaron.

Comment 4

[Jesse Ruderman]

Should the mochitest for this bug be in the form of modifying the existing comprehensive tests (/layout/style/test/) to try every valid property value with a semicolon at the end, and make sure it's not accepted?  Or would that be overkill?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Please remove the XXXbz comment before the function in question, since you're fixing the thing it complains about?

And doesn't this change make such sets throw NS_ERROR_FAILURE?  Is that really what we want?  I'd think not...

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch with comments addressed

1. remove the XXX comment
2. move the error message as dbaron suggested
3. leave errorCode as is, which should be NS_OK in this case

Comment 7

[Mats Palmgren (inactive)]

Attachment: Mochitest: layout/style/test/test_bug383075.html

Comment 8

[Mats Palmgren (inactive)]

The mochitest tests both style.prop=val and style.setProperty(..., null)
but it doesn't test style.setProperty(..., "important") because that takes
a different path through the code and we have bugs there too.
I'll file a separate bug on that.

Comment 9

[Mats Palmgren (inactive)]

Checked in to trunk at 2007-06-06 10:56 PDT.

-> FIXED

Comment 10

[Mats Palmgren (inactive)]

Filed bug 383488 on style.setProperty(..., "important")