The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.9alpha6

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Jesse Ruderman, Assigned: mats)

Tracking

({testcase})

Trunk
mozilla1.9alpha6
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Reporter)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
Created attachment 267137 [details]
Testcase #2

It affects all properties.
Assignee: nobody → mats.palmgren
Status: NEW → ASSIGNED
(Assignee)

Comment 2

10 years ago
Created attachment 267138 [details] [diff] [review]
Patch rev. 1
Attachment #267138 - Flags: superreview?(dbaron)
Attachment #267138 - Flags: review?(dbaron)
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.
Attachment #267138 - Flags: superreview?(dbaron)
Attachment #267138 - Flags: superreview+
Attachment #267138 - Flags: review?(dbaron)
Attachment #267138 - Flags: review+
(Reporter)

Comment 4

10 years ago
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?
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...
(Assignee)

Updated

10 years ago
Flags: in-testsuite?
OS: Mac OS X → All
Hardware: PC → All
(Assignee)

Comment 6

10 years ago
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
(Assignee)

Comment 7

10 years ago
Created attachment 267396 [details] [diff] [review]
Mochitest:  layout/style/test/test_bug383075.html
(Assignee)

Comment 8

10 years ago
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.
(Assignee)

Comment 9

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

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha6
(Assignee)

Comment 10

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

Updated

9 years ago
Depends on: 441555
(Assignee)

Updated

9 years ago
Blocks: 441555
No longer depends on: 441555
You need to log in before you can comment on or make changes to this bug.