mdew Reporter
	Description • 15 years ago

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20090311 Shiretoko/3.1b4pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20090311 Shiretoko/3.1b4pre

This CSS3 value says the background has to scroll with the element. 

Reproducible: Always

Steps to Reproduce:
1. Open link
2. Desired result: the background image scrolls with the div.
3.
Actual Results:  
Image doesn't scroll

Expected Results:  
Desired result: the background image scrolls with the div.

	Ria Klaassen (not reading all bugmail)
	Comment 1 • 15 years ago

Only IE7 has a scrolling background image (Windows XP).

	mdew Reporter
	Comment 2 • 15 years ago

Safari 4.0.2 (530.19.1) also has support for this.

	Jeff Walden [:Waldo]
	Comment 3 • 15 years ago

Maybe for "3.7", i.e. whatever comes after the release after 3.5 (currently "3.6").  Certainly not for "3.6", have enough I need to finish for then already...

	Jeff Walden [:Waldo]
	Comment 6 • 14 years ago

The blocking2.0? request wouldn't have been a bad idea half a year ago.  This is one of the few non-bug omissions (possibly even only, but I can't say I'm up-to-date on our support and the spec) in our support for CSS3 borders/backgrounds, so it might well have made sense to block on this, so as to fill out our support.

But now, even though a release is still several months out, we are in bugfix mode.  Implementing whole new features, which must be tested both in isolation and in concert with existing features (and background properties are quite interdependent), isn't feasible if we actually want to ship.  Our blocker count for a release is too high as it is (and doubtless we mis-evaluated some of those as + rather than -).  So expect this to be minused: because we are too far into the release cycle to start work on this to satisfactorily and rigorously complete it, and because we already have an overlarge number of other bugs to fix in order to actually ship.

	Emil Ivanov
	Comment 7 • 14 years ago

OK, this is not so big deal but maybe we can see it implemented in the next release? I made the request because I saw the target milestone... Else if not possible to block 2.0 for any new features, then why not expose .next so we can mark some bugs from now, and not wait several months... just wondering things could be better that way.

	Jeff Walden [:Waldo]
	Comment 8 • 14 years ago

Target milestones don't necessarily reflect reality.  Unless you see someone actively working on a patch, commenting in the bug, and posting status updates  (or if the bug's already marked fixed), you probably should assume it's not meaningful.

I don't know why a .next hasn't been exposed yet.  There seems a good case for not exposing it yet, however, in order to focus attention on work that needs to be done *now* versus trickier work not required immediately.  This doesn't stop anyone from working on such a bug -- you could go off and write all the patches you wanted to implement this now, regardless whether there's a blocking flag -- but it does mean people are more likely to focus on what can and should be done now versus at some "distant" future time.

Since I'm busy with other things now, I'll throw this back in the pool to reflect reality.  Maybe I'll get back to it, but I guarantee it won't be before Firefox 4.

	Jeff Walden [:Waldo]
	Comment 9 • 13 years ago

I started work on this again over the weekend.  No promises or predictions for any particular due date.  If the powers that be decide this must be done sooner rather than later, I'll either expedite or execute a handoff as needed.

	Lazar Sumar [:nonsensickle]
	Comment 10 • 13 years ago

Jeff, if you're not working on this atm, I would like to give it a go.

	Jeff Walden [:Waldo]
	Comment 11 • 13 years ago

I'm still interested in this, although I'm not certain how much time I have to let myself be distracted by it.  Give me through January 2 (that day being a vacation day more amenable to diversions like this) to fix this -- if I'm not substantially done in a week and a half, I'll hand it off then.  You probably have enough things worth doing (especially this time of year) that that much delay won't hurt anything.  :-)

	Lazar Sumar [:nonsensickle]
	Comment 12 • 13 years ago

You can have as much time as you like. I was hunting for bugs and noticed the last post was a while ago so I asked. When you no longer want it feel free to reassign it to me :).

	Jeff Walden [:Waldo]
	Comment 13 • 12 years ago

Hmm.  I never did get to this over Christmas, did I?  At this rate I don't think I'm going to timely get to this -- back in the pool it goes.

	Lazar Sumar [:nonsensickle]
	Comment 17 • 12 years ago

Seems that chrome already has this one implemented... Here's a simple test case.

	Sergey Sokoloff
	Comment 18 • 12 years ago

Lea Verou at http://lea.verou.me/2012/04/background-attachment-local/ says that «local» value is implemented in IE9+, Safari 5+, Chrome and Opera.

(That's basically all modern browsers except Firefox.)

	Alan{H}
	Comment 19 • 12 years ago

Would like to see this land in Firefox. Following issue.

	Dmitry
	Comment 20 • 12 years ago

Im interested in new features in my prima browser. Join.

	Brian Blakely
	Comment 21 • 12 years ago

Throwing in my request for this feature.

	Jordan Arentsen
	Comment 22 • 12 years ago

Would love to see this implemented.

	Binyamin
	Comment 23 • 12 years ago

@Jordan Arentsen, as more votes for the issue, as bigger attention of issue importance.

	edimoldovan
	Comment 24 • 12 years ago

It would be great to use this in my future projects! The feature has my vote too!

	effexts
	Comment 25 • 12 years ago

Voted too, hope this get implemented.

	Jonathan
	Comment 26 • 11 years ago

Looking forward to this being fixed/implemented.

	Simon Sapin (:SimonSapin) Assignee
	Comment 27 • 11 years ago

The relevant spec is here: http://www.w3.org/TR/css3-background/#the-background-attachment

I’m looking into implementing this.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 28 • 11 years ago

My understanding here is that the basic idea of what we want to implement involves propagating a background from the scroll frame to the scrolled frame (i.e., the anonymous block inside it).  It then might (or might not) require special background positioning code.  (Per current spec, I think it does, but the positioning behavior you'd get without the special code is likely better.  I seem to recall fantasai saying that Chrome did positioning relative to the scrolled frame (which isn't a concept in CSS, so it requires more spec work but less implementation work).)

So one of the decisions regarding implementing this that roc and I briefly discussed last week was how we want to propagate the background to the scrolled frame.

One option would be to use background:inherit on the scrolled frame styles and then somehow suppress drawing on the scroll frame (i.e., in nsCSSRendering::FindBackground or one of the helper functions it calls).  This would be expensive on the style system side in both performance and memory since it would defeat storing in the rule tree (and also thus lead to hitting bug 862276 more).

Another option is to implement all of the propagation inside nsCSSRendering::FindBackground, without any style system inheritance.  This might require slightly more adjusting of invariants in the layout code than the first option.  Nonetheless I think I prefer at least trying it first (maybe changing course if we find problems).

	Simon Sapin (:SimonSapin) Assignee
	Comment 29 • 11 years ago

fantasai raised an issue with CSS WG about the background positioning area with 'background-attachment: local'. We might end up changing the spec to the better behavior.

http://lists.w3.org/Archives/Public/www-style/2013May/0542.html

I’ll look into the "no style inheritance" approach described by dbaron in the previous comment.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 30 • 11 years ago

So, after further discussion, that strategy won't work.

In particular, we need to be able to handle multiple background layers, some of which are 'local' and some aren't, and z-order them correctly.  Simon says that the Chrome and IE implementations of background-attachment do this correctly.

I think this means that we should implement this primarily in the display list system.  We already have separate nsDisplayBackgroundImage and nsDisplayBackgroundColor for each background layer (though nsDisplayBackgroundGeometry and the DLBI interactions might be more interesting).  I suspect we can do some sort of tricks with associating the different display items for the different layers with a different reference frame (not sure if that's still the right term) so that we have the right idea about which ones need to scroll and which ones don't.

We'll then need code to handle the position correctly as well, or something like that.

But I think roc, mattwoodrow, and tn know a lot more about display lists than I do, so cc:ing them.

	Simon Sapin (:SimonSapin) Assignee
	Comment 32 • 11 years ago

This test case shows that Chromium 25, IE10 and Opera 12 (Presto) get the stacking order right for multiple layers with background-attachment: scroll/fixed/local

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 33 • 11 years ago

I think if you modify nsCSSRendering::ComputeBackgroundPositioningArea, that might take care of everything. Yay DLBI!

	Simon Sapin (:SimonSapin) Assignee
	Comment 34 • 11 years ago

This patch does the right thing with different values of background-origin, but only in the LTR direction. Also needs tests.

Is it considered bad practice to use 'return' in the middle of a method?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 35 • 11 years ago

(In reply to Simon Sapin (:SimonSapin) from comment #34)
> Is it considered bad practice to use 'return' in the middle of a method?

Sometimes yes, sometimes no.  It's often reasonable in error handling cases, or in cases where you're intentionally saying "I don't want that other chunk of code".  In this case it's a little bit more like an if/else than an early return, though I think the early return is probably ok.  It might or might not be cleaner to make it more like an if/else.  (The reason to avoid it is in case somebody later adds another chunk of code after the early return and fails to notice that the early return is there.)

	Simon Sapin (:SimonSapin) Assignee
	Comment 36 • 11 years ago

I think I got the background positioning right this time, including with horizontal scroll and dir=rtl

However the rectangles for 'background-origin: content-box' and 'background-clip: content-box' are completely different. I will write to www-style@w3.org to ask about this.

	Simon Sapin (:SimonSapin) Assignee
	Comment 37 • 11 years ago

Add a few reftests. How much testing is appropriate for a new feature? I feel there are many more corner cases that could go wrong and not be caught by these tests…

One test is failing because the background painting area not scrolling:
http://lists.w3.org/Archives/Public/www-style/2013Jun/0276.html

roc, how should I approach having the painting area as described in the email linked above?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 38 • 11 years ago

Modify nsDisplayBackgroundImage::GetBoundsInternal. It's probably just a matter of passing the scrolled frame to nsCSSRendering::GetBackgroundLayerRect.

	Simon Sapin (:SimonSapin) Assignee
	Comment 39 • 11 years ago

Passing the scrolled frame (the result of nsIScrollableFrame::GetScrolledFrame()) is not enough. The "position" of this frame does not move when scrolling. For the background positioning area I had to make a rectangle as follows:

https://bugzilla.mozilla.org/attachment.cgi?id=764387&action=diff#a/layout/base/nsCSSRendering.cpp_sec2

    bgPositioningArea = nsRect(
      scrollableFrame->GetScrolledFrame()->GetPosition()
        // For the dir=rtl case:
        + scrollableFrame->GetScrollRange().TopLeft(),
      scrollableFrame->GetScrolledRect().Size());

… and then adjust it depending on background-origin.

It’s really the same rectangle that I need here (or its intersection with the "outer" scrollable frame) but the code path is completely different. I looked into GetBackgroundLayerRect and PrepareBackgroundLayer but I don’t see where the different values of background-clip take effect.

I tried a few different things such as returning my rectangle directly without even calling GetBackgroundLayerRect(), but in every case I get the "Presto/Trident" clipping rather than the "Blink/WebKit" clipping that I want, as described here:

http://lists.w3.org/Archives/Public/www-style/2013Jun/0387.html

What function is responsible for applying the background-clip property?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 40 • 11 years ago

GetBackgroundClip.

	Simon Sapin (:SimonSapin) Assignee
	Comment 41 • 11 years ago

Here is an attempt at changing the background painting area. The reftests now pass, but scrolling around when using the browser interactively gives an incorrect rendering: parts of the image disappear or re-appear at unexpected times. Is this an invalidation bug?

	Simon Sapin (:SimonSapin) Assignee
	Comment 42 • 11 years ago

The attached video show the bug I’m seeing. (Is something other than video preferred for this kind of thing?)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 43 • 11 years ago

Looks like incorrect invalidation or possibly layerization.

Set environment variable MOZ_DUMP_PAINT_LIST=1, run your test, and collect the display-list/layer-tree dump when the rendering is incorrect, and paste it here.

	Simon Sapin (:SimonSapin) Assignee
	Comment 44 • 11 years ago

Moving the cursor or scrolling triggers a lot of paint list output, so it’s hard to find the relevant part. The attached file contains 1493 lines, some of which happened when the bug was visible. Hopefully this is still helpful.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 45 • 11 years ago

That didn't help me much ... try getting mattwoodrow or tnikkel to help, I'm got some critical stuff on.

	Simon Sapin (:SimonSapin) Assignee
	Comment 46 • 11 years ago

Returning a rectangle in the right coordinates system from GetBackgroundClip fixed the previous weird rendering bugs, and yields a much saner behavior. However the clipping area is still not quite correct.

Currently, the BackgroundClipState describes a rectangle that can have rounded corners. In the specific case where the same element has:

    overflow: scroll;
    background-attachment: local;
    background-clip: content-box;
    padding: (non-zero);
    border-radius: (greater than padding + border);

… then the desired clipping area is the intersection of two rectangles, one of which has rounded corners while the other one scrolls. So my current plan is to change BackgroundClipState so that it can represent that, and update accordingly the code (in multiple places) that uses it.

	Simon Sapin (:SimonSapin) Assignee
	Comment 47 • 11 years ago

Victory! This updated patch yields the behavior I want in all the corner cases I could think of.

I want to add reftests that involve 'background-clip: content-box' before calling this done, but should I ask for review of the code part in the meantime?

	Simon Sapin (:SimonSapin) Assignee
	Comment 48 • 11 years ago

Here is a test-case that involves:

* Some scrolling due to 'overflow' on an element (not the viewport)
* background-attachment: local
* background-clip: content-box
* border-image
* border-color
* Non-zero padding
* Non-zero, semi-transparent border
* border-radius bigger than border + padding, so that the curve’s center is within the content area

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 49 • 11 years ago

Comment on attachment 774825 [details] [diff] [review]
Updated patch. Correct behavior (finally!), needs moar reftests.

Review of attachment 774825 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

::: layout/base/nsCSSRendering.cpp
@@ +2890,5 @@
> +      scrollableFrame->GetScrolledFrame()->GetPosition()
> +        // For the dir=rtl case:
> +        + scrollableFrame->GetScrollRange().TopLeft(),
> +      scrollableFrame->GetScrolledRect().Size());
> +    // The ScrolldRect’s size does not include the borders or scrollbars,

ScrolledRect

	Simon Sapin (:SimonSapin) Assignee
	Comment 50 • 11 years ago

Unsurprisingly, adding tests revealed a bug: background-color painting uses a different code path based on the presence of border-radius, and I had only tested one.

This patch fixes the bug (add a few lines to do the intersection of the two rectangles in GetBackgroundClip when there is no border-radius) and adds reftests for the background clipping area.

I believe this patch is ready for checkin. Since I made code changes, does the patch need to be reviewed again?

	Simon Sapin (:SimonSapin) Assignee
	Comment 51 • 11 years ago

Same patch, with 'r=roc' in the commit message.

	Ryan VanderMeulen [:RyanVM]
	Comment 52 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/9022f5fdcf98

	Ryan VanderMeulen [:RyanVM]
	Comment 53 • 11 years ago

Backed out for mochitest and reftest failures. *Please* run these patches through Try before requesting checkin.
http://hg.mozilla.org/integration/mozilla-inbound/rev/12deadfb628e

https://tbpl.mozilla.org/php/getParsedLog.php?id=25326440&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=25326785&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=25326720&tree=Mozilla-Inbound

	Simon Sapin (:SimonSapin) Assignee
	Comment 54 • 11 years ago

I’ll request push access to Try and do that. I understand some but not all of these errors. Ryan, could some of the errors be caused by unrelated patches? This issue only has one patch, but https://tbpl.mozilla.org/?rev=3e3d601a9ab2&tree=Mozilla-Inbound shows five.

	Daniel Holbert [:dholbert]
	Comment 55 • 11 years ago

(In reply to Simon Sapin (:SimonSapin) from comment #54)
> Ryan, could some of the errors be caused by unrelated
> patches? This issue only has one patch, but
> https://tbpl.mozilla.org/?rev=3e3d601a9ab2&tree=Mozilla-Inbound shows five.

Nope, looks like basically all of the orange on this push was caused by the patch here. Mochitest-5, Android's Mochitest-8, and Reftests all went green when Ryan backed this out:
 https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=12deadfb628e

(Those were still all orange in the push immediately before the backout:
 https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d5d03b2c7fab )

	Simon Sapin (:SimonSapin) Assignee
	Comment 56 • 11 years ago

Raised the fuzzy() parameters to cover cross-platform differences, fix layout/inspector/tests/test_bug877690.html

	Simon Sapin (:SimonSapin) Assignee
	Comment 57 • 11 years ago

Try results are looking good: https://tbpl.mozilla.org/?tree=Try&rev=b2c53d7b4fcc

	Ryan VanderMeulen [:RyanVM]
	Comment 58 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c2e8714201f

	Ryan VanderMeulen [:RyanVM]
	Comment 59 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/8c2e8714201f

	Jean-Yves Perrier [:teoli]
	Comment 60 • 11 years ago

Documentated in :
https://developer.mozilla.org/en-US/docs/Web/CSS/background-attachment
and
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/25

	Petruta Horea [:phorea], Desktop QA
	Comment 61 • 11 years ago

I've tested latest Aurora build (ID: 20130829004002) on several operating systems using the pages mentioned in this bug and I didn't see any issues. 

Considering that this feature is also covered by automated tests, is there anything else that needs QA testing? In this case please let me know how could I help.

Mozilla/5.0 (X11; Linux x86_64; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (X11; Linux i686; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Windows NT 5.1; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Windows NT 6.0; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Windows NT 6.2; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Machintosh; Intel Mac OS X 10.7; rv:25.0) Gecko/20100101 Firefox/25.0

	Simon Sapin (:SimonSapin) Assignee
	Comment 62 • 11 years ago

IMO the automated tests are enough for this feature. The reftests compare "screenshots" at different scrolling position by setting `element.scrollTop` and `element.scrollLeft` form JavaScript.

	Petruta Horea [:phorea], Desktop QA
	Comment 63 • 11 years ago

Marking as verified as per above comments.

	Alex Keybl [:akeybl]
	Comment 64 • 11 years ago

Adding the feature keyword so that this bug is properly picked up by the Release Tracking wiki page.