601 bytes, text/css
1.24 KB, image/gif
28.14 KB, image/jpeg
1.92 KB, text/html
802 bytes, text/html
2.40 KB, text/html
52.68 KB, patch
|Details | Diff | Splinter Review|
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.
Only IE7 has a scrolling background image (Windows XP).
Safari 4.0.2 (530.19.1) also has support for this.
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...
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.
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.
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.
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.
Jeff, if you're not working on this atm, I would like to give it a go.
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. :-)
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 :).
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.
Created attachment 602815 [details] attachment local test page Seems that chrome already has this one implemented... Here's a simple test case.
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.)
Would like to see this land in Firefox. Following issue.
Im interested in new features in my prima browser. Join.
Throwing in my request for this feature.
Would love to see this implemented.
@Jordan Arentsen, as more votes for the issue, as bigger attention of issue importance.
It would be great to use this in my future projects! The feature has my vote too!
Voted too, hope this get implemented.
Looking forward to this being fixed/implemented.
The relevant spec is here: http://www.w3.org/TR/css3-background/#the-background-attachment I’m looking into implementing this.
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).
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.
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.
Created attachment 755247 [details] Test case: multiple overlapping layers with different background-attachment values 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
I think if you modify nsCSSRendering::ComputeBackgroundPositioningArea, that might take care of everything. Yay DLBI!
Created attachment 756403 [details] [diff] [review] WIP patch. Wrong with dir=rtl, needs tests 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?
(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.)
Created attachment 762712 [details] [diff] [review] Updated patch. Fixes positioning, but seems inconsistent with background-clip 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 firstname.lastname@example.org to ask about this.
Created attachment 764387 [details] [diff] [review] Updated patch. Adds tests, one failing because of painting area 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?
Modify nsDisplayBackgroundImage::GetBoundsInternal. It's probably just a matter of passing the scrolled frame to nsCSSRendering::GetBackgroundLayerRect.
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?
Created attachment 767164 [details] [diff] [review] Updated patch. Reftests passing, but interactive rendering is incorrect 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?
Created attachment 767166 [details] Screen capture with attachment 767164 [details] [diff] [review] The attached video show the bug I’m seeing. (Is something other than video preferred for this kind of thing?)
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.
Created attachment 767898 [details] Output of MOZ_DUMP_PAINT_LIST=1 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.
That didn't help me much ... try getting mattwoodrow or tnikkel to help, I'm got some critical stuff on.
Created attachment 773211 [details] [diff] [review] Updated patch. Saner behavior, but clipping area still incorrect 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.
Created attachment 774825 [details] [diff] [review] Updated patch. Correct behavior (finally!), needs moar reftests. 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?
Created attachment 774831 [details] "Pile of interesting corner cases" test case 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
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
Created attachment 775716 [details] [diff] [review] Updated patch. One bug fix, more tests. 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?
Created attachment 776234 [details] [diff] [review] Same patch, with r=roc Same patch, with 'r=roc' in the commit message.
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
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.
(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 )
Created attachment 777801 [details] [diff] [review] Updated patch, needs to go through Try. Raised the fuzzy() parameters to cover cross-platform differences, fix layout/inspector/tests/test_bug877690.html
Created attachment 779194 [details] [diff] [review] Updated patch. Avoid text in reftests as it fails by a few pixels on Mac and Android. Try results are looking good: https://tbpl.mozilla.org/?tree=Try&rev=b2c53d7b4fcc
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
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
Marking as verified as per above comments.
Adding the feature keyword so that this bug is properly picked up by the Release Tracking wiki page.