Closed Bug 322475 Opened 19 years ago Closed 16 years ago

Implement support for CSS3 multiple backgrounds

Categories

(Core :: CSS Parsing and Computation, enhancement, P4)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.2a1

People

(Reporter: moon, Assigned: dbaron)

References

(Depends on 1 open bug, Blocks 1 open bug,
URL
)

Details

(Keywords: css3, dev-doc-complete)

Attachments

(4 files, 15 obsolete files)

pre-patch: merge nsCSSQuotes and nsCSSCounterData into nsCSSValuePairList
42.09 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
patch 1: allow arbitrary number of image loaders
33.04 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
patch 2: implement multiple backgrounds
179.99 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
tinderbox failures on mac unit test
719.13 KB, text/plain
Details 
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. ;)
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...
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
The z-order is the order given (I forget whether it's top->bottom or bottom->top).
(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."
Assignee: dbaron → nobody
QA Contact: ian → style-system
More information here, where an example is also available:

http://www.css3.info/preview/multiple-backgrounds/
Blocks: 387345
I get a 404 for the URL...
I tried it and it is working.
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 :)
No longer blocks: 387345
(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.
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/ .
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?
Flags: wanted1.9.1?
@ 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.
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.
OK, tell me what you want to use multiple backgrounds for.
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.
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.
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.
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.
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.
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.)
(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.
@ 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.
Assignee: nobody → dbaron
Mochitests in layout/style pass, although I should probably write a test for the nsHTMLCSSStyleSheet code...
Attachment #332034 - Flags: superreview?(bzbarsky)
Attachment #332034 - Flags: review?(bzbarsky)
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
Attachment #332034 - Flags: superreview?(bzbarsky)
Attachment #332034 - Flags: superreview+
Attachment #332034 - Flags: review?(bzbarsky)
Attachment #332034 - Flags: review+
I reworded it so it doesn't repeat the number twice, and landed: http://hg.mozilla.org/mozilla-central/index.cgi/rev/6ad2e5aa3c64
Flags: wanted1.9.1?
Flags: wanted1.9.1-
Flags: blocking1.9.1-
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.
Or I suppose Firefox could simply switch to using the Webkit rendering engine.
:p
@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. :-)
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.
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.
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]
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.
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
All these comments are not helping.  They just make the bug report harder to read.
Attached patch work in progress (obsolete) — — Splinter Review 
This has most of the style system changes (no nsComputedDOMStyle changes).  No tests yet, and no painting changes or other layout changes yet.
Attached patch work in progress (obsolete) — — Splinter Review 
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.
Attachment #338576 - Attachment is obsolete: true
Attached patch work in progress (obsolete) — — Splinter Review 
Here's the alternative approach, which I think is better.
Attachment #338929 - Attachment is obsolete: true
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.
Still needs tests!
Attachment #340284 - Attachment is obsolete: true
This version at least passes our existing mochitests.
Attachment #340470 - Attachment is obsolete: true
Attachment #340469 - Flags: superreview?(bzbarsky)
Attachment #340469 - Flags: review?(bzbarsky)
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 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.
Attachment #340469 - Flags: superreview?(bzbarsky)
Attachment #340469 - Flags: superreview+
Attachment #340469 - Flags: review?(bzbarsky)
Attachment #340469 - Flags: review+
(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.
?
If you want to do the rename, that's great.  I like nsImageLoadNotifier.

The comment sounds good, and good catch on the destructor.
I have the rename patch ready to land as well -- I may not land until Tuesday, though.
Blocks: 446328
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
No substantive changes, just merged to tip (which has involved a good bit of merging).
Attachment #340492 - Attachment is obsolete: true
Depends on: 459649
I started writing tests on the plane (and fixed some issues they uncovered), but there's still a lot more to be written.
Attachment #342748 - Attachment is obsolete: true
(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"?
Please discuss that in bug 460487, not here.
I backed out the arbitrary number of image loaders stuff; see bug 460796 for why.
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.
How can i turn this stuff on in the recently released firefox 3.1 beta 2?
And voted for this bug.
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.
(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.
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
Here's patch 1 merged to mozilla-central, including the landing of bug 456219.
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.
Attachment #340469 - Attachment is obsolete: true
Attachment #344907 - Attachment is obsolete: true
Attachment #356263 - Flags: superreview?(bzbarsky)
Attachment #356263 - Flags: review?(bzbarsky)
Unmodified patch 1 updated to mozilla-central.
Attachment #356263 - Attachment is obsolete: true
Attachment #357601 - Flags: superreview?(bzbarsky)
Attachment #357601 - Flags: review?(bzbarsky)
Attachment #356263 - Flags: superreview?(bzbarsky)
Attachment #356263 - Flags: review?(bzbarsky)
Attached patch Implement multiple backgrounds (obsolete) — — Splinter Review 
Patch 2 updated to mozilla-central. The only bits I modified were in nsCSSRendering, the rest is completely from dbaron.
Attachment #356265 - Attachment is obsolete: true
Attachment #357602 - Flags: superreview?(roc)
Attachment #357602 - Flags: review?(roc)
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...
Attachment #357602 - Flags: superreview?(roc)
Attachment #357602 - Flags: superreview-
Attachment #357602 - Flags: review?(roc)
Attachment #357602 - Flags: review-
My apologies, I was assigned to unbitrot this but didn't realise it was incomplete.

What kind of diff settings are different?
(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.
Attachment #357601 - Attachment description: Arbitrary number of image loaders → patch 1: allow arbitrary number of image loaders
Here's a merged patch with the issues I know about fixed, although still not updated to the current spec.
Attachment #357602 - Attachment is obsolete: true
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/ .

        Attachment #357601 -
        Attachment is obsolete: true

        Attachment #359803 -
        Flags: superreview?(bzbarsky)

        Attachment #359803 -
        Flags: review?(bzbarsky)

        Attachment #357601 -
        Flags: superreview?(bzbarsky)

        Attachment #357601 -
        Flags: review?(bzbarsky)

    
    

    
  


  

        Attachment #358312 -
        Attachment is obsolete: true

        Attachment #359804 -
        Flags: superreview?(bzbarsky)

        Attachment #359804 -
        Flags: review?(bzbarsky)

    
    

    
  
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.

    
    

    
  
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?

    
    

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

    
    

    
  
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.

    
    

    
  
If an array is used, I suppose it would be helpful to add a helper
  Layer& BottomLayer() { return mLayers[mImageCount - 1]; }

    
    

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

    
    

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

    
    

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

    
    

    
  


  
Updated to roc's comments.

        Attachment #359804 -
        Attachment is obsolete: true

        Attachment #361708 -
        Flags: superreview?(bzbarsky)

        Attachment #361708 -
        Flags: review?(roc)

        Attachment #359804 -
        Flags: superreview?(bzbarsky)

        Attachment #359804 -
        Flags: review?(roc)

        Attachment #359804 -
        Flags: review?(bzbarsky)

        Attachment #359803 -
        Flags: superreview?(bzbarsky)

        Attachment #359803 -
        Flags: superreview+

        Attachment #359803 -
        Flags: review?(bzbarsky)

        Attachment #359803 -
        Flags: review+

    
    

    
  
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.

        Attachment #361708 -
        Flags: superreview?(bzbarsky) → superreview-

    
    

    
  
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?

    
    

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

    
    

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

    
    

    
  
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.

    
    

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

    
    

    
  
> wrote != nsnull instead of == nsnull.

Right.  That makes a lot more sense to me.  That's what I suggested in comment 81 too.  ;)

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

    
    

    
  
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.

    
    

    
  
See also bug 299138 as far as 404 goes.

    
    

    
  


  
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.

        Attachment #361708 -
        Attachment is obsolete: true

        Attachment #362765 -
        Flags: superreview?(bzbarsky)

        Attachment #362765 -
        Flags: review?(bzbarsky)

    
    

    
  
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.

        Attachment #362765 -
        Flags: superreview?(bzbarsky)

        Attachment #362765 -
        Flags: superreview+

        Attachment #362765 -
        Flags: review?(bzbarsky)

        Attachment #362765 -
        Flags: review+

    
    

    
  
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.

    
    

    
  
The main patch here was also responsible for the talos startup test failing on Mac only.

    
    

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

    
    

    
  
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)

    
    

    
  
(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 '/'.)

    
    

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

    
    

    
  
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.
Status: NEW → RESOLVED
Closed: 16 years ago
Priority: -- → P4
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1

    
    

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

    
  
Depends on: 479402

    
    

    
  
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.

    
    

    
  
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

    
    

    
  
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;

    
    

    
  
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

    
    

    
  
Possible regression. Please see  Bug 422851,  Bug 482941.

    
  
Depends on: 494808

    
    

    
  
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

    
    

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

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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.
Whiteboard: [doc-waiting-info]

    
    

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

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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.
Depends on: 552538
Depends on: 676637

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: