Closed Bug 556661 Opened 12 years ago Closed 12 years ago

CSSStyleDeclaration::setProperty() fails to change !important properties

Categories

(Core :: CSS Parsing and Computation, defect)

x86
Linux
defect
Not set
minor

Tracking

()

RESOLVED FIXED

People

(Reporter: zwol, Assigned: zwol)

References

()

Details

Attachments

(1 file, 1 obsolete file)

The test case at the URL above demonstrates a bug in the (Javascript) setProperty() method on CSSStyleDeclaration objects: if you try to use it to wipe out an !important on a property, it silently fails.

We get this wrong at least as far back as 3.5.  In fact, the only browser that I've found to get this test case right is Opera 10.5, and even that fails if you don't change the values at the same time :-P

What I *think* is going on here is, each of the rules in the first stylesheet gives rise to a CSSStyleRuleImpl object pointing to a nsCSSDeclaration object, pointing in turn to a nsCSSCompressedDataBlock object for the !important property.  (The "normal" datablock slot is empty in this case.) When we scan the rule tree, we notice that each of the declarations holds !important properties, so we create a CSSImportantRule object which holds a *direct* pointer to the !important data block, and we add that to the rule tree as a separate thing.  So far so good.

When Javascript comes along and tries to mess with the properties through the nsDOMCSSDeclaration interface, it winds up calling nsCSSParser::ParseProperty().  That calls EnsureMutable() on the declaration, which clones its data block; the CSSImportantRule object is left dangling in the rule tree, pointing to the *original* data block.  I haven't fully grokked how DeclarationChanged() operates, but it appears not to zap that, either.

I don't think this is itself an important bug; however, I tripped over these dangling CSSImportantRule objects when working on bug 553456, in which nsCSSCompressedDataBlock ceases to exist as a separate object, and I was trying to get rid of EnsureMutable too.
Are you sure this isn't expected behavior?
I talked it over with bz on irc -- we think it's actually because of the way CSSParserImpl::TransferTempData() works, not any problem with the rule tree, and we also think that while the spec is a little vague, it makes most sense for setProperty(prop, value, "") to override a previous !important setting -- for one thing, the priority argument is more useful that way, and for another, it ought to be consistent with what happens if you removeProperty() first.
Attached patch patch (obsolete) — Splinter Review
Patch.
Assignee: nobody → zweinberg
Status: NEW → ASSIGNED
Attachment #436818 - Flags: review?(dbaron)
Comment on attachment 436818 [details] [diff] [review]
patch

Could you either (a) find an existing test or (b) write a new test that checks that:

p { color: green ! important; color: red }

makes the paragraph green?


r=dbaron with that
Attachment #436818 - Flags: review?(dbaron) → review+
I didn't find any existing tests for that so here's a revision that adds it.
Attachment #436818 - Attachment is obsolete: true
Attachment #437426 - Flags: review+
Bustage fix: http://hg.mozilla.org/mozilla-central/rev/a016ac686be0

(I *thought* I'd pushed this to try, but apparently that was some other set of patches.  Sorry for the trouble.)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
So I still think the old behavior is the expected behavior, and was arguing for it yesterday/today on www-style in the thread on "CSSStyleDeclaration: Setting only a value or a priority".
I still think it's more useful the new way (see comment 2). And I think your position depends much too strongly on details of both the spec and of Gecko, which nobody who's not a browser developer knows or cares about.

Frankly, I think most people would expect

   p { color: green ! important; color: red }

to make Ps red.
The problem in the declarative case is that we cannot even in principle detect empty strings that are appended to value strings. Hence both the CSS interpreter and setProperty currently infer a normal priority "" when "important" is not present, hence property declarations/assignments set both value and priority in all cases, hence the Ps should become red.

Still, when calling setProperty, we are wearing our imperative hats (and using the corresponding mental model), hence setProperty could still be allowed to leave a property value or priority unchanged in case the corresponding argument is not provided, <undefined>, or invalid.
Alternatively, CSSStyleDeclaration could be extended with setPropertyValue and setPropertyPriority functions, for imperative use only. Perhaps David Baron could comment on this last alternative (here or on the above-mentioned thread)?
Now the proposal at http://www.w3.org/TR/css3-cascade/ just arrived, which clearly specifies that the Ps in the above example should become green. On reflection, this makes perfect sense for the declarative case, and leaves us with the proposed extension of the imperative case, which is different and !important as well. David Baron?
(In reply to pjs.nl from comment #11)
> Now the proposal at http://www.w3.org/TR/css3-cascade/ just arrived, which
> clearly specifies that the Ps in the above example should become green.

This didn't "just arrive"; it's been clear since CSS 1.  See, say, http://www.w3.org/TR/REC-CSS1-961217#cascading-order from December 1996.

Prior to this bug's change, it sounds like setProperty matched the logic of "append a declaration to a declaration block" in all browsers except for Opera.  I think it still should.
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #12)
> (In reply to pjs.nl from comment #11)
> > Now the proposal at http://www.w3.org/TR/css3-cascade/ just arrived, which
> > clearly specifies that the Ps in the above example should become green.
> 
> This didn't "just arrive"; it's been clear since CSS 1.  See, say,
> http://www.w3.org/TR/REC-CSS1-961217#cascading-order from December 1996.

That elevates this to the status of too-much-trouble-to-change-now, so I'll drop that prong of the discussion, but I still think this semantic is abstractly wrong, confusing to style authors, and liable to cause headaches if and when we ever have other !annotations.

> Prior to this bug's change, it sounds like setProperty matched the logic of
> "append a declaration to a declaration block" in all browsers except for
> Opera.  I think it still should.

I cannot agree with this.  The semantic you want is strictly less useful than the semantic introduced with this bug's change, and, again, confusing to users of the API (it is *bizarre* for setProperty(prop, value, "") to be a nop when the existing decl for that property in that rule is !important; I'm quite confident that your average JS programmer will expect it to remove the !important and change the value).
(In reply to David Baron from comment #12):
What I meant was that the arrival of the new spec made me notice that "!important" is still interpreted as it was (and, on reflection, in the declarative case, should be).

I still regard your remarks on www-style@w3.org as irrelevant to the imperative use case. However, if modifying the setProperty function would be too involved (e.g. because of its use in browser internal code), I would be willing to advocate adding setPropertyValue and setPropertyPriority functions instead of changing the setProperty function, as is proposed now.
You need to log in before you can comment on or make changes to this bug.