Closed Bug 532617 Opened 12 years ago Closed 12 years ago

-moz-background-size does not serialise if all standard background properties are present and all other non-standard background properties have their default value


(Core :: CSS Parsing and Computation, defect)

Not set





(Reporter: neil, Assigned: neil)





(1 file, 2 obsolete files)

If you set the style attribute on an HTML node to "-moz-background-size: 100% 100%;" then it will serialise as such, but if you set it to "background: none repeat scroll 0% 0% transparent; -moz-background-size: 100% 100%;" then the -moz-background-size does not get serialised. If you also set a non-standard background property to a non-default value then the background shortcut is not used and all the properties again get serialised correctly.
Attached patch Proposed patch (obsolete) — Splinter Review
Does this count as a dataloss bug? I wasn't sure whether to set the keyword.
Assignee: nobody → neil
Attachment #415895 - Flags: review?(dbaron)
Could you add a test to the end of layout/style/test/test_shorthand_property_getters ?  In particular:
 * test that the getter returns the empty string once '-moz-background-size' is non-default
 * maybe also test the original case in this bug, though that's a little more work
Comment on attachment 415895 [details] [diff] [review]
Proposed patch

Also, given the big #if 0 block, I think it would actually be better to make this a separate if () { aValue.Truncate(); return NS_OK; }

Could you post a revised patch with that and the test?
Attached patch With test fix (obsolete) — Splinter Review
One of the existing tests doesn't use the default values for -moz-background-size and I updated its comment to reflect that.
Attachment #415895 - Attachment is obsolete: true
Attachment #416159 - Flags: review?(dbaron)
Attachment #415895 - Flags: review?(dbaron)
Comment on attachment 416159 [details] [diff] [review]
With test fix

>+        if (size->mXValue.GetUnit() != eCSSUnit_Auto ||
>+            size->mXValue.GetUnit() != eCSSUnit_Auto) {

You should make the second of these checks test size->mYValue.

You should probably also make the test test "100% auto" and "auto 100%" in addition to testing "100% 100%".

r=dbaron with that
Attachment #416159 - Flags: review?(dbaron) → review+
I just realised that the patch made some of the other tests test the wrong thing, as they would bail from an unsupported background size rather than what they were testing for, so hopefully this should fix those tests too.
Attachment #416159 - Attachment is obsolete: true
Attachment #416300 - Flags: review?(dbaron)
Attachment #416300 - Flags: review?(dbaron) → review+
Comment on attachment 416300 [details] [diff] [review]
Updated for review comments

Oops, I should have caught that.  r=dbaron
Pushed changeset 7886b4c68ea9 to mozilla-central.
Closed: 12 years ago
Resolution: --- → FIXED
Duplicate of this bug: 526602
You need to log in before you can comment on or make changes to this bug.