Last Comment Bug 570326 - add support for background-size in background shorthand property (css3-background)
: add support for background-size in background shorthand property (css3-backgr...
Status: RESOLVED FIXED
[good first bug][mentor=jwalden/Waldo]
: css3, dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla18
Assigned To: Anthony Ricaud (:rik)
:
Mentors:
: 720945 (view as bug list)
Depends on:
Blocks: css3-background
  Show dependency treegraph
 
Reported: 2010-06-05 09:40 PDT by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2012-09-29 09:26 PDT (History)
11 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP patch (13.39 KB, patch)
2012-09-14 20:58 PDT, Anthony Ricaud (:rik)
no flags Details | Diff | Review
Patch v1 (19.27 KB, patch)
2012-09-15 10:18 PDT, Anthony Ricaud (:rik)
dbaron: review+
Details | Diff | Review
Patch v2 (19.32 KB, patch)
2012-09-15 14:38 PDT, Anthony Ricaud (:rik)
dbaron: review+
Details | Diff | Review

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-06-05 09:40:47 PDT
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-7 (review requests must explain patch) 2010-06-05 09:41:17 PDT
er, sorry, the latter bug should have been bug 549809
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-06-08 21:31:33 PDT
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-7 (review requests must explain patch) 2012-01-25 12:54:45 PST
(And in GetValue we should, of course, remove the code that refuses to serialize when background-size is not its default value.)
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-04-06 17:00:13 PDT
*** Bug 720945 has been marked as a duplicate of this bug. ***
Comment 5 Anthony Ricaud (:rik) 2012-09-14 20:58:02 PDT
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.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-09-15 00:44:09 PDT
(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-7 (review requests must explain patch) 2012-09-15 00:47:03 PDT
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) 2012-09-15 03:33:08 PDT
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) 2012-09-15 10:18:32 PDT
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 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-09-15 11:01:17 PDT
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) 2012-09-15 14:38:09 PDT
Created attachment 661522 [details] [diff] [review]
Patch v2

Adds one more initial value.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-09-15 15:36:05 PDT
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] 2012-09-15 20:17:02 PDT
Pushed to Try. Will land if all's green.
https://tbpl.mozilla.org/?tree=Try&rev=eab52439426a
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-09-16 17:23:34 PDT
(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] 2012-09-17 12:27:50 PDT
https://hg.mozilla.org/mozilla-central/rev/69e42083a29a

Note You need to log in before you can comment on or make changes to this bug.