Last Comment Bug 383075 - "red;" (with a semicolon) should not be accepted as a color
: "red;" (with a semicolon) should not be accepted as a color
Status: RESOLVED FIXED
: testcase
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9alpha6
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on:
Blocks: 441555
  Show dependency treegraph
 
Reported: 2007-06-03 14:13 PDT by Jesse Ruderman
Modified: 2008-06-25 10:30 PDT (History)
4 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (666 bytes, text/html)
2007-06-03 14:13 PDT, Jesse Ruderman
no flags Details
Testcase #2 (1.44 KB, text/html)
2007-06-04 05:41 PDT, Mats Palmgren (:mats)
no flags Details
Patch rev. 1 (1.58 KB, patch)
2007-06-04 05:45 PDT, Mats Palmgren (:mats)
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review
Patch with comments addressed (2.54 KB, patch)
2007-06-06 01:43 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Mochitest: layout/style/test/test_bug383075.html (4.59 KB, patch)
2007-06-06 01:44 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review

Description Jesse Ruderman 2007-06-03 14:13:46 PDT
Created attachment 267076 [details]
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 (:mats) 2007-06-04 05:41:45 PDT
Created attachment 267137 [details]
Testcase #2

It affects all properties.
Comment 2 Mats Palmgren (:mats) 2007-06-04 05:45:13 PDT
Created attachment 267138 [details] [diff] [review]
Patch rev. 1
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-06-04 12:06:27 PDT
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 2007-06-04 12:16:03 PDT
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 [:bz] 2007-06-05 21:57:38 PDT
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 (:mats) 2007-06-06 01:43:11 PDT
Created attachment 267395 [details] [diff] [review]
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 (:mats) 2007-06-06 01:44:42 PDT
Created attachment 267396 [details] [diff] [review]
Mochitest:  layout/style/test/test_bug383075.html
Comment 8 Mats Palmgren (:mats) 2007-06-06 01:50:45 PDT
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 (:mats) 2007-06-06 11:46:28 PDT
Checked in to trunk at 2007-06-06 10:56 PDT.

-> FIXED
Comment 10 Mats Palmgren (:mats) 2007-06-06 12:20:18 PDT
Filed bug 383488 on style.setProperty(..., "important")

Note You need to log in before you can comment on or make changes to this bug.