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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Zack Weinberg (:zwol)]

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.

Comment 1

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

Are you sure this isn't expected behavior?

Comment 2

[Zack Weinberg (:zwol)]

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.

Comment 3

[Zack Weinberg (:zwol)]

Attachment: patch

Patch.

Comment 4

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

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

Comment 5

[Zack Weinberg (:zwol)]

Attachment: patch with additional test for !important + non in the same declaration

I didn't find any existing tests for that so here's a revision that adds it.

Comment 6

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/mozilla-central/rev/54d74759a24c

Comment 7

[Zack Weinberg (:zwol)]

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.)

Comment 8

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

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".

Comment 9

[Zack Weinberg (:zwol)]

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.

Comment 10

[Peter J. Sloetjes]

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)?

Comment 11

[Peter J. Sloetjes]

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?

Comment 12

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

(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.

Comment 13

[Zack Weinberg (:zwol)]

(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).

Comment 14

[Peter J. Sloetjes]

(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 [e-mail] 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.