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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

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

Comment 1

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

er, sorry, the latter bug should have been bug 549809

Comment 2

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

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.

Comment 3

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

(And in GetValue we should, of course, remove the code that refuses to serialize when background-size is not its default value.)

Comment 5

[Anthony Ricaud (:rik)]

Attachment: WIP patch

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.

Comment 6

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

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

Comment 7

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

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.

Comment 8

[Anthony Ricaud (:rik)]

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

Comment 9

[Anthony Ricaud (:rik)]

Attachment: Patch v1

Ok, I fixed (1) and (2), simplified the algorithm and added some tests.

I'm not sure to understand (3) though.

Comment 10

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

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

Comment 11

[Anthony Ricaud (:rik)]

Attachment: Patch v2

Adds one more initial value.

Comment 12

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

Comment on attachment 661522 [details] [diff] [review]
Patch v2

You didn't need to request review again, but r=dbaron.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Pushed to Try. Will land if all's green.
https://tbpl.mozilla.org/?tree=Try&rev=eab52439426a

Comment 14

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen from comment #13)
> https://tbpl.mozilla.org/?tree=Try&rev=eab52439426a

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/69e42083a29a

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/69e42083a29a

Comment 16

[Jean-Yves Perrier [:teoli]]

Doc updated:
https://developer.mozilla.org/en-US/docs/CSS/background
https://developer.mozilla.org/en-US/docs/Firefox_18_for_developers