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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[[e-mail]]

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.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Does this count as a dataloss bug? I wasn't sure whether to set the keyword.

Comment 2

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

Could you add a test to the end of layout/style/test/test_shorthand_property_getters ?  In particular:
 * test that the element.style.background 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 3

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

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?

Comment 4

[[e-mail]]

Attachment: With test fix

One of the existing tests doesn't use the default values for -moz-background-size and I updated its comment to reflect that.

Comment 5

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

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

Comment 6

[[e-mail]]

Attachment: Updated for review comments

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.

Comment 7

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

Comment on attachment 416300 [details] [diff] [review]
Updated for review comments

Oops, I should have caught that.  r=dbaron

Comment 8

[[e-mail]]

Pushed changeset 7886b4c68ea9 to mozilla-central.