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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: neil, Assigned: neil)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment, 2 obsolete attachments)

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.
Posted 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
Status: NEW → ASSIGNED
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 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 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?
Posted 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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.