383488 - style.setProperty(prop,value,"important") accepts trailing tokens in value

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mats Palmgren (inactive)]

Attachment: Testcase

style.setProperty(prop,value,"important") accepts trailing tokens in value.

(Followup from bug 383075)

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 2

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

This seems to work but the comment makes me worried I'm missing
something.

Comment 3

[Mats Palmgren (inactive)]

... should have bumped the IID on nsICSSParser too ...

Comment 4

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

Also, out-params should generally go at the end, so your new aIsImportant should go before the existing aChanged.

Hopefully a more complete review to follow shortly...

Comment 5

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

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

Given comment 3, comment 4, and 

>   if (aPriority.IsEmpty()) {
>-    return ParsePropertyValue(propID, aValue);
>+    return ParsePropertyValue(propID, aValue, PR_FALSE);
>+  }
>+  else if (aPriority.EqualsLiteral("important")) {
>+    return ParsePropertyValue(propID, aValue, PR_TRUE);
>   }

Change the "else if" here to an "if".

And maybe add at least an XXX comment below that we should put a CSS parser error on the error console if the priority is unknown?  Maybe even a followup bug/patch?

r+sr=dbaron with that, and sorry for the delay

Comment 6

[Zack Weinberg (:zwol)]

Attachment: updated patch with reftest

I noticed this bug while filing bug 556661.  I've taken the liberty of updating the patch to the latest tree and converting the original test case into a reftest.  Very few changes to the code were required so I feel fairly comfortable carrying dbaron's old r+ forward, but I shan't land it immediately in case someone disagrees with me.

Comment 7

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

Comment on attachment 436635 [details] [diff] [review]
updated patch with reftest

looks fine

Comment 8

[Zack Weinberg (:zwol)]

Landed http://hg.mozilla.org/mozilla-central/rev/60360c37c700

I'm going to bed - don't hesitate to back out if it causes trouble.