Closed Bug 570326 Opened 14 years ago Closed 12 years ago

add support for background-size in background shorthand property (css3-background)

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: dbaron, Assigned: rik)

References

(Blocks 1 open bug)

Details

(Keywords: css3, dev-doc-complete, Whiteboard: [good first bug][mentor=jwalden/Waldo])

Attachments

(1 file, 2 obsolete files)

We should extend our support of the background shorthand property to support 'background-size'.  However, we should probably wait on this a little bit since it's one of the least stable parts of css3-background (it's going to change between the current CR and the upcoming last call).

(See also bug 189519, where background-size was implemented, and bug 451134, where we rename -moz-background-size to background-size.)
er, sorry, the latter bug should have been bug 549809
Keywords: css3
Summary: add support for background-size in background shorthand property → add support for background-size in background shorthand property (css3-background)
This requires code changes in two places: nsCSSParser::ParseBackgroundItem for parsing and nsCSSDeclaration::GetValue for the getter/serialization case, plus adjusting or adding relevant tests.
Whiteboard: [good-first-bug][mentor=jwalden/Waldo]
Whiteboard: [good-first-bug][mentor=jwalden/Waldo] → [good first bug][mentor=jwalden/Waldo]
(And in GetValue we should, of course, remove the code that refuses to serialize when background-size is not its default value.)
Attached patch WIP patch (obsolete) — Splinter Review
So I tried to work on this. The parsing is working but not the serialisation.

I get 49 errors in layout/style/tests/test_value_storage.html. They look like this:
failed | parse+serialize should be idempotent for 'background: transparent' - got , expected none repeat scroll 0% 0% / auto auto transparent

I don't see the difference in my code with other parts of the shorthand so I must be missing something.
(In reply to Anthony Ricaud (:rik) [out between Sep 8 and Sep 16]] from comment #5)
> So I tried to work on this. The parsing is working but not the serialisation.
> 
> I get 49 errors in layout/style/tests/test_value_storage.html. They look
> like this:
> failed | parse+serialize should be idempotent for 'background: transparent'
> - got , expected none repeat scroll 0% 0% / auto auto transparent
> 
> I don't see the difference in my code with other parts of the shorthand so I
> must be missing something.

So, basically, what that's testing is that we set:
  elt.style.background = "transparent";
then we serialize it:
  var step1val = elt.style.background;
Based on the message above, step1val is "none repeat scroll 0% 0% / auto auto transparent".

Then we call setProperty again, with the value we got out:
  elt.style.background = step1val;
and serialize it again:
  var step2val = elt.style.background;
and assert that step2val is the same as step1val.


Since the serialization looks right, I suspect the problem is actually in the parsing; it seems like there's a bug that the shorthand parsing code can't parse "none repeat scroll 0% 0% / auto auto transparent" correctly.

That said, I think you should *also* make the change so that we only output the background-size part of the shorthand when it has a non-default value.  That would cover up the previous bug (i.e., there would no longer be any failing tests as a result), so I recommend:
 (1) fix the parsing bug that the test failure shows
 (2) change the modification you made to Declaration::GetValue to only output the value when there's a non-default value (i.e., put the new code inside the same condition that you removed above)
 (3) add a test so the bug fixed in (1) still causes a failure somehow, probably by adding it to the other_values line in property_database.js for the background property.  And probably some more tests, too.
I also think the parsing code you have looks a good bit more complicated than it needs to be.  You shouldn't need all those new variables -- and, in fact, you should be able to just rename havePosition to havePositionAndSize, since they have to come together.
Thanks, I understand the test output better now. The important step is that the property is also reset between step1val and step2val. So the failure reads like "got (empty string), expected none repeat scroll 0% 0% / auto auto transparent".
Attached patch Patch v1 (obsolete) — Splinter Review
Ok, I fixed (1) and (2), simplified the algorithm and added some tests.

I'm not sure to understand (3) though.
Assignee: nobody → anthony
Attachment #661451 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #661509 - Flags: review?(dbaron)
Comment on attachment 661509 [details] [diff] [review]
Patch v1

Oops, I should have said initial_values rather than other_values.

Could you add "none repeat scroll 0% 0% / auto auto transparent" to the initial_values line in property_database.js (in addition the the other additions you've made, which are good)?

r=dbaron with that
Attachment #661509 - Flags: review?(dbaron) → review+
Attached patch Patch v2Splinter Review
Adds one more initial value.
Attachment #661509 - Attachment is obsolete: true
Attachment #661522 - Flags: review?(dbaron)
Comment on attachment 661522 [details] [diff] [review]
Patch v2

You didn't need to request review again, but r=dbaron.
Attachment #661522 - Flags: review?(dbaron) → review+
Keywords: checkin-needed
Pushed to Try. Will land if all's green.
https://tbpl.mozilla.org/?tree=Try&rev=eab52439426a
https://hg.mozilla.org/mozilla-central/rev/69e42083a29a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=jwalden/Waldo] → [good first bug][mentor=jwalden/Waldo]
Target Milestone: --- → mozilla18
Whiteboard: [good first bug][mentor=jwalden/Waldo] → [good first bug][mentor=jwalden/Waldo]
You need to log in before you can comment on or make changes to this bug.