Last Comment Bug 322475 - Implement support for CSS3 multiple backgrounds
: Implement support for CSS3 multiple backgrounds
Status: RESOLVED FIXED
: css3, dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P4 enhancement with 33 votes (vote)
: mozilla1.9.2a1
Assigned To: David Baron :dbaron: ⌚️UTC-10
:
: Jet Villegas (:jet)
Mentors:
http://www.css3.info/wp-content/uploa...
: 494404 (view as bug list)
Depends on: 676637 459649 460796 461731 479402 482708 482941 494808 552538
Blocks: css3-background 446328
  Show dependency treegraph
 
Reported: 2006-01-05 06:23 PST by Stefan Thomas
Modified: 2012-03-10 05:28 PST (History)
62 users (show)
roc: blocking1.9.1-
roc: wanted1.9.1-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
pre-patch: merge nsCSSQuotes and nsCSSCounterData into nsCSSValuePairList (42.09 KB, patch)
2008-08-01 22:25 PDT, David Baron :dbaron: ⌚️UTC-10
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
work in progress (51.69 KB, patch)
2008-09-14 19:50 PDT, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
work in progress (64.75 KB, patch)
2008-09-16 13:28 PDT, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
work in progress (71.97 KB, patch)
2008-09-24 20:35 PDT, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch 1: allow arbitrary number of image loaders (21.93 KB, patch)
2008-09-25 17:19 PDT, David Baron :dbaron: ⌚️UTC-10
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
patch 2: implement multiple backgrounds (117.28 KB, patch)
2008-09-25 17:20 PDT, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch 2: implement multiple backgrounds (128.54 KB, patch)
2008-09-25 18:41 PDT, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch 2: implement multiple backgrounds (124.46 KB, patch)
2008-10-11 17:53 PDT, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch 2: implement multiple backgrounds (136.81 KB, patch)
2008-10-27 08:20 PDT, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch 1: allow arbitrary number of image loaders (33.02 KB, patch)
2009-01-09 15:56 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch 2: implement multiple backgrounds (140.73 KB, patch)
2009-01-09 15:57 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch 1: allow arbitrary number of image loaders (23.93 KB, patch)
2009-01-18 18:20 PST, Michael Ventnor
no flags Details | Diff | Splinter Review
Implement multiple backgrounds (120.60 KB, patch)
2009-01-18 18:21 PST, Michael Ventnor
dbaron: review-
dbaron: superreview-
Details | Diff | Splinter Review
patch 2: implement multiple backgrounds (147.32 KB, patch)
2009-01-22 17:39 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch 1: allow arbitrary number of image loaders (33.04 KB, patch)
2009-01-30 12:11 PST, David Baron :dbaron: ⌚️UTC-10
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
patch 2: implement multiple backgrounds (168.01 KB, patch)
2009-01-30 12:12 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch 2: implement multiple backgrounds (167.18 KB, patch)
2009-02-10 21:13 PST, David Baron :dbaron: ⌚️UTC-10
roc: review+
bzbarsky: superreview-
Details | Diff | Splinter Review
patch 2: implement multiple backgrounds (179.99 KB, patch)
2009-02-17 13:12 PST, David Baron :dbaron: ⌚️UTC-10
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
tinderbox failures on mac unit test (719.13 KB, text/plain)
2009-02-19 14:47 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details

Description Stefan Thomas 2006-01-05 06:23:56 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8) Gecko/20051111 Firefox/1.5

Gecko has already support for various CSS3 extensions, most notably multiple-columns and opacity. Opacity is already widely used, because it does degrade gently.

I propose that Gecko should implement multiple backgrounds, because it is one of the most anticipated features of CSS3 and can be used to emulate at lot of other features (like shadows, CSS3-style borders, rounded corners etc.).

The syntax for this style is defined in the CSS3 Background & Borders module at:
http://www.w3.org/TR/2005/WD-css3-background-20050216/#layering

Alternatively, a proprietary syntax, based on -moz- could be used for now.

Reproducible: Always

Steps to Reproduce:
1. View the supplied website in any Gecko-based browser.

Actual Results:  
None of the images are displayed.

Expected Results:  
All of the images should be displayed.

The project I'm leading currently bundles Firefox, therefore we can safely use proprietary CSS elements (-moz-). Multiple backgrounds are something we really need, because the general workaround involves having a number of nested elements. But our page is already pretty complex (entirely built from JavaScript and also updated permanently), so this has significantly reduced performance (five times the number of elements). For now, we use a simpler design (with Firefox's rounded corners instead of images), but this is not satisfying. That's why we hope for help from the almighty Gecko. ;)
Comment 1 David Baron :dbaron: ⌚️UTC-10 2006-01-05 10:25:54 PST
FWIW, I'm not a big fan of the current proposal -- trying to describe everything in one line isn't always the best solution.  More on that later...
Comment 2 John A. Bilicki III 2006-04-10 16:38:47 PDT
I'd like to see this too however the only issue I can't seem to figure out is the W3C's regard to z-index.  If there are going to be multiple backgrounds then I'm more then 100% certain people are going to want control over which image overlaps the other if say a div with 100x100px size is used with background corners that are greater then 25px each.
Comment 3 David Baron :dbaron: ⌚️UTC-10 2006-04-10 17:28:18 PDT
The z-order is the order given (I forget whether it's top->bottom or bottom->top).
Comment 4 Michael K. 2006-04-11 06:41:03 PDT
(In reply to comment #3)
> (I forget whether it's top->bottom or
> bottom->top).

From the latest draft: "Images are drawn with the first specified one on top (closest to the user) and each subsequent image behind the previous one. The 'background-color' is painted below all the images."
Comment 5 jrblier 2007-05-26 20:02:59 PDT
More information here, where an example is also available:

http://www.css3.info/preview/multiple-backgrounds/
Comment 6 AndrewM 2007-10-26 09:55:49 PDT
I get a 404 for the URL...
Comment 7 jrblier 2007-10-26 16:30:20 PDT
I tried it and it is working.
Comment 8 AndrewM 2007-10-27 09:33:57 PDT
Sorry, I wasn't clear. The URL you posted (http://www.css3.info/preview/multiple-backgrounds/) is working fine. I was meaning the URL in the URL field of the bug
(http://petertoh.com/images/multi-bk.html). I get a 404 for that. It would probably be best to replace that one with the css3.info one (I don't have the privileges to do that :)
Comment 9 Just Me 2008-03-06 06:37:26 PST
(In reply to comment #1)
> FWIW, I'm not a big fan of the current proposal -- trying to describe
> everything in one line isn't always the best solution.  More on that later...

Now that we're two years later, I guess we're "later enough" :-)

What's your problem with the current draft? To me it seems fine, it can degrade gracefully to a single background if needed, e.g. http://css-class.com/test/css/backgrounds/multiple-backgrounds.htm . For some reason the author put a backslash in his CSS, but it works just the same if you remove it: single background on Gecko & IE7, multiple backgrounds on Safari.
Comment 10 David Baron :dbaron: ⌚️UTC-10 2008-03-06 08:24:47 PST
Problems I was referring to:
* it crams too much information into a single property value
* it isn't going to extend well to the next things we'd want to do with backgrounds
* I sort of think background images were designed wrong to begin with since the image often isn't meaningfully separable from the repeat/position/etc. so you really want all the properties to cascade together

I had an alternative proposal partly written at http://dbaron.org/css/css-vg/ .
Comment 11 Jonathan Haas 2008-06-23 10:30:18 PDT
I propose to implement this for Mozilla 1.9.1 as other major CSS3-Properties are implemented, too, for 1.9.1.

Also, Safari 3.1.1 supports this, could someone add the [parity-safari] keyword?
Comment 12 John A. Bilicki III 2008-06-26 17:32:47 PDT
@ David, if you dislike the current status then don't implement it as a final standard, simply implement it via a proprietary manner using the "-moz" prefix. Even if it becomes botched in the future it would still mean we have some support versus a complete lack of support. Besides if you truly dislike long lines it won't break CSS when you add break lines to make it by your taste readable. Once the standard is finalized for this property *then* implement it without the proprietary prefix. This would be much more reasonable and forward thinking then not having any support for it at all.
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-07-13 22:10:23 PDT
We plan to support two features in Gecko 1.9.1 which might at least partially obviate the need for this:
http://weblogs.mozillazine.org/roc/archives/2008/07/the_latest_feat.html
http://weblogs.mozillazine.org/roc/archives/2008/07/svg_paint_serve.html
Comment 14 John A. Bilicki III 2008-07-13 22:27:57 PDT
Robert, are you suggesting that element-as-background could be used to apply a second background-image in addition to the default? I'm not seeing the relation between what you posted and multiple background-images in any other way.
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-07-13 22:47:08 PDT
OK, tell me what you want to use multiple backgrounds for.
Comment 16 John A. Bilicki III 2008-07-13 23:20:28 PDT
Dynamic layouts for one! With 24inch screens sporting 1920x1200 resolution nearing the $300 threshold and 22inch screens becoming the norm it's becoming more and more important to create pages that stretch to various resolutions. Is not proper (X)HTML/CSS supposed to emulate a newspaper in a sense with headers, images, and paragraphs? It's obvious that the web is even more then just a newspaper but that it does in essence build upon such a layout's strength's.

Let's not forget what the roles of (X)HTML and CSS are: (X)HTML is in essence strictly a noun language while CSS is strictly an adjective language (and while we're at it JavaScript in a sense a verb language). So why should we have to create divisible elements to place graphic background-images? This goes against many of the things we work against as web designers (and web developers who are forced to do clientside). It's bad for SEO because it increases the ratio of code to content. It's bad for semantics because it creates unnecessary code that may tarnish a search engine's understanding of the structure of a website (lots of mysteriously empty elements in example). It increases the need to fix and adjust multiple elements if the layout is changed. It can also reduce bandwidth by replacing large static images for fixed layouts and allow for unique items in the same block element (that may contain multiple columns of text) such as a column divider. It further reduces bandwidth and even minor amounts of server load by the server having to produce less code (the more complicated the design/layout the more savings). It also allows for greater creativity and ability to express one's artistic skills by for example using an APNG image such as grass at the bottom of an element along with a repeating seamless transparent GIF of rain or snow to give the page an organic feeling.

I could go on and on all night for the endless applications and benefits of having *some* / *any* support for multiple background-images.

...and again like I mentioned earlier even off-spec proprietary support would be *very* welcome versus a complete lack of support whatsoever. All things considered this is one of ***the*** most important parts of CSS3. Three dozen votes can't be wrong.
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-07-14 00:58:38 PDT
OK, from all that I can extract one use-case, which is to have a background consisting of multiple repeating elements with different periods.

Many kinds of complex backgrounds like that could be done using SVG background images. I wonder how that approach would compare in practice.
Comment 18 philippe (part-time) 2008-07-14 01:44:22 PDT
The example linked form
http://www.css3.info/preview/multiple-backgrounds/
is an interesting use case.

That said, most demand I see for multiple backgrounds is to reduce the number of (useless) divs used to create an effect. 

That said, if I get border-image, that would seriously reduce the (my) need for multiple backgrounds (and we already have box-shadow in Gecko 1.9.1, yay, shadows being another of those use cases for multiple backgrounds)

The use of SVG does sound *very* interesting. I have your special build at hand, haven't played much yet - doesn't help that my SVG skills are not yet up to par.
Comment 19 Kevin Cannon 2008-07-14 03:41:51 PDT
Rob - There is a very large demand for multiple background images within the web design community. It's also in the CSS3 spec for a reason. I would say that I would bump up against that limitation on every second web project I work on. There's always hacks and methods of getting around it, but they usually require extra markup or a design change.

One of the main benefits of multiple BG images is the ability to add images onto each corner of the div and have everything scale nicely. Similarly, with the body element, you can apply a background image to the top of the page and the bottom - impossible now, without extra markup and other hacks.

CSS3 multiple background images will open up a lot of options for designers, many of which will not even be guessable until the feature is given to them. Trust the professionals when they say they want this feature. Firefox is lagging behind Safari these days, it would be nice if Firefox was the leader again.
Comment 20 John A. Bilicki III 2008-07-14 11:47:41 PDT
SVG is a great tool but it's not nearly as practical as multiple background-image support. Some people may actually have the skill/time to convert all of their site's images to SVG but at this moment in time that would be a minority of people. Even if most people could honestly you can't convert everything to SVG. I'm not trying to demean SVG in any way but it's simply just not a practical and necessary feature in comparison to multiple background-image support.

Another example: you've got to setup a Halloween page for a school event. They say they want cobwebs in all the corners and a little spider dangling from the top about 10% from the left. That alone would take a bit of time to setup multiple divisible elements (breaking semantics in the process) and wasting the designer's time.

Now imagine setting up a website for a computer game such as World of Warcraft. Such layouts are so complex I'm not surprised they went with a table layout.


...and as far as competition goes there is still a lot of stuff I've been meaning to add to my site for in WebKit but I can't in Firefox but the differences should be clear to anyone who knows how to look for them...
http://www.jabcreations.com/blog/?backgroundimages=1&connection=2&css3=1

It's all about practicality: this is a feature that we can use in the here and now and it's not necessarily all about fun and games either.
Comment 21 David Baron :dbaron: ⌚️UTC-10 2008-07-25 23:36:13 PDT
I think a major use case people have is to do http://www.alistapart.com/articles/slidingdoors/ better.  (Never mind that sliding doors is itself really a workaround for a lack of the box models that people want, perhaps combined with a lack of border-image support.)  I'd note that a good bit of what people want there can be done with (-moz-)border-image, which we recently added.

That said, I'm ok with just doing this.  The tricky part is probably dealing with nsPresContext::LoadImage and related code, which assume one background image per frame.  Most of the other places that need changing (converting to appropriate loops) will probably be caught by the compiler as a result of the datatype changes in nsCSSColor/nsStyleBackground.  The one interesting data structure question is whether (in nsStyleBackground) we want a single linked list or one for each property.  (In nsCSSColor we certainly want one for each property.)
Comment 22 David Baron :dbaron: ⌚️UTC-10 2008-07-29 15:05:50 PDT
(In reply to comment #21)
> datatype changes in nsCSSColor/nsStyleBackground.  The one interesting data
> structure question is whether (in nsStyleBackground) we want a single linked
> list or one for each property.  (In nsCSSColor we certainly want one for each
> property.)

I think we want one for each property because of inheritance; given a long list for a single one of the subproperties and a descendant that inherits a different one of the subproperties, we want the shorter length.
Comment 23 Robin Winslow 2008-07-30 13:45:14 PDT
@ david baron.

It never occurred to me that multiple backgrounds could be used for sliding doors. I don't even know how it would work.

I want to use multiple backgrounds for rounded boxes, and it allows far more flexibility than using border-image, and makes much more sense. It seems to me that multiple backgrounds just makes a lot of sense, and would be the most elegent solution to a whole host of problems. Assuming it's possible, it should definitely be implemented.
Comment 24 David Baron :dbaron: ⌚️UTC-10 2008-08-01 22:25:04 PDT
Created attachment 332034 [details] [diff] [review]
pre-patch: merge nsCSSQuotes and nsCSSCounterData into nsCSSValuePairList

Mochitests in layout/style pass, although I should probably write a test for the nsHTMLCSSStyleSheet code...
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2008-08-04 13:33:29 PDT
Comment on attachment 332034 [details] [diff] [review]
pre-patch: merge nsCSSQuotes and nsCSSCounterData into nsCSSValuePairList

>+++ b/layout/style/nsCSSDataBlock.cpp
>  * CSS declaration blocks.  The value is stored in one of the six CSS

s/six/five/

r+sr=bzbarsky with that
Comment 26 David Baron :dbaron: ⌚️UTC-10 2008-08-04 15:17:09 PDT
I reworded it so it doesn't repeat the number twice, and landed: http://hg.mozilla.org/mozilla-central/index.cgi/rev/6ad2e5aa3c64
Comment 27 Robin Winslow 2008-09-09 03:24:40 PDT
Hiya.

I've been carefully watching the progress of this bug because I genuinely can't wait until the day that Firefox finally supports this. It is definitely the most desirable enhancement out there.

I don't really understand those last 3 comments. Where are we at with this? Is it now actually being worked on? Is there a build I could download that has it in?

I would also like to point out that not only does Safari support it, but now also Google Chrome - which has already surpassed Safari in the market (after about a week).

If Gecko is genuinely going to compete with Webkit, it *needs* this.

Robin.
Comment 28 Robin Winslow 2008-09-09 03:27:29 PDT
Or I suppose Firefox could simply switch to using the Webkit rendering engine.
:p
Comment 29 John A. Bilicki III 2008-09-09 03:31:47 PDT
@Robin

It looks like David Baron is our savior though I'm not sure what the status is on the bug. At the top look at "assigned to".

Additionally look for "blocking" which this does not seem to be blocking for the next version of Firefox.

I would recommend having your web savvy friends register and vote for this bug. I'm not sure what would get a feature request to block a version seeing as I would suspect it's usually crash or security issue related.

Also please keep unnecessary comments to a minimal...two messages mean over sixty emails were just sent, mine makes ninety.

@David Baron

How is work on this progressing? I too would like to see this implemented ASAP. If there is something we can do to help please let us know. :-)
Comment 30 timeless 2008-09-09 04:39:29 PDT
john, robin, please read: https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
the bug is owned by engineers and they're trying to use it to do work. your comments (as well as mine) don't belong in this bug. when it's done, the bug will be resolved as fixed (see https://bugzilla.mozilla.org/page.cgi?id=fields.html#status).

you can already see from flags that this is not going to block any currently planned release of gecko, however when it's ready it will presumably go in.

personally, i think http://www.css3.info/preview/multiple-backgrounds/ from 
comment 18 is a terrible example and in fact is bad css (the content isn't properly accessible and the piece it's happy about including is something which should be accessible). at best as dbaron says it's an argument for border-image, and sliding doors was just the description of a technique, the example was applied to tabs (read it).

if you want to be helpful, you could find a couple of web sites which actually use this feature for something useful (and accessible - i.e. not misusing css to exclude users from access to content which should have been exposed to html) which can't be solved by border-image (or an enhanced box object). in a single comment, mention 5 useful live urls, no description, just urls.
Comment 31 Robin Winslow 2008-09-09 08:07:58 PDT
First, my thanks to David Baron for working on this.

Okay. As I am instructed to provide useful links:

- Good discussions of good uses for multiple backgrounds:
http://abeautifulsite.net/notebook/20
http://24ways.org/2006/rounded-corner-boxes-the-css3-way
- Another situation where it could have been used, and complaint at not having the option:
http://www.htmldog.com/ptg/archives/000107.php
- My original post on the subject, all those moons ago:
http://groups.google.com/group/mozilla.dev.tech.css/browse_thread/thread/f8ff2b969ec7a725/0534326100f946ee?lnk=raot&fwc=1

@timeless
I am rather shocked at this rebuttal.

Surely the point of having an open bug-tracker system si so that anyone who is interested can comment on the job. And is asking for a progress update not a perfectly justified reason for commenting?

Open discussion by anybody is a corner-stone principle of the Open-Source community and is the reason that this open bug-tracker exists. 

And people have subscribed to this bug because they are interested in that open discussion, so I hardly think they are going to mind receiving a couple of comments from me - you never know they might even appreciate my request for a status update, and might perhaps be interested to know that Google Chrome supports this feature and is rapidly gaining popularity. 

And as for 30 emails being sent every time I comment - are you seriously telling me mozilla.org's servers can't handle it?

But as it seems I got the spirit of the people on this bug completely wrong, you don't have to worry fellas, this will be my last post.

Kind regards, and once again my thanks to David Baron.

Cheers,
Robin.
Comment 32 Unknown W. Brackets 2008-09-10 01:05:34 PDT
Robin, none of those links actually specify a use that *isn't* better achieved using border-image.  Probably, border-image is the correct way to solve all of those problems.  Please take a look at it here:

http://ejohn.org/blog/border-image-in-firefox/

Anyway, for my part, border-image covers most of the things I want to do (that said, multiple backgrounds probably is an easier, more foolproof way, if inefficient...)

Most of the useful interactions of multiple backgrounds have to do with, like:
  - partially transparent PNGs.  Gradients, for example.
  - hover effects (e.g. an element that has one background normally, and two on hover.)
  - page backgrounds (if one background can be fixed and another not, for example.)

That and they make sense as soon as you look at them (which will never be said about border-image.)  But almost all of the useful things people want them for can do can be done using border-image.

-[Unknown]
Comment 33 Neil Skrypuch 2008-09-13 22:07:29 PDT
One use case for multiple backgrounds that border-image doesn't satisfy would be multiple backgrounds in the *content* area of an element. As far as I know, border-image doesn't extend past the border area of the box. For example, say you wanted to have a tree branch extending into the content area on the left and the right of an element, and the element is variable width.

I agree that border-image makes very common things like rounded corners and dropshadows easy, though.
Comment 34 John A. Bilicki III 2008-09-14 09:10:04 PDT
The border-image property should not be used to create a border-radius, that is after all what border-radius is for! Additionally I find it horribly bad etiquette for any one to be posting AGAINST implementing this feature. If you want to advocate border-images then do it in the appropriate bug.

 - Not Unknown
Comment 35 David Baron :dbaron: ⌚️UTC-10 2008-09-14 09:27:43 PDT
All these comments are not helping.  They just make the bug report harder to read.
Comment 36 David Baron :dbaron: ⌚️UTC-10 2008-09-14 19:50:28 PDT
Created attachment 338576 [details] [diff] [review]
work in progress

This has most of the style system changes (no nsComputedDOMStyle changes).  No tests yet, and no painting changes or other layout changes yet.
Comment 37 David Baron :dbaron: ⌚️UTC-10 2008-09-16 13:28:23 PDT
Created attachment 338929 [details] [diff] [review]
work in progress

Here's a checkpoint in which layout/style compiles.

I started to look at converting some of the calling code, and concluded my data structure decision might not have been best, and that I probably do want to have a single list, but that I'll need to have separate lengths for each property to handle inheritance correctly.
Comment 38 David Baron :dbaron: ⌚️UTC-10 2008-09-24 20:35:22 PDT
Created attachment 340284 [details] [diff] [review]
work in progress

Here's the alternative approach, which I think is better.
Comment 39 David Baron :dbaron: ⌚️UTC-10 2008-09-25 17:19:34 PDT
Created attachment 340469 [details] [diff] [review]
patch 1: allow arbitrary number of image loaders

This also fixes a nasty bug from border-image, where nsPresContext::DoLoadImage used mImageLoaders.Put() instead of aTable.Put().

It also fixes that we also omitted the traversal code and the animation-mode setting code for mBorderImageLoaders, although the former is irrelevant since image loaders themselves don't implement cycle collection.
Comment 40 David Baron :dbaron: ⌚️UTC-10 2008-09-25 17:20:07 PDT
Created attachment 340470 [details] [diff] [review]
patch 2: implement multiple backgrounds

Still needs tests!
Comment 41 David Baron :dbaron: ⌚️UTC-10 2008-09-25 18:41:22 PDT
Created attachment 340492 [details] [diff] [review]
patch 2: implement multiple backgrounds

This version at least passes our existing mochitests.
Comment 42 David Baron :dbaron: ⌚️UTC-10 2008-09-27 12:51:35 PDT
Comment on attachment 340469 [details] [diff] [review]
patch 1: allow arbitrary number of image loaders

I think this is a useful simplification whether or not multiple backgrounds makes it in, and it also probably fixes some border-image bugs given comment 39.

I was thinking I might want to add some more comments in DidSetStyleContext about how (1) we're intentionally cancelling the old loads after creating the new ones and (2) that doesn't really matter anyway since they're both clones of the real load.

I'd also note this makes it a little bit harder (though not much, I think) to move towards separate animation state per-frame (since the drawing code is now getting the container from the original request proxy load rather than the frame's clone), if we ever wanted to do that (although  I suspect we wouldn't want to try that, given various causes of reframing, etc.).
Comment 43 Boris Zbarsky [:bz] (still a bit busy) 2008-10-08 10:20:49 PDT
Comment on attachment 340469 [details] [diff] [review]
patch 1: allow arbitrary number of image loaders

>+++ b/layout/base/nsImageLoader.cpp

I keep meaning to rename this to nsImageInvalidator or something (and rename LoadImage to TrackImage?).  It doesn't do any actual loading anymore...

I'm assuming you don't really feel like doing that, right?  ;)

>+  nsRefPtr<nsImageLoader> list = mNextLoader;

We should probably use nsRefPtr in some of the places where we use nsCOMPtr<nsImageLoader> right now...

>+++ b/layout/base/nsImageLoader.h
>+  nsImageLoader(nsIFrame *aFrame, PRBool aReflowOnLoad,
>+                nsImageLoader *aNextLoader);

Make that private, since we want all consumers to use Create()?

With that, looks good.  Comments in DidSetStyleContext would be a good idea.
Comment 44 David Baron :dbaron: ⌚️UTC-10 2008-10-08 15:23:13 PDT
(In reply to comment #43)
> (From update of attachment 340469 [details] [diff] [review])
> >+++ b/layout/base/nsImageLoader.cpp
> 
> I keep meaning to rename this to nsImageInvalidator or something (and rename
> LoadImage to TrackImage?).  It doesn't do any actual loading anymore...
> 
> I'm assuming you don't really feel like doing that, right?  ;)

Not today.  Though if I get a chance to do it before I land, do you want me to do that rename?  Invalidator doesn't seem right, since it sometimes does reflows; perhaps nsImageLoadNotifier?

> >+  nsRefPtr<nsImageLoader> list = mNextLoader;
> 
> We should probably use nsRefPtr in some of the places where we use
> nsCOMPtr<nsImageLoader> right now...

Ah, looks like that should be easy to fix if I s/nsInterfaceHashtable/nsRefPtrHashtable/.

> >+++ b/layout/base/nsImageLoader.h
> >+  nsImageLoader(nsIFrame *aFrame, PRBool aReflowOnLoad,
> >+                nsImageLoader *aNextLoader);
> 
> Make that private, since we want all consumers to use Create()?

Good point.  (And the destructor too.)

> With that, looks good.  Comments in DidSetStyleContext would be a good idea.

How about:
  // Ensure that this frame gets invalidates (and, in the case of some
  // 'border-image's, reflows) when images that affect it load.
?
Comment 45 Boris Zbarsky [:bz] (still a bit busy) 2008-10-08 19:21:41 PDT
If you want to do the rename, that's great.  I like nsImageLoadNotifier.

The comment sounds good, and good catch on the destructor.
Comment 46 David Baron :dbaron: ⌚️UTC-10 2008-10-11 13:08:24 PDT
I have the rename patch ready to land as well -- I may not land until Tuesday, though.
Comment 47 David Baron :dbaron: ⌚️UTC-10 2008-10-11 17:52:10 PDT
Landed the arbitrary number of image loaders patch:
http://hg.mozilla.org/mozilla-central/rev/23eebebb8b48
the rename:
http://hg.mozilla.org/mozilla-central/rev/7f708623bf59
and also removed some constants that I found while searching for things to rename:
http://hg.mozilla.org/mozilla-central/rev/c7b659c2efba
Comment 48 David Baron :dbaron: ⌚️UTC-10 2008-10-11 17:53:36 PDT
Created attachment 342748 [details] [diff] [review]
patch 2: implement multiple backgrounds

No substantive changes, just merged to tip (which has involved a good bit of merging).
Comment 49 David Baron :dbaron: ⌚️UTC-10 2008-10-11 19:10:10 PDT
Add to comment 47:
bustage fix: http://hg.mozilla.org/mozilla-central/rev/d7338fec7266
Comment 50 Vladimir Vukicevic [:vlad] [:vladv] 2008-10-20 09:55:30 PDT
Could this have caused bug 460487?
Comment 51 David Baron :dbaron: ⌚️UTC-10 2008-10-27 08:20:51 PDT
Created attachment 344907 [details] [diff] [review]
patch 2: implement multiple backgrounds

I started writing tests on the plane (and fixed some issues they uncovered), but there's still a lot more to be written.
Comment 52 Diego 2008-10-27 12:09:04 PDT
(In reply to comment #47)
> Landed the arbitrary number of image loaders patch:
> http://hg.mozilla.org/mozilla-central/rev/23eebebb8b48
> the rename:
> http://hg.mozilla.org/mozilla-central/rev/7f708623bf59
> and also removed some constants that I found while searching for things to
> rename:
> http://hg.mozilla.org/mozilla-central/rev/c7b659c2efba

Forgive me if this request doesn't make any sense.

Any chance to have a trybuild with this patches backed out to check if they caused the regression bug 460487 "lack of refresh on background image of acid3 reference rendering"?
Comment 53 David Baron :dbaron: ⌚️UTC-10 2008-10-27 12:47:12 PDT
Please discuss that in bug 460487, not here.
Comment 54 David Baron :dbaron: ⌚️UTC-10 2008-10-28 16:52:26 PDT
I backed out the arbitrary number of image loaders stuff; see bug 460796 for why.
Comment 55 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-11-05 11:48:50 PST
I made a couple of reftests for this (when I had the patch in my tree, which I don't have anymore): http://martijn.martijn.googlepages.com/multibackgrounds_reftests.diff
I knew the -moz-transform: translate one was failing, but that might very well not be related to the patch.
Comment 56 Mark 2008-12-09 06:10:32 PST
How can i turn this stuff on in the recently released firefox 3.1 beta 2?
And voted for this bug.
Comment 57 Ryan VanderMeulen [:RyanVM] 2008-12-09 06:31:22 PST
The actual patch to enable it hasn't landed yet, so there is no way to turn it on currently. Given its status as wanted-1.9.1- and blocking-1.9.1- and the fact that Gecko 1.9.1 is past the point of feature freeze, I wouldn't be shocked if this doesn't see the light of day until Firefox 3.2 at this point.

At this point, dbaron and crew are trying to wrap up their 3.1 work and fix up the remaining blocker bugs, so that work will probably take priority over this.
Comment 58 Mark 2008-12-09 10:42:35 PST
(In reply to comment #57)
> The actual patch to enable it hasn't landed yet, so there is no way to turn it
> on currently. Given its status as wanted-1.9.1- and blocking-1.9.1- and the
> fact that Gecko 1.9.1 is past the point of feature freeze, I wouldn't be
> shocked if this doesn't see the light of day until Firefox 3.2 at this point.
> 
> At this point, dbaron and crew are trying to wrap up their 3.1 work and fix up
> the remaining blocker bugs, so that work will probably take priority over this.

So, from the way things look now this awesome feature is gonna be left out of Firefox 3.1 and "perhaps" in Firefox 3.2 (or whatever the next version number is gonna be)

Also something for everyone to note. I am developing quite a lot web stuff and for me personally all the new features (besides border-image) that are coming in Firefox 3.1 and just second rank features.. the ones needed most are the multiple backgrounds (http://www.w3.org/TR/css3-background/) and the advanced layout module (http://www.w3.org/TR/css3-layout/) The rest (that is included now) is nice but not the parts i (/we, the site devs) need most.
Comment 59 Ryan VanderMeulen [:RyanVM] 2008-12-09 10:56:17 PST
Thanks for your comments and for caring, but that's nothing that hasn't already been said in this bug many times over. Please avoid more comments that don't help to fix the bug as it sends a lot of emails to a lot of people and makes it harder to get at the comments where useful information is posted.
https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
Comment 60 David Baron :dbaron: ⌚️UTC-10 2009-01-09 15:56:06 PST
Created attachment 356263 [details] [diff] [review]
patch 1: allow arbitrary number of image loaders

Here's patch 1 merged to mozilla-central, including the landing of bug 456219.
Comment 61 David Baron :dbaron: ⌚️UTC-10 2009-01-09 15:57:31 PST
Created attachment 356265 [details] [diff] [review]
patch 2: implement multiple backgrounds

Here's patch 2 merged to mozilla-central shortly before the landing of bug 456219.

I'm giving up on maintaining it; merging with that landing looks like multiple hours of work that I don't have time to do now.
Comment 62 Michael Ventnor 2009-01-18 18:20:10 PST
Created attachment 357601 [details] [diff] [review]
patch 1: allow arbitrary number of image loaders

Unmodified patch 1 updated to mozilla-central.
Comment 63 Michael Ventnor 2009-01-18 18:21:47 PST
Created attachment 357602 [details] [diff] [review]
Implement multiple backgrounds

Patch 2 updated to mozilla-central. The only bits I modified were in nsCSSRendering, the rest is completely from dbaron.
Comment 64 David Baron :dbaron: ⌚️UTC-10 2009-01-18 19:36:32 PST
Comment on attachment 357602 [details] [diff] [review]
Implement multiple backgrounds

This is not yet ready for review.  It has some pretty substantial bugs in it (e.g., no new call to SetupBackgroundImageLoaders) and it needs more tests.  I also have it mostly updated in my tree, although it's hard to compare your updating to mine given the different diff settings...
Comment 65 Michael Ventnor 2009-01-18 19:43:25 PST
My apologies, I was assigned to unbitrot this but didn't realise it was incomplete.

What kind of diff settings are different?
Comment 66 David Baron :dbaron: ⌚️UTC-10 2009-01-18 19:47:38 PST
(Never mind that the spec has changed substantially and the patch needs to be updated to match.)

Also, I'd already done the patch 1 merging; I keep patches in my tree.
Comment 67 David Baron :dbaron: ⌚️UTC-10 2009-01-22 17:39:15 PST
Created attachment 358312 [details] [diff] [review]
patch 2: implement multiple backgrounds

Here's a merged patch with the issues I know about fixed, although still not updated to the current spec.
Comment 68 David Baron :dbaron: ⌚️UTC-10 2009-01-23 17:15:15 PST
I've updated locally to the new background-color syntax; I still need to update to the new rule for the number of layers (depends only on background-image now, rather than on all properties) and add more tests (including testing inherit combined with the layer number business, since the computed values are as specified).

I'm not going to post patches all that frequently, but my current one is always in http://hg.mozilla.org/users/dbaron_mozilla.com/patches/ .
Comment 69 David Baron :dbaron: ⌚️UTC-10 2009-01-30 12:11:47 PST
Created attachment 359803 [details] [diff] [review]
patch 1: allow arbitrary number of image loaders
Comment 70 David Baron :dbaron: ⌚️UTC-10 2009-01-30 12:12:39 PST
Created attachment 359804 [details] [diff] [review]
patch 2: implement multiple backgrounds
Comment 71 David Baron :dbaron: ⌚️UTC-10 2009-01-30 13:06:30 PST
Comment on attachment 359804 [details] [diff] [review]
patch 2: implement multiple backgrounds

Actually, let's make this a request to bzbarsky to review the layout/style parts of the patch, and to roc to review the rest.

This patch is actually doing three things:
 * making -moz-background-inline-policy no longer be reset by the background shorthand (to match the spec for background-break)
 * implementing multiple background layers
 * implementing fallback background color

I'd initially also added support for the background-origin and background-clip syntax in the background shorthand, but I realized we shouldn't do that until we rename the properties and their values (since the spec uses different value names than we do, and I'd like to do both renames at once), which should wait until CR, and because I'm not sure the spec for that is stable yet.
Comment 72 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-01-31 00:59:45 PST
Wouldn't it be simpler to use nsAutoTArray<1,Layer> for the layers, keeping them in top-to-bottom order?

-  if (!styleBackground->IsTransparent())
-    return styleBackground->mBackgroundColor;
+  if (NS_GET_A(styleBackground->mFallbackBackgroundColor) > 0)
+    return styleBackground->mFallbackBackgroundColor;

Shouldn't we keep using the mBackgroundColor here if it's not transparent?

-      aData->mColorData->mBackColor.SetColorValue(color);
+      aData->mColorData->mBackColor.mXValue.SetColorValue(color);
+      aData->mColorData->mBackColor.mYValue.SetColorValue(color);

Why do we need to set the fallback color here as well?

-        aData->mColorData->mBackColor.SetColorValue(color);
+        aData->mColorData->mBackColor.mXValue.SetColorValue(color);
+        aData->mColorData->mBackColor.mYValue.SetColorValue(color);

Ditto

Your changes to ComputeBackgroundAnchorPoint are OK but we should refactor it so that there is a function that takes a Position, the origin dimension, and the image dimension, and returns the position coordinate, which we just call twice. I guess that can wait until we implement CSS3 background-position.

+  PRBool useFallbackColor = aColor.mBottomLayer->mImage &&
+                            (!drawBackgroundImage ||
+                             !(bottomImage =
+                                 GetImageRequestForBackground(aPresContext,
+                                   aColor.mBottomLayer->mImage, aForFrame)));

I'd prefer some control flow instead of this grotesquely nested expression with side effects. Like
  PRBool useFallbackColor = PR_FALSE;
  imgIRequest *bottomImage = nsnull;
  if (aColor.mBottomLayer->mImage) {
    if (!drawBackgroundImage) {
      bottomImage = GetImageRequestForBackground(aPresContext,
          aColor.mBottomLayer->mImage, aForFrame);
    }
    useFallbackColor = bottomImage != nsnull;
  }

Won't the call to SetupBackgroundClip in the layer loop cause the background-clips to be repeatedly intersected as we go through the loop? We need some save/restore here.

-          bg->mBackgroundClip == NS_STYLE_BG_CLIP_BORDER &&
+          bg->mBottomLayer->mClip == NS_STYLE_BG_CLIP_BORDER &&

Might be helpful to comment here that according to spec, the color is clipped using the clipping option for the bottom layer.

+  if (NS_GET_A(bg->mBackgroundColor) < 255 ||
+      NS_GET_A(bg->mFallbackBackgroundColor) < 255 ||
+      bg->mBottomLayer->mClip != NS_STYLE_BG_CLIP_BORDER)

If mFallbackBackgroundColor is transparent but mBackgroundColor is not, we're still opaque right?
Comment 73 David Baron :dbaron: ⌚️UTC-10 2009-01-31 10:02:29 PST
(In reply to comment #72)
> Wouldn't it be simpler to use nsAutoTArray<1,Layer> for the layers, keeping
> them in top-to-bottom order?

I don't see why it would be simpler.  I'd note that the CSS code nearly always deals with them in top-to-bottom order and the layout code always deals with them in bottom-to-top order (but the bottoms in those two could actually be different, because layout only deals with the layers that have background images).

> Shouldn't we keep using the mBackgroundColor here if it's not transparent?

> Why do we need to set the fallback color here as well?

> Ditto

> If mFallbackBackgroundColor is transparent but mBackgroundColor is not, we're
> still opaque right?

The answer to all four of these is that I'm storing 'background-color: red' by expanding it to 'background-color: red red', since that seems like the simplest way to handle things.  'red red' can be collapsed to 'red' whereas 'red transparent' cannot.

> Won't the call to SetupBackgroundClip in the layer loop cause the
> background-clips to be repeatedly intersected as we go through the loop? We
> need some save/restore here.

SetupBackgroundClip does a restore and a save itself whenever necessary by destroying and recreating aAutoSR.
Comment 74 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-02-02 00:56:43 PST
I guess for some reason it seems simpler to me to use an array rather than a linked list, if we're not inserting into the list, especially when we're already having to deal with counts and indexes of items in the list.

For example,

+  const nsStyleBackground::Layer *l1 = mBottomLayer, *l2 = aOther.mBottomLayer;
+  do {
+    if (*l1 != *l2)
+      return NS_STYLE_HINT_VISUAL;
+    l1 = l1->mHigherLayer;
+    l2 = l2->mHigherLayer;
+  } while (l1 && l2);
+
+  if (!l1 != !l2)
+    return NS_STYLE_HINT_VISUAL; // different number of active layers

could become

  if (mImageCount != aOther.mImageCount)
    return NS_STYLE_HINT_VISUAL;
  for (PRUint32 i = 0; i < mImageCount; ++i) {
    if (mLayers[i] != aOther.mLayers[i])
      return NS_STYLE_HINT_VISUAL;
  }

This code:

+    // Delete any extra items and set mBottomLayer correctly.  We need
+    // to keep layers in which any property was specified, but we must
+    // set mBottomLayer to the last layer in which background-image was
+    // specified (and fill any shorter properties up to that layer).
+    PRUint32 imageCount = bg->mImageCount; // at least 1
+    PRUint32 keepCount = maxItemCount; // at least imageCount
+    nsStyleBackground::Layer *l = &bg->mTopLayer;
+    while (--imageCount != 0) {
+      --keepCount;
+      l = l->mLowerLayer;
+    }
+    bg->mBottomLayer = l;
+
+    while (--keepCount != 0) {
+      l = l->mLowerLayer;
+    }
+    delete l->mLowerLayer;
+    l->mLowerLayer = nsnull;

could become

  bg->mLayers.SetLength(maxItemCount);

In nsComputedDOMStyle::GetBackgroundList the 'layer' temporary is no longer needed.

I suspect that other functions like SetBackgroundList that currently need to wrangle layer counts as well as linked lists would also get simpler.

I could be wrong. Making suggestions like this always sucks because often the only way to prove whether an alternative approach is simpler or not is to actually code it :-(. Sorry.
Comment 75 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-02-02 00:58:16 PST
If an array is used, I suppose it would be helpful to add a helper
  Layer& BottomLayer() { return mLayers[mImageCount - 1]; }
Comment 76 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-02-02 01:05:13 PST
(In reply to comment #73)
> (In reply to comment #72)
> > Shouldn't we keep using the mBackgroundColor here if it's not transparent?
> 
> > Why do we need to set the fallback color here as well?
> 
> > Ditto
> 
> > If mFallbackBackgroundColor is transparent but mBackgroundColor is not, we're
> > still opaque right?
> 
> The answer to all four of these is that I'm storing 'background-color: red' by
> expanding it to 'background-color: red red', since that seems like the simplest
> way to handle things.  'red red' can be collapsed to 'red' whereas 'red
> transparent' cannot.

OK, that makes sense. I also missed that according to spec if the first image fails to load then the fallback color is used *instead of* the first color.

> > Won't the call to SetupBackgroundClip in the layer loop cause the
> > background-clips to be repeatedly intersected as we go through the loop? We
> > need some save/restore here.
> 
> SetupBackgroundClip does a restore and a save itself whenever necessary by
> destroying and recreating aAutoSR.

Yikes, that seems quite ugly to me. I think we should just not bother trying to use an auto object if we're going to violate scoped save/restore like that. Let's just use explicit Save/Restore instead.
Comment 77 David Baron :dbaron: ⌚️UTC-10 2009-02-04 11:44:50 PST
(In reply to comment #76)
> (In reply to comment #73)
> > (In reply to comment #72)
> > > Won't the call to SetupBackgroundClip in the layer loop cause the
> > > background-clips to be repeatedly intersected as we go through the loop? We
> > > need some save/restore here.
> > 
> > SetupBackgroundClip does a restore and a save itself whenever necessary by
> > destroying and recreating aAutoSR.
> 
> Yikes, that seems quite ugly to me. I think we should just not bother trying to
> use an auto object if we're going to violate scoped save/restore like that.
> Let's just use explicit Save/Restore instead.

Would it be ok if I add a method to the autoSaveRestore class instead?  I'd really prefer not to manage the whole thing manually, since then there's a real risk of not matching save/restore pairs.  I'd also prefer not to double the number of save/restore calls in the normal (non-layers) case.

(In reply to comment #74)
> I guess for some reason it seems simpler to me to use an array rather than a
> linked list, if we're not inserting into the list, especially when we're
> already having to deal with counts and indexes of items in the list.

So I have three concerns:

The first is that I worry somebody will come along and decide that nsAutoTArray should be NS_STACK_CLASS, and "fix" all the occurrences that were on the heap, whether intentionally or not (just as was done with nsAutoString).

The second is that I'm a bit uncomfortable making all the callers in layout know that they have to ignore the count on the array and instead always iterate from mImageCount-1 down to 0.

Third, it's a good bit of work to convert and I'm not sure it's worth the time.

But I'll give it a shot anyway sometime in the next few days.
Comment 78 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-02-04 15:24:02 PST
(In reply to comment #77)
> Would it be ok if I add a method to the autoSaveRestore class instead?  I'd
> really prefer not to manage the whole thing manually, since then there's a
> real risk of not matching save/restore pairs.  I'd also prefer not to double
> the number of save/restore calls in the normal (non-layers) case.

Sure, a new method would be fine.

> (In reply to comment #74)
> > I guess for some reason it seems simpler to me to use an array rather than a
> > linked list, if we're not inserting into the list, especially when we're
> > already having to deal with counts and indexes of items in the list.
> 
> So I have three concerns:
> 
> The first is that I worry somebody will come along and decide that nsAutoTArray
> should be NS_STACK_CLASS, and "fix" all the occurrences that were on the heap,
> whether intentionally or not (just as was done with nsAutoString).

They'd better not, since there are other places in the code we use nsAutoTArray<T,1> in objects. We can add a comment to that effect in nsTArray.h.

> The second is that I'm a bit uncomfortable making all the callers in layout
> know that they have to ignore the count on the array and instead always
> iterate from mImageCount-1 down to 0.

That's a good thought, but I think if there's a good comment and some appropriate tests, it shouldn't be a problem.

> Third, it's a good bit of work to convert and I'm not sure it's worth the
> time.
> 
> But I'll give it a shot anyway sometime in the next few days.

Thanks.
Comment 79 David Baron :dbaron: ⌚️UTC-10 2009-02-10 21:13:51 PST
Created attachment 361708 [details] [diff] [review]
patch 2: implement multiple backgrounds

Updated to roc's comments.
Comment 80 Boris Zbarsky [:bz] (still a bit busy) 2009-02-12 18:03:59 PST
Comment on attachment 359803 [details] [diff] [review]
patch 1: allow arbitrary number of image loaders

Hmm.  So this reclones the imgIRequest on every paint...  I guess that's ok.
Comment 81 Boris Zbarsky [:bz] (still a bit busy) 2009-02-12 20:55:51 PST
Comment on attachment 361708 [details] [diff] [review]
patch 2: implement multiple backgrounds

>+++ b/content/html/content/src/nsGenericHTMLElement.cpp
>+  if (!aData->mColorData->mBackImage && presContext->UseDocumentColors()) {
...
>         // in NavQuirks mode, allow the empty string to set the
>         // background to empty
>-        aData->mColorData->mBackImage.SetNoneValue();
>+        aData->mColorData->mBackImage = nsnull;

That doesn't look right, since mBackImage is already null...  Shouldn't you be setting mTempBackImage to a "none" and pointing mBackImage to it?

Sadly it's really hard to write a testcase for this, since it only affects UA/user sheets that set backgrounds.  :(

>+++ b/layout/base/nsCSSRendering.cpp
>+static imgIRequest*
>+GetImageRequestForBackground(nsPresContext *aPresContext,
>+                             imgIRequest *aRequest, nsIFrame *aForFrame)

This function doesnt't use either aPresContext or aForFrame.  Why pass them in?

Is there even a good reason to have the function return imgIRequest instead of a PRBool that says whether to use the aRequest?  I guess it makes the diff smaller in some places where you assign the result to |req|.

>+  if (!(status & imgIRequest::STATUS_FRAME_COMPLETE) ||
>+      !(status & imgIRequest::STATUS_SIZE_AVAILABLE)) {

I believe FRAME_COMPLETE implies SIZE_AVAILABLE, though you might want to double-check that with Joe or Stuart.  I guess you just moved the existing code, though.  OK.

>+static void
>+SetupDirtyRects(const nsRect& aBGClipArea, const nsRect& aCallerDirtyRect,
>+                nscoord aAppUnitsPerPixel,
>+                /* OUT: */
>+                nsRect* aDirtyRect, gfxRect* aDirtyRectGfx)

Would it make sense to inline this?

>@@ -1381,30 +1482,34 @@ nsCSSRendering::PaintBackgroundWithSC(ns
>+    useFallbackColor = bottomImage != nsnull;

I'm not sure I follow this.  As far as I can tell from the spec, the fallback color should be used if the bottom-most background image cannot be drawn, including if it was specified as "none", right?

If it's specified as "none", then aColor.BottomLayer().mImage (and hence bottomImage) will be null, if I understand the rulenode code correctly.  If the bottommost image doesn't have NS_FRAME_COMPLETE, bottomImage will also be null.

So it seems like this should be testing |bottomImage == nsnull|.  If not, what am I missing?  If this test is wrong right now, add a testcase please?

I didn't really read the reftests in this patch.

>+++ b/layout/style/nsCSSDataBlock.cpp
>@@ -277,25 +250,60 @@ nsCSSCompressedDataBlock::MapRuleInfoInt
>                     NS_ASSERTION(val->mXValue.GetUnit() != eCSSUnit_Null ||
>                                  val->mYValue.GetUnit() != eCSSUnit_Null, "oops");

Given how many places we assert that, would it make sense to have a debug-only IsConsistent() method or something on nsCSSValuePair?

>+++ b/layout/style/nsCSSDeclaration.cpp
>-      const nsCSSValue *inlinePolicyValue = static_cast<const nsCSSValue*>(
>-        data->StorageFor(eCSSProperty__moz_background_inline_policy));

Is there a reason you're not longer checking that inline policy is continuous when serializing this shorthand?  Is it just that we don't allow this shorthand to set that value?

>+++ b/layout/style/nsCSSParser.cpp
>+CSSParserImpl::ParseBackgroundItem(CSSParserImpl::BackgroundItem& aItem,
>+  aItem.mRepeat.SetIntValue(NS_STYLE_BG_REPEAT_XY,
>                                            eCSSUnit_Enumerated);

Fix the indent here?

>+  aItem.mAttachment.SetIntValue(NS_STYLE_BG_ATTACHMENT_SCROLL,
>                                                eCSSUnit_Enumerated);

And here.

>+        mTempData.mColor.mBackColor.mXValue = val;
>+        mTempData.mColor.mBackColor.mYValue = val;
...
>+        aItem.mPosition.mXValue = val;
>+        aItem.mPosition.mYValue = val;

Could use SetBothValuesTo here, but either way.


>+      // Note: ParseBackgroundColor parses 'inherit' and 'initial', but
>+      // we've already checked for them, so it's ok.
>+      if (!ParseBackgroundColor(PR_TRUE)) {
>+        return PR_FALSE;
>+      }

Don't you need to set aItem.mLastItem to true here?  Test for this?

>+CSSParserImpl::ParseBackgroundColor(PRBool aInShorthand)
>+    case eCSSUnit_Initial:
>+      return PR_TRUE; // we're done

Not ExpectEndProperty()?  If not, why do we need to check it for the final return of this function?

>+++ b/layout/style/nsRuleNode.cpp
>+SetBackgroundList(nsStyleContext* aStyleContext,
>+        ++aItemCount;
>+        if (!aLayers.EnsureLengthAtLeast(aItemCount)) {
>+          NS_WARNING("out of memory");
>+          break;

Should this decrement aItemCount before breaking?

> nsRuleNode::ComputeBackgroundData(void* aStartStruct,

>+    PRUint32 fillCount = bg->mImageCount;

The spec seems to say that the repeating thing happens out to max of number of images, repeats, sizes, positions.  We're only doing it out to number of images.  Why is that?
Comment 82 David Baron :dbaron: ⌚️UTC-10 2009-02-12 22:02:46 PST
(In reply to comment #81)
> >+static imgIRequest*
> >+GetImageRequestForBackground(nsPresContext *aPresContext,
> >+                             imgIRequest *aRequest, nsIFrame *aForFrame)
> 
> This function doesnt't use either aPresContext or aForFrame.  Why pass them in?

Ah.  It used to, before I completely rewrote the way image loaders worked.  Will fix.

> Is there even a good reason to have the function return imgIRequest instead of
> a PRBool that says whether to use the aRequest?  I guess it makes the diff
> smaller in some places where you assign the result to |req|.

Seems simpler that way.

> >@@ -1381,30 +1482,34 @@ nsCSSRendering::PaintBackgroundWithSC(ns
> >+    useFallbackColor = bottomImage != nsnull;
> 
> I'm not sure I follow this.  As far as I can tell from the spec, the fallback
> color should be used if the bottom-most background image cannot be drawn,
> including if it was specified as "none", right?

The spec has been clarified in the editor's draft, see:
http://dev.w3.org/csswg/css3-background/#the-background-color-property
so I only want to use the fallback color if there is an image but it couldn't load or be displayed.

> >+++ b/layout/style/nsCSSDeclaration.cpp
> >-      const nsCSSValue *inlinePolicyValue = static_cast<const nsCSSValue*>(
> >-        data->StorageFor(eCSSProperty__moz_background_inline_policy));
> 
> Is there a reason you're not longer checking that inline policy is continuous
> when serializing this shorthand?  Is it just that we don't allow this shorthand
> to set that value?

Per the spec, background-break (which is what -moz-background-inline-policy is) is not set by the shorthand, so this patch changes it so it is no longer set by the shorthand.

> >+      // Note: ParseBackgroundColor parses 'inherit' and 'initial', but
> >+      // we've already checked for them, so it's ok.
> >+      if (!ParseBackgroundColor(PR_TRUE)) {
> >+        return PR_FALSE;
> >+      }
> 
> Don't you need to set aItem.mLastItem to true here?  Test for this?
> 
> >+CSSParserImpl::ParseBackgroundColor(PRBool aInShorthand)
> >+    case eCSSUnit_Initial:
> >+      return PR_TRUE; // we're done
> 
> Not ExpectEndProperty()?  If not, why do we need to check it for the final
> return of this function?

ExpectEndProperty() is not a required check these days now that we have mTempData.  The end of the function tests for it with ||, not &&, although I suspect the end could just return true.  Then again, I should probably be consistent and check ExpectEndProperty in the initial case, just to be consistent with the rest of the code.

> >+    PRUint32 fillCount = bg->mImageCount;
> 
> The spec seems to say that the repeating thing happens out to max of number of
> images, repeats, sizes, positions.  We're only doing it out to number of
> images.  Why is that?

This is the big change in the editor's draft for which I had to update the patch.
Comment 83 Boris Zbarsky [:bz] (still a bit busy) 2009-02-12 22:39:33 PST
> so I only want to use the fallback color if there is an image but it couldn't
> load or be displayed.

Right.  But bottomImage != nsnull means that there is an image and it's got STATUS_FRAME_COMPLETE set, no?  It seems to me that if you're trying to test for the case when we have an image in the computed style but not an image to paint the right test would be:

  useFallbackColor = !bottomImage && aColor.BottomLayer().mImage;

Thanks for the editor's draft pointer; that makes the patch make a lot more sense.  ;)
Comment 84 John A. Bilicki III 2009-02-12 22:48:23 PST
You'll need to render the background-color automatically unless you detect the background-image's mime since GIF and PNG support transparency. Since they do even if there are a hundred background-images it's still possible that they are all transparent. Since this feature is liquid friendly and presuming SVG images aren't supported (or at least not yet) at higher resolutions (such as my screen's native 1920x1200) it is entirely likely that parts of the background where there are no background-images for the element would still be visible. So I can't see *any* scenario where you would want to not render the element's background-color automatically...unless of course I'm missing something like background-image-width/height support (if any exists/supported) though even then if the images are transparent even if the size is set to 100%/100% you'll need to render the background-color regardless.
Comment 85 David Baron :dbaron: ⌚️UTC-10 2009-02-12 22:50:02 PST
(In reply to comment #83)
>   useFallbackColor = !bottomImage && aColor.BottomLayer().mImage;

Er, that's almost what it is (since the assignment is inside an if (aColor.BottomLayer.mImage), except that I got it backwards when I was addressing roc's comment in comment 72 and wrote != nsnull instead of == nsnull.
Comment 86 Boris Zbarsky [:bz] (still a bit busy) 2009-02-12 22:59:48 PST
> wrote != nsnull instead of == nsnull.

Right.  That makes a lot more sense to me.  That's what I suggested in comment 81 too.  ;)
Comment 87 David Baron :dbaron: ⌚️UTC-10 2009-02-16 19:42:27 PST
(In reply to comment #81)
> >+++ b/layout/style/nsCSSDataBlock.cpp
> >@@ -277,25 +250,60 @@ nsCSSCompressedDataBlock::MapRuleInfoInt
> >                     NS_ASSERTION(val->mXValue.GetUnit() != eCSSUnit_Null ||
> >                                  val->mYValue.GetUnit() != eCSSUnit_Null, "oops");
> 
> Given how many places we assert that, would it make sense to have a debug-only
> IsConsistent() method or something on nsCSSValuePair?

I'd rather not mess with that now.  I think a bunch of these assertions assert different things for good reasons; I think the rules for whether value pairs are separable vary by property.
Comment 88 David Baron :dbaron: ⌚️UTC-10 2009-02-16 19:49:20 PST
(In reply to comment #81)
> >+      // Note: ParseBackgroundColor parses 'inherit' and 'initial', but
> >+      // we've already checked for them, so it's ok.
> >+      if (!ParseBackgroundColor(PR_TRUE)) {
> >+        return PR_FALSE;
> >+      }
> 
> Don't you need to set aItem.mLastItem to true here?  Test for this?

Why would I?  If this returns false, the parse fails and the item is discarded.
Comment 89 Boris Zbarsky [:bz] (still a bit busy) 2009-02-16 20:31:01 PST
> Why would I?  If this returns false, the parse fails and the item is discarded.

Yes, but I meant after that block, just like in the other place (the eCSSToken_ident case) where you do ParseBackgroundColor.  Sorry about not making that clearer.

In other words, I think that the patch as-is won't bail out on:

  background: rgb(0, 0, 0), url(foo);
Comment 90 David Baron :dbaron: ⌚️UTC-10 2009-02-16 20:51:41 PST
(In reply to comment #89)
> Yes, but I meant after that block, just like in the other place (the
> eCSSToken_ident case) where you do ParseBackgroundColor.  Sorry about not
> making that clearer.

See, when I saw the quoted text, I lined it up with the first occurrence, and didn't realize there were two.

Fixed now, with tests (and in my patch queue).


The remaining issue I need to fix is that the tests I just wrote for fallback colors (which I should have written earlier) showed that we're not using the fallback color for url(404.png).
Comment 91 David Baron :dbaron: ⌚️UTC-10 2009-02-16 22:15:10 PST
(In reply to comment #90)
> The remaining issue I need to fix is that the tests I just wrote for fallback
> colors (which I should have written earlier) showed that we're not using the
> fallback color for url(404.png).

We presumably have the same issue for blocked images.  nsCSSValue::Image::mImage may be null; we need to distinguish the case where it's null (use fallback) from the case where there was a eCSSUnit_None value.  We currently don't store that difference in the computed data (i.e., nsStyleBackground::Layer).

I need to figure out whether it's better to fix this by:

   (1) replacing the nsCOMPtr<imgIRequest> mImage in nsStyleBackground::Layer by a struct containing that nsCOMPtr and a boolean, or

   (2) creating an instance of imgIRequest (not sure whether it would be an imgRequest or another implementation) that has an error status and other data as appropriate.

I'd also note that if we did (2) we could (but don't have to) change whether the url shows up in getComputedStyle() or not; if we did (1) we're still stuck with our current behavior of saying the computed style is 'none'.

bzbarsky, thoughts?
Comment 92 Boris Zbarsky [:bz] (still a bit busy) 2009-02-17 07:46:48 PST
Creating an imgRequest with the behavior you want would take some work, I suspect.  Your own imgIRequest impl is probably ok as long as you don't hand it around to people too much...  I would be rather wary of it, because the imgIRequest interface does NOT define the behavior consumers actually expect from it in all cases (mostly because it doesn't define much).

So I suspect the safe fix is the struct.  If we wanted to, we could use an nsCOMPtr<nsIURI> in lieu of a boolean, no?  If mImage is null but the uri is not, then we had an error.  Or something.

As for 404, I don't think imagelib exposes that information at the moment at all...  As in, if your 404 is an image, it'll just load the image.  Image blocking is quite different, since it prevents the image load altogether, as you note.
Comment 93 Boris Zbarsky [:bz] (still a bit busy) 2009-02-17 07:48:00 PST
See also bug 299138 as far as 404 goes.
Comment 94 David Baron :dbaron: ⌚️UTC-10 2009-02-17 13:12:32 PST
Created attachment 362765 [details] [diff] [review]
patch 2: implement multiple backgrounds

This should address bzbarsky's comments.

This uses the struct approach mentioned above.  While doing that, I also took the suggestion of changing GetImageRequestForBackground to UseImageRequestForBackground (as suggested above) since the advantage of the former went away.
Comment 95 Boris Zbarsky [:bz] (still a bit busy) 2009-02-18 10:58:47 PST
Comment on attachment 362765 [details] [diff] [review]
patch 2: implement multiple backgrounds

>+++ b/layout/base/nsCSSRendering.cpp
>+UseImageRequestForBackground(imgIRequest *aRequest)
>+  if (!(status & imgIRequest::STATUS_FRAME_COMPLETE) ||
>+      !(status & imgIRequest::STATUS_SIZE_AVAILABLE)) {
>+    return PR_FALSE;
>+  }
>+
>+  return PR_TRUE;

  return (status & imgIRequest::STATUS_FRAME_COMPLETE) &&
         (status & imgIRequest::STATUS_SIZE_AVAILABLE);

>+PaintBackgroundLayer(nsPresContext* aPresContext,
>+  imgIRequest *req = aLayer.mImage.mRequest;
>+  if (!UseImageRequestForBackground(aLayer.mImage.mRequest)) {

Pass |req| there, since you've got it already?

r+sr=bzbarsky.
Comment 96 David Baron :dbaron: ⌚️UTC-10 2009-02-19 13:44:14 PST
Dolske has hit this a few times:

###!!! ABORT: should have returned early after aDirtyRectGfx test: '!bgAreaGfx.IsEmpty()', file /Users/dolske/build/mozilla-central/layout/base/nsCSSRendering.cpp, line 1431

... I need to look into it.
Comment 97 David Baron :dbaron: ⌚️UTC-10 2009-02-19 13:57:05 PST
The main patch here was also responsible for the talos startup test failing on Mac only.
Comment 98 David Baron :dbaron: ⌚️UTC-10 2009-02-19 14:12:34 PST
(In reply to comment #96)
> Dolske has hit this a few times:
> 
> ###!!! ABORT: should have returned early after aDirtyRectGfx test:
> '!bgAreaGfx.IsEmpty()', file
> /Users/dolske/build/mozilla-central/layout/base/nsCSSRendering.cpp, line 1431
> 
> ... I need to look into it.

So I think this became possible to hit when I merged the patch with http://hg.mozilla.org/mozilla-central/rev/50e934e4979b .  I guess I should go back to having:

-    if (bgAreaGfx.IsEmpty()) {
-      NS_WARNING("converted background area should not be empty");
-      return;
-    }

which is what I removed, but I think that's now possible to hit because of http://hg.mozilla.org/mozilla-central/rev/50e934e4979b .
Comment 99 David Baron :dbaron: ⌚️UTC-10 2009-02-19 14:47:48 PST
Created attachment 363198 [details]
tinderbox failures on mac unit test
Comment 100 David Baron :dbaron: ⌚️UTC-10 2009-02-19 15:31:31 PST
The image loader patch is now in:
http://hg.mozilla.org/mozilla-central/rev/8bf2088734ac
but the main patch is backed out due to:
 * Mac OS X talos startup test failures
 * Mac OS X unit test failures (above)
 * comment 96 (also Mac OS X)
Comment 101 David Baron :dbaron: ⌚️UTC-10 2009-02-19 17:21:29 PST
(In reply to comment #99)
> Created an attachment (id=363198) [details]
> tinderbox failures on mac unit test

I don't see any of these failures on my Mac except for the one described in http://lists.w3.org/Archives/Public/www-archive/2009Feb/0083.html which I've fixed locally.  (We only run that reftest on Mac, and I guess I hadn't run reftests on Mac since updating to the syntax change to remove the '/'.)
Comment 102 David Baron :dbaron: ⌚️UTC-10 2009-02-19 18:24:31 PST
(In reply to comment #100)
> The image loader patch is now in:
> http://hg.mozilla.org/mozilla-central/rev/8bf2088734ac
> but the main patch is backed out due to:
>  * Mac OS X talos startup test failures

This might be the same as comment 96 (the third problem), but I can't test anymore thanks to bug 479333.

>  * Mac OS X unit test failures (above)

One was http://lists.w3.org/Archives/Public/www-archive/2009Feb/0083.html , which I've fixed locally.

The rest seem to be that on the unit test Mac boxes, background-position seems to do nothing; it always draws like it's at 0,0 (although the style system tests work).

>  * comment 96 (also Mac OS X)

I think I have this fixed locally, as described in comment 98.
Comment 103 David Baron :dbaron: ⌚️UTC-10 2009-02-19 21:45:10 PST
Relanded as http://hg.mozilla.org/mozilla-central/rev/55a739725414 , with I think a total of 4 changes from the reviewed patch:

  1. removing the MOZ_COUNT_CTOR and MOZ_COUNT_DTOR for nsStyleBackground::Layer, which (a) shouldn't be needed since they're part of an nsTArray now instead of a linked list and (b) cause problems because there's no explicit copy-constructor, so they lead to imbalance.

  2. fixing the merging as described in comment 98, given what dolske was seeing.  I need to file a followup bug on that.

  3. fixing our internal copy of the Acid2 test to change the test that checks that incorrect CSS rules are dropped, since we're now implementing css3-background support that makes one of those previously-invalid rules valid.

  4. A fix to return PR_TRUE at the end of EnsureLengthAtLeast (the cause of the Mac reftest failures and talos crashes) rather than falling off the end of the function.


Optimistically marking as fixed.
Comment 104 David Baron :dbaron: ⌚️UTC-10 2009-02-19 23:08:13 PST
(In reply to comment #103)
>   2. fixing the merging as described in comment 98, given what dolske was
> seeing.  I need to file a followup bug on that.

I filed this as bug 479387.
Comment 105 David Baron :dbaron: ⌚️UTC-10 2009-02-20 11:51:08 PST
Made a fifth change now (separately):
http://hg.mozilla.org/mozilla-central/rev/96e034632074
which I think is correct, although I don't see how it could fix the random test failure on Mac.
Comment 106 David Baron :dbaron: ⌚️UTC-10 2009-02-21 12:09:49 PST
I think the intermittent reftest failures were fixed by
http://hg.mozilla.org/mozilla-central/rev/9d5337daba7b
because I'm guessing that reftests were sharing the profile between different branches, and our caching heuristics allowed us to reused the cached copy from the run on the tracemonkey (or maybe 1.9.1, though I suspect the gCount would be different) builds.

So I backed out the temporary modifications to 289480.html that I made: http://hg.mozilla.org/mozilla-central/log/ccb7bc7113a4/layout/reftests/bugs/289480.html
Comment 107 Ildar Nurislamov 2009-03-04 04:08:30 PST
Not fixed completely. In this case:
 http://www.css3.info/wp-content/uploads/2007/09/multiple-backgrounds-example.html
banner_fresco.jpg background not shown (http://www.css3.info/images/banner_fresco.jpg)

background: url(/images/body-top.gif) top left no-repeat,
				url(/images/banner_fresco.jpg) 11px 11px no-repeat,
				url(/images/body-bottom.gif) bottom left no-repeat,
				url(/images/body-middle.gif) left repeat-y;
Comment 108 David Baron :dbaron: ⌚️UTC-10 2009-03-04 04:17:58 PST
That works for me, I think, but if you're testing using a 3.2a1pre build, please file a separate bug that contains the above comment plus:
 * what build you're using
 * what you see
 * what you expect to see instead
Comment 109 Ildar Nurislamov 2009-03-04 04:28:20 PST
ok. i have opened new bug: https://bugzilla.mozilla.org/show_bug.cgi?id=481390
Comment 110 Philip Chee 2009-03-20 12:19:43 PDT
Possible regression. Please see  Bug 422851,  Bug 482941.
Comment 111 Robert Longson 2009-05-22 08:52:24 PDT
*** Bug 494404 has been marked as a duplicate of this bug. ***
Comment 112 Edward Webb 2009-06-23 08:05:19 PDT
Multiple backgrounds still broken on Firefox 3.5rc2 (Gecko/20090616), based on below. Is this going to miss 3.5?

http://www.css3.info/wp-content/uploads/2007/09/multiple-backgrounds-example.html
http://www.quirksmode.org/css/multiple_backgrounds.html
Comment 113 u88484 2009-06-23 08:17:27 PDT
(In reply to comment #112)
> Multiple backgrounds still broken on Firefox 3.5rc2 (Gecko/20090616), based on
> below. Is this going to miss 3.5?

Yes, the patch was checked in on the trunk and will not make the Firefox 3.5 release.  The Firefox.next will included support of multiple backgrounds.
Comment 114 Boris Zbarsky [:bz] (still a bit busy) 2009-06-23 09:36:27 PDT
Edward, as as you might have noticed the spec for multiple backgrounds is still heavily in flux.  Given that, there was no intention of trying to ship this in Firefox 3.5.
Comment 115 Edward Webb 2009-06-23 11:19:20 PDT
Thanks for the info Kurt, Boris. Did notice that the milestone's aimed for 1.9.2, however believed it may have already been fixed. More important to get it right first time than having a rushed addition.

Looking forward to the implementation in Firefox.next, as this (combined with media queries) will allow downloading additional backgrounds only if they're visible (e.g. for larger screens), without requiring extra markup.
Comment 116 Eric Shepherd [:sheppy] 2009-10-19 07:09:33 PDT
I've updated the documentation here:

https://developer.mozilla.org/en/CSS/background
https://developer.mozilla.org/en/CSS/background-image

Do we support the new multiple values syntax for background-repeat, background-attachment, background-position, background-origin, and background-size? A quick look through the patch didn't make this clear to me.
Comment 117 David Baron :dbaron: ⌚️UTC-10 2009-10-19 07:22:43 PDT
(In reply to comment #116)
> Do we support the new multiple values syntax for background-repeat,
> background-attachment, background-position, background-origin, and
> background-size? A quick look through the patch didn't make this clear to me.

Yes.  (The changes to layout/style/test/property_database.js should probably be the most readable in the patch.)
Comment 119 Jeff Walden [:Waldo] (remove +bmo to email) 2009-10-19 13:18:41 PDT
Documentation of multiple backgrounds, outside of the syntax, appears latent in those articles, not specifically called out.  Is there a single article that describes how multiple backgrounds work, how not-fully-specified lists for single backgrounds are repeated (?, might have been looped, memory hazy), and provides a few examples?  I went to look at the background-size changes and see only that it's described in the pseudo-grammar in the syntax section, without mention of the effect on rendering.
Comment 120 David Baron :dbaron: ⌚️UTC-10 2009-10-19 13:51:31 PDT
I would suggest strongly discouraging use of multiple backgrounds through the longhand properties and documenting them primarily on the page for the 'background' shorthand.  In other words, I think terse syntax updates are fine for everything other than the 'background' shorthand property's page.
Comment 121 Jeff Walden [:Waldo] (remove +bmo to email) 2009-10-19 17:02:04 PDT
Makes sense.  The minimal change I added to the background-size page to at least mention the meaning of the multiple-background syntax is designed to link off to some other page that addresses multiple backgrounds directly.  -moz-background-size: 50% 50%, auto, 50% 50% doesn't have much (really any) meaning in isolation from values of the other properties.

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