CSS3 background-attachment: local support

VERIFIED FIXED in mozilla25

Status

()

Core
CSS Parsing and Computation
--
enhancement
VERIFIED FIXED
9 years ago
4 years ago

People

(Reporter: mdew, Assigned: SimonSapin)

Tracking

(Blocks: 1 bug, {dev-doc-complete, feature})

unspecified
mozilla25
dev-doc-complete, feature
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 -, relnote-firefox 25+)

Details

(Whiteboard: [parity-ie][parity-webkit][parity-opera], URL)

Attachments

(7 attachments, 13 obsolete attachments)

601 bytes, text/css
Details
1.24 KB, image/gif
Details
28.14 KB, image/jpeg
Details
1.92 KB, text/html
Details
802 bytes, text/html
Details
2.40 KB, text/html
Details
52.68 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

9 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.
Only IE7 has a scrolling background image (Windows XP).
Component: General → Style System (CSS)
Product: Firefox → Core
QA Contact: general → style-system
(Reporter)

Comment 2

8 years ago
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...
Assignee: nobody → jwalden+bmo
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.3
Target Milestone: mozilla1.9.3 → mozilla2.0

Comment 4

7 years ago
Created attachment 465220 [details]
Testcase: background-attachment: local;

Updated

7 years ago
Whiteboard: [parity-ie][parity-webkit][parity-opera]

Updated

7 years ago
Duplicate of this bug: 589685

Updated

7 years ago
Blocks: 554013

Updated

7 years ago
blocking2.0: --- → ?
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.

Comment 7

7 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.
blocking2.0: ? → -
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.
Assignee: jwalden+bmo → nobody
Target Milestone: mozilla2.0 → Future
Blocks: 631373
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.
Assignee: nobody → jwalden+bmo
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.
Assignee: jwalden+bmo → nobody
Assignee: nobody → lsumar
Created attachment 602811 [details]
image
Created attachment 602812 [details]
background
Created attachment 602814 [details]
background
Attachment #602812 - Attachment is obsolete: true
Created attachment 602815 [details]
attachment local test page

Seems that chrome already has this one implemented... Here's a simple test case.
Attachment #602815 - Attachment mime type: text/plain → text/html
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.)

Comment 19

5 years ago
Would like to see this land in Firefox. Following issue.

Comment 20

5 years ago
Im interested in new features in my prima browser. Join.

Comment 21

5 years ago
Throwing in my request for this feature.

Comment 22

5 years ago
Would love to see this implemented.

Comment 23

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

Comment 24

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

Comment 25

5 years ago
Voted too, hope this get implemented.

Comment 26

4 years ago
Looking forward to this being fixed/implemented.
(Assignee)

Comment 27

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

I’m looking into implementing this.
Assignee: bugzilla → simon.sapin
Status: NEW → ASSIGNED
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).
(Assignee)

Comment 29

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

Comment 31

4 years ago
Created attachment 755246 [details]
Test case
(Assignee)

Comment 32

4 years ago
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
Attachment #755246 - Attachment is obsolete: true
I think if you modify nsCSSRendering::ComputeBackgroundPositioningArea, that might take care of everything. Yay DLBI!
(Assignee)

Comment 34

4 years ago
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?
Flags: needinfo?(dbaron)
(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.)
Flags: needinfo?(dbaron)
(Assignee)

Comment 36

4 years ago
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 www-style@w3.org to ask about this.
Attachment #756403 - Attachment is obsolete: true
(Assignee)

Comment 37

4 years ago
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?
Attachment #762712 - Attachment is obsolete: true
Flags: needinfo?(roc)
Modify nsDisplayBackgroundImage::GetBoundsInternal. It's probably just a matter of passing the scrolled frame to nsCSSRendering::GetBackgroundLayerRect.
Flags: needinfo?(roc)
(Assignee)

Comment 39

4 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?
Flags: needinfo?(roc)
GetBackgroundClip.
Flags: needinfo?(roc)
(Assignee)

Comment 41

4 years ago
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?
Attachment #764387 - Attachment is obsolete: true
(Assignee)

Comment 42

4 years ago
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.
(Assignee)

Comment 44

4 years ago
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.
(Assignee)

Comment 46

4 years ago
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.
Attachment #767164 - Attachment is obsolete: true
(Assignee)

Comment 47

4 years ago
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?
Attachment #767166 - Attachment is obsolete: true
Attachment #767898 - Attachment is obsolete: true
Attachment #773211 - Attachment is obsolete: true
Flags: needinfo?(roc)
(Assignee)

Comment 48

4 years ago
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
Attachment #774825 - Flags: review+
(Assignee)

Comment 50

4 years ago
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?
Attachment #774825 - Attachment is obsolete: true
Attachment #775716 - Flags: review?(roc)
Flags: needinfo?(roc)
Attachment #775716 - Flags: review?(roc) → review+
(Assignee)

Comment 51

4 years ago
Created attachment 776234 [details] [diff] [review]
Same patch, with r=roc

Same patch, with 'r=roc' in the commit message.
Attachment #775716 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/9022f5fdcf98
Flags: in-testsuite+
Keywords: checkin-needed
Keywords: dev-doc-needed
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
(Assignee)

Comment 54

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

Comment 56

4 years ago
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
Attachment #776234 - Attachment is obsolete: true
(Assignee)

Comment 57

4 years ago
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
Attachment #777801 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c2e8714201f
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8c2e8714201f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: Future → mozilla25
relnote-firefox: --- → ?

Updated

4 years ago
relnote-firefox: ? → 25+
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
Keywords: dev-doc-needed → dev-doc-complete
QA Contact: petruta.rasa
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
Flags: needinfo?(simon.sapin)
(Assignee)

Comment 62

4 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.
Flags: needinfo?(simon.sapin)
Marking as verified as per above comments.
Status: RESOLVED → VERIFIED
Adding the feature keyword so that this bug is properly picked up by the Release Tracking wiki page.
Keywords: feature
You need to log in before you can comment on or make changes to this bug.