Closed
Bug 165149
Opened 22 years ago
Closed 20 years ago
no-repeat backgrounds do not scroll correctly - overflow backgrounds
Categories
(Core Graveyard :: GFX, defect, P2)
Core Graveyard
GFX
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: tom, Assigned: roc)
References
()
Details
(Keywords: testcase, Whiteboard: comment 19)
Attachments
(5 files, 7 obsolete files)
2.89 KB,
text/html
|
Details | |
3.23 KB,
text/html
|
Details | |
14.30 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
8.23 KB,
text/html
|
Details | |
56.86 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.1) Gecko/20020826 Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.1) Gecko/20020826 When an overflowing element has scrollbars (set to overflow:auto or scroll), and has: * A background image * background-repeat set to no-repeat * background-color not set, or set to transparent ...then the image does not scroll along with the element's contents correctly. It looks as if the image scrolls, but the background is then not redrawn so the image stays in place. Reproducible: Always Steps to Reproduce: 1. 2. 3.
Comment 2•22 years ago
|
||
Related to bug 152373?
Comment 3•22 years ago
|
||
this is compositor.
Assignee: attinasi → kmcclusk
Component: Layout → GFX Compositor
Assignee | ||
Comment 6•22 years ago
|
||
This is a layout bug, taking. I think what's happening is that we're painting the background in the scroll port and in the scrolled frame.
Assignee | ||
Comment 8•22 years ago
|
||
slow down boris, you're scaring me. I eat background bugs for breakfast.
Assignee | ||
Comment 9•22 years ago
|
||
Slightly modified testcase
Comment 10•22 years ago
|
||
I find the following code in nsCSSRendering::PaintBackgroundWithSC suspicious: 2898 if (NS_STYLE_BG_ATTACHMENT_FIXED == aColor.mBackgroundAttachment) { 2899 // If it's a fixed background attachment, then the image is placed 2900 // relative to the nearest scrolling ancestor, or the viewport if 2901 // the frame doesn't have a scrolling ancestor If I read the CSS2/CSS2.1 specs correctly, we should in fact paint relative to the viewport at all times, no?
Assignee | ||
Comment 11•22 years ago
|
||
Yeah, I just noticed that. But that is a different bug.
Assignee | ||
Comment 12•22 years ago
|
||
What I want to know is this: The testcase shows that we are drawing two backgrounds: one which gets scrolled, and one which doesn't. Which one is the one we should draw? I think that we should be drawing the one that doesn't get scrolled, because the content of the DIV is scrolled but the background is not part of the content. Furthermore, the height of the DIV is clearly 300px, so we should center the image within that, but if we let it scroll authors and users will expect it to be centered within the scrolling stuff which obviously has a much larger height. Note that the viewport contains the canvas frame so the canvas background does and should get scrolled in the normal case.
Comment 13•22 years ago
|
||
IMO we should center the background on the canvas...
Assignee | ||
Comment 14•22 years ago
|
||
this is not for fixed, this is for background-attachment: scroll.
Assignee | ||
Comment 15•22 years ago
|
||
Actually, we're not painting 2 backgrounds. We're painting THREE. This testcase shows that. (Actually so does the previous test; if you look carefully, the "unscrolled" image is a little wider than the scrolled image --- it's actually two images positioned very close together.) Basically the frame tree for DIV#main looks like this: nsGfxScrollFrame + nsScrollBoxFrame + nsBlockFrame ALL THREE of these frames inherit the background style and they all paint the background, each in a slightly different way :-).
Assignee | ||
Comment 16•22 years ago
|
||
Attachment #97721 -
Attachment is obsolete: true
Assignee | ||
Comment 17•22 years ago
|
||
Hixie convinced me that the background should scroll with the content and be positioned relative to the content size. So I will implement that by making sure that nsGfxScrollFrame and nsScrollBoxFrame never paint their backgrounds (leaving that to the scrolled content), and I will also fix the bug(s) that make frames use the incorrect size for the padding area when they're being scrolled.
Assignee | ||
Comment 18•22 years ago
|
||
OK, that wasn't so hard. Correcting the border area for scrolled frames in nsCSSRendering is a bit ugly but it's better than spraying that logic through lots of different nsIFrame::Paint() implementations.
Comment 19•22 years ago
|
||
Oh ouch, interesting, what happens to the background behind the border when you have a scrolling inside? Eek. Must bring this up to the WG.
Whiteboard: WG comment 19
Assignee | ||
Comment 20•22 years ago
|
||
> what happens to the background behind the border when you
> have a scrolling inside?
Could you explain? We don't paint the background of an element behind its
border, nor should we.
Comment 21•22 years ago
|
||
Re comment 20, yes we should! Backgrounds "start" at the content edge (that is, a 'top left' positioned background image is placed in the top left corner of the content area) but extend to the outer edge of the border, so that the background should be visible through any dashes or dots or gaps in a double border. Ian raises an excellent question.
Assignee | ||
Comment 22•22 years ago
|
||
My copy of CSS2 Rec says
> In terms of the box model, "background" refers to the background of the
> content and the padding areas.
Are you telling me this is incorrect and backgrounds extend to the border area
too? If so, then you're saying a tiled image, say a box being tiled to make a
grid, used as the background of an element with a non-opaque border, will be
drawn like this: (assuming initial background-position)
| | |
-+--+--+-
| | |
| | |
-+--+--+-
| | |
| | |
-+--+--+-
| | |
(border omitted for clarity)
That would be rather silly, if you ask me.
Comment 23•22 years ago
|
||
It would be, which is why that isn't the case. Values of 'background-position' are in relation to the padding area of the box (my earlier assertion about the content area was incorrect), as per CSS2:14.2.1 and the same portoin of CSS2.1. However, the background area goes under the borders. See CSS2.1:8.1-- it's near the end-- for an explicit description of this. This behavior was implicit in CSS2 but 2.1 makes it clear. Thus your example would looke like this inside the borders: +--+--+- | | | | | | +--+--+- | | | | | | +--+--+- | | | ...but if you had transparent borders of some thickness, you'd get something more like what you describe in comment 22. The extra bits to the top and left would be the background of the border areas. I can create a testcase if you really want one.
Assignee | ||
Comment 24•22 years ago
|
||
CSS2:8.1 has exactly this to say about background:
> The background style of the various areas of a box are determined as follows:
> * Content area: The 'background' property of the generating element.
> * Padding area: The 'background' property of the generating element.
> * Border area: The border properties of the generating element.
> * Margin area: Margins are always transparent.
It seems pretty clear from this that the background does not cover the border area.
Can you give me a URL to CSS2.1? I can't find it on the W3C Web site.
Pardon me for saying so, but at first glance it appears you've changed the
behavior in CSS2.1 to something that makes no sense for scrolling elements and
looks silly in most of the few cases where it's actually visible.
Comment 25•22 years ago
|
||
roc, here's the link to CSS2.1 : http://www.w3.org/TR/2002/WD-CSS21-20020802/ Also see bug 162252, which while sort of unrelated to this bug except through the merge conflicts we'll cause each other, has some discussion as to how backgrounds are drawn in CSS2.1 and CSS3.
Assignee | ||
Comment 26•22 years ago
|
||
OK, now it's pretty clear that CSS2.1 and CSS3 specify different behavior from CSS2. And I argue that this new behaviour is broken.
Assignee | ||
Comment 27•22 years ago
|
||
Given that -- background covering borders makes no sense because the background is scrolled and borders aren't -- background covering borders makes little sense because the background position is relative to the padding origin, not the border origin, which means you can't draw a background in the top and left borders unless you use background-repeat or negative background-position, and if you use background-repeat it looks silly because the rendered background starts with little strip of the tiled image how about we ask the WG to change CSS2.1 and CSS3 back to be in agreement with CSS2?
Comment 28•22 years ago
|
||
> -- background covering borders makes no sense because the background is scrolled and borders aren't Think transparent borders. > -- background covering borders makes little sense because the background position is relative to the padding origin, not the border origin, which means you can't draw a background in the top and left borders unless you use background-repeat or negative background-position, and if you use background-repeat it looks silly because the rendered background starts with little strip of the tiled image In CSS3, you can using the 'background-origin' property. I have that working in my tree (bug 162252)
Assignee | ||
Comment 29•22 years ago
|
||
> In CSS3, you can using the 'background-origin' property.
OK, so now there are three ways to work around this inconsistency in the spec.
Comment 30•22 years ago
|
||
Comment 27: I can see utility in both approaches, which is why there's now 'background-origin' and caillon's work in bug 162252. If I assign a repeated background image to an element that has a 20-pixel thick double border, I might expect the tiled image to fill in the gap in the border. Or maybe I'd prefer it didn't. CSS2 described the latter, and CSS2.1 describes the former; CSS3 gives the option of choosing. Let's find out what Ian learns from the WG on this subject and stop p---ing all over the work people have done to give authors what they want, shall we?
Assignee | ||
Comment 31•22 years ago
|
||
OK. Sorry for being nasty. If we have to have backgrounds painted under borders then I think I'm going to rescind my support for the idea that the background of a scrolling element gets scrolled, which was largely based on the argument that the background was painted in the padding area and therefore should be scrolled with the padding. Instead we should paint the background in the border area and not scroll it.
Comment 32•22 years ago
|
||
The problem here is that the padding _is_ getting scrolled... so in the "background-origin: padding; background-clip: border" case we still have a problem... Namely, we need to "start tiling at the padding edge" but "paint to the border edge", whatever that would mean in that case... Hence comment 19 and Ian wanting to bring this up with the WG.
Comment 33•22 years ago
|
||
roc: Actually CSS2 says the same as CSS 2.1 -- you are forgetting to take the errata into account. The CSS2 errata is _large_, and should always be checked. :-)
Updated•22 years ago
|
Priority: -- → P2
Assignee | ||
Comment 34•22 years ago
|
||
Hixie told me on IRC that the WG has decided the background should NOT be scrolled. I will attach a new patch to suit.
Comment 35•22 years ago
|
||
Is this related? background-repeat: repeat-y repeats x too!
Comment 36•22 years ago
|
||
No, it is not related (and repeat-y works just fine for me).
Assignee | ||
Comment 37•22 years ago
|
||
This turned out to be veeery easy.
Attachment #97729 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #112025 -
Flags: superreview?(bzbarsky)
Attachment #112025 -
Flags: review?(bzbarsky)
Comment 38•22 years ago
|
||
Comment on attachment 112025 [details] [diff] [review] Easy fix! hmm.. good old ua.css... Are we sure that the scrolling container does this right in general? If so, looks great. ;) (it seems to do the right thing in the cases I've looked at, but....)
Attachment #112025 -
Flags: superreview?(bzbarsky)
Attachment #112025 -
Flags: superreview+
Attachment #112025 -
Flags: review?(bzbarsky)
Attachment #112025 -
Flags: review+
Assignee | ||
Comment 39•22 years ago
|
||
Comment on attachment 112025 [details] [diff] [review] Easy fix! Sorry, this is wrong. I'll have to think about it some more.
Attachment #112025 -
Flags: superreview-
Attachment #112025 -
Flags: superreview+
Attachment #112025 -
Flags: review-
Attachment #112025 -
Flags: review+
Comment 40•21 years ago
|
||
*** Bug 214034 has been marked as a duplicate of this bug. ***
Comment 41•21 years ago
|
||
I personally believe that the image should be scrolled with the div inatead of the page. This is based on the fact that it is easy to mimic the effect of having the background fixed to the div using layers, etc. Making it scroll with the div, however, requires advanced javascript. Even than, I imagine that the sceoll would be jerky and unpleasant. Of coarse this is really for the w3c to decide and I would not have gecko defy their standards. Just wanted to give my oppinion.
Comment 42•21 years ago
|
||
CSS2.1 specifies this. Thank you for shopping with the CSS Working Group. Please come again. Test cases: http://www.hixie.ch/tests/adhoc/css/background/block/001.html http://www.hixie.ch/tests/adhoc/css/background/block/002.html http://www.hixie.ch/tests/adhoc/css/background/block/003.html http://www.hixie.ch/tests/adhoc/css/background/block/004.html
QA Contact: petersen → ian
Summary: no-repeat backgrounds do not scroll correctly → no-repeat backgrounds do not scroll correctly - overflow backgrounds
Whiteboard: WG comment 19 → comment 19
Assignee | ||
Comment 43•21 years ago
|
||
Hixie, maybe you should change the text in those testcases to say what the behaviour should be?
Comment 44•21 years ago
|
||
Yeah, I'll try to get to them at some point. Can't promise it'll happen soon. Ping me on IRC if you need them urgently (i.e. if you're going to work on this).
Assignee | ||
Comment 45•21 years ago
|
||
I am working on this :-). I have a patch that works. It's a bit annoying for a couple of reasons. My ua.css hack didn't work because it breaks background inheritance like this: <div style="height:10px; overflow:auto; background-image:url('foo');"> <p style="background:inherit;">blah blah blah</p> </div> The <p> would inherit ua.css's 'background:none'. So we have to allow the scrolled-content frames to have the proper background style. So I modify nsCSSRendering::PaintBackground to check for the scrolledContent pseudo, and suppress background painting there instead. That works fine. Because we're suppressing background painting, I also have to make nsContainerFrame::SyncFrameView mark scrolled-content views transparent. No problem there either. This does create another problem, though: it destroys our ability to do bitblit scrolling of 'overflow:auto' elements having a solid background. Right now the view manager determines that bitblit is possible because the scrolled view is opaque; therefore any non-scrolling background does not show through and the values of the pixels are determined entirely by the scrolled view. Now, however, the scrolled view is genuinely NOT opaque; we are seeing the background drawn by the scrolling container. It so happens that bitblit scrolling is still OK as long as the scrolling container's background is a solid field of a single color, because then the background is position-invariant. So now I need to detect THAT situation, inform the view manager using a new nsIView method, and then have the view manager take that into account when it analyzes the display list to see if it is safe to blit-scroll. I have done all this but it needs a little more testing.
Assignee | ||
Comment 46•21 years ago
|
||
This is the real fix as explained in the above comment.
Attachment #112025 -
Attachment is obsolete: true
Assignee | ||
Comment 47•21 years ago
|
||
Sorry, an extra hunk crept in by mistake
Assignee | ||
Updated•21 years ago
|
Attachment #131704 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #131706 -
Flags: superreview?(dbaron)
Attachment #131706 -
Flags: review?(dbaron)
If the outermost frame that scrolls has a style context with a :scrolled-content pseudo (for the element with overflow), how does it end up with background styles that you need to prevent from being painted? Background isn't inherited. (And if there's a good answer to that, why not add to the |CanPaintBackground| implementation rather than adding to the caller? I don't see any need for the implementations to be inlined -- no caller is capable of inlining them, since they're virtual and the true type is never known.)
Assignee | ||
Comment 49•21 years ago
|
||
> If the outermost frame that scrolls has a style context with a > :scrolled-content pseudo (for the element with overflow), how does it end up > with background styles that you need to prevent from being painted? Because of this rule in ua.css: *|*::-moz-scrolled-content { /* e.g., text inputs, select boxes */ background: inherit; padding: inherit; display: inherit; -moz-box-orient: inherit; } We need this to make 'background:inherit' on the child of an 'overflow:auto' element work. I guess another option would be to whack the style context parent chain but that's sort of icky too. > why not add to the |CanPaintBackground| implementation rather than adding to > the caller? Yeah, thought about it, but even if I do that, I still need to get and check the pseudo in nsContainerFrame as well so I can detect the uniform background special case. I can put the psuedo check into CanPaintBackground if you feel that's more pleasing. If I do that then I'll just call CanPaintBackground from PaintBackground.
Assignee | ||
Comment 50•21 years ago
|
||
> that's sort of icky too.
It's icky because it's all the children of the overflow:auto element that would
need to have their style context parents adjusted, and who knows what that would
break.
> We need this to make 'background:inherit' on the child of an 'overflow:auto'
> element work. I guess another option would be to whack the style context parent
> chain but that's sort of icky too.
What about '*: inherit', where * is any of the non-inherited properties not
mentioned there? I think the right solution is fixing the style context parent
chain.
Comment 52•21 years ago
|
||
Test cases updated: http://www.hixie.ch/tests/adhoc/css/background/block/001.html http://www.hixie.ch/tests/adhoc/css/background/block/002.html http://www.hixie.ch/tests/adhoc/css/background/block/003.html http://www.hixie.ch/tests/adhoc/css/background/block/004.html They are a bit wordy. I wasn't really sure how to make simple testcases out of them. I might make more some day.
To clarify comment 51: I think you should remove the 'background: inherit', remove the stuff from this patch that works around having that 'background: inherit' there, and file a bug on the style context parent chain issue.
Attachment #131706 -
Flags: superreview?(dbaron)
Attachment #131706 -
Flags: superreview-
Attachment #131706 -
Flags: review?(dbaron)
Attachment #131706 -
Flags: review-
Assignee | ||
Comment 54•21 years ago
|
||
I'd hate to break pages until the style context chain was fixed. I'd rather fix it for real myself.
Do any real pages use 'background: inherit'?
Assignee | ||
Comment 56•21 years ago
|
||
Here's a fix that adjusts the style context tree. Seems to work.
Assignee | ||
Updated•21 years ago
|
Attachment #131706 -
Attachment is obsolete: true
Assignee | ||
Comment 57•21 years ago
|
||
Not again ... the previous attachment was missing the change to ua.css
Assignee | ||
Updated•21 years ago
|
Attachment #132922 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #132923 -
Flags: superreview?(dbaron)
Attachment #132923 -
Flags: review?(dbaron)
Comment on attachment 132923 [details] [diff] [review] fix missing hunk You also need to change the implementations of nsIFrame::GetParentStyleContextFrame so that ReResolveStyleContext creates the same style context tree structure. (This is why I say the frame-style context relationship is backwards.) In reality, this means changing GetCorrectedParent in nsFrame.cpp.
Attachment #132923 -
Flags: superreview?(dbaron)
Attachment #132923 -
Flags: superreview-
Attachment #132923 -
Flags: review?(dbaron)
Attachment #132923 -
Flags: review-
Assignee | ||
Comment 59•21 years ago
|
||
That patch does change GetCorrectedParent
Attachment #132923 -
Flags: superreview-
Attachment #132923 -
Flags: superreview+
Attachment #132923 -
Flags: review-
Attachment #132923 -
Flags: review+
Assignee | ||
Comment 60•21 years ago
|
||
Checked in. Thanks!!
Assignee | ||
Comment 61•21 years ago
|
||
Here's a testcase I was using that tests background inheritance and bitblit scrolling of elements with uniform backgrounds.
Assignee | ||
Comment 62•21 years ago
|
||
This checkin caused a Tp hit. I'll try a patch to reduce the hit by making nsStyleContext::GetPseudoType inline and weak.
Assignee | ||
Comment 63•21 years ago
|
||
check this out...
Assignee | ||
Updated•21 years ago
|
Attachment #133073 -
Flags: superreview?(bzbarsky)
Attachment #133073 -
Flags: review?(bzbarsky)
Comment 64•21 years ago
|
||
Comment on attachment 133073 [details] [diff] [review] patch >Index: layout/html/base/src/nsContainerFrame.cpp >- nsCOMPtr<nsIAtom> pseudo = aFrame->GetStyleContext()->GetPseudoType(); >- >- if (pseudo == nsCSSAnonBoxes::scrolledContent) { >+ if (aStyleContext->GetPseudoType() == nsCSSAnonBoxes::scrolledContent) { Are we guaranteed that aStyleContext is the style context of aFrame here? If not, why is it the right style context to look at? Looks good with that cleared up....
(And if it is guaranteed to be the style context of |aFrame|, why is it a parameter to the function? If not, it could use some documentation.)
Assignee | ||
Comment 66•21 years ago
|
||
This is a legacy issue, that parameter has been in CreateViewForFrame since time immemorial, so FrameNeedsView takes the parameter just in case it's not the frame's own style context. But now that you ask ... I believe that anywhere aStyleContext != aFrame->GetStyleContext(), it's simply a bug. Here are the only cases I can find: http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstructor.cpp#13045 Looks like just a bug. http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstructor.cpp#13473 http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstructor.cpp#13483 Should probably be using the frame's style context. If this code really requires a view to be created (it appears so), just pass PR_TRUE as the aForce parameter. I'll make a patch to make this changes and do all the cleanup.
Comment 67•21 years ago
|
||
I briefly tested a build with this painting fix, and it works great. roc: Incidentally, how hard would it be to add support for a new background- attachment keyword (-moz-overflow-scroll) that scrolls the background with the overflow, instead of having it pinned to the document as per the CSS2.1 draft and as per your implementation? It's likely CSS3 will have such a keyword.
Assignee | ||
Comment 68•21 years ago
|
||
I think that could be implemented by adding another special case to nsCSSRendering::FindBackground ... a small change.
Comment 69•21 years ago
|
||
ok, cool.
Assignee | ||
Comment 70•21 years ago
|
||
As promised ... I also removed the unnecessary aPresContext parameters
Attachment #133073 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #133243 -
Flags: superreview?(bzbarsky)
Attachment #133243 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•21 years ago
|
Attachment #133073 -
Flags: superreview?(bzbarsky)
Attachment #133073 -
Flags: review?(bzbarsky)
Comment 71•21 years ago
|
||
SyncFrameViewGeometryDependentProperties is still taking an aStyleContext param and this patch is still switching from aFrame->GetStyleContext() to aStyleContext in that function.... Why?
Assignee | ||
Comment 72•21 years ago
|
||
More legacy issues; SyncFrameViewAfterReflow and its brethren historically took an aStyleContext parameter. It is unnecessary and can be removed, but if it's OK with you I'd rather save that cleanup for later. This patch has already grown quite a bit.
Comment 73•21 years ago
|
||
Comment on attachment 133243 [details] [diff] [review] complete cleanup OK. How about adding a "aFrame->GetStyleContext() == aStyleContext" assert to that method then, and filing a followup bug for that cleanup? nsBoxFrame could remove the HasView() check before calling CreateViewForFrame on itself. r+sr=bzbarsky with those nits fixed. We could use a follow-up bug on removing nsBoxFrame::CreateViewForFrame or fixing it up the same way. The frame constructor changes make me tempted to move the CreateViewForFrame stuff into Init()... Again, followup-bug material.
Attachment #133243 -
Flags: superreview?(bzbarsky)
Attachment #133243 -
Flags: superreview+
Attachment #133243 -
Flags: review?(bzbarsky)
Attachment #133243 -
Flags: review+
Assignee | ||
Comment 74•21 years ago
|
||
OK.
> The frame constructor changes make me tempted to move the CreateViewForFrame
> stuff into Init()
That would be good if it could be done safely, but I'm not 100% sure of that.
Comment 75•21 years ago
|
||
Yeah, it was just something that seemed worth looking into... if it's not easy to make it work well, that's fine.
Are you planning to land this for 1.6alpha?
Assignee | ||
Comment 77•20 years ago
|
||
Yikes. Somehow I forgot to ever land this.
Assignee | ||
Comment 78•20 years ago
|
||
Wrong. I did land it on 10/16/2003.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•