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
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.
6 years ago
6 years ago
(And in GetValue we should, of course, remove the code that refuses to serialize when background-size is not its default value.)
Created attachment 661451 [details] [diff] [review] 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.
(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".
Created attachment 661509 [details] [diff] [review] Patch v1 Ok, I fixed (1) and (2), simplified the algorithm and added some tests. I'm not sure to understand (3) though.
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
Created attachment 661522 [details] [diff] [review] Patch v2 Adds one more initial value.
Comment on attachment 661522 [details] [diff] [review] Patch v2 You didn't need to request review again, but r=dbaron.
Pushed to Try. Will land if all's green. https://tbpl.mozilla.org/?tree=Try&rev=eab52439426a
(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
Doc updated: https://developer.mozilla.org/en-US/docs/CSS/background https://developer.mozilla.org/en-US/docs/Firefox_18_for_developers