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)

defect

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.
Confirmed on 1.1/Win2K
Status: UNCONFIRMED → NEW
Ever confirmed: true
Related to bug 152373?
this is compositor.
Assignee: attinasi → kmcclusk
Component: Layout → GFX Compositor
Oh, happens on Linux too.
OS: Windows 98 → All
Hardware: PC → All
-> dcone
Assignee: kmcclusk → dcone
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.
taking?  ;)
Assignee: dcone → roc+moz
slow down boris, you're scaring me.

I eat background bugs for breakfast.
Attached file Testcase
Slightly modified testcase
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?
Yeah, I just noticed that. But that is a different bug.
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.
IMO we should center the background on the canvas...
this is not for fixed, this is for background-attachment: scroll.
Attached file Even better testcase (obsolete) —
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 :-).
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.
Attached patch the fix (obsolete) — Splinter Review
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.
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
> 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.
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.
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.
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.
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.
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.
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.
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?
> -- 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)
> 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 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?
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.
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.
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. :-)
Priority: -- → P2
Hixie told me on IRC that the WG has decided the background should NOT be
scrolled. I will attach a new patch to suit.
Is this related?

background-repeat: repeat-y

repeats x too!
No, it is not related (and repeat-y works just fine for me).
Attached patch Easy fix! (obsolete) — Splinter Review
This turned out to be veeery easy.
Attachment #97729 - Attachment is obsolete: true
Attachment #112025 - Flags: superreview?(bzbarsky)
Attachment #112025 - Flags: review?(bzbarsky)
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+
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+
*** Bug 214034 has been marked as a duplicate of this bug. ***
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.
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
Hixie, maybe you should change the text in those testcases to say what the
behaviour should be?
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).
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.
Attached patch fix (obsolete) — Splinter Review
This is the real fix as explained in the above comment.
Attachment #112025 - Attachment is obsolete: true
Attached patch oops (obsolete) — Splinter Review
Sorry, an extra hunk crept in by mistake
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.)
> 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.
> 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.
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-
Blocks: 220848
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'?
Attached patch better fix (obsolete) — Splinter Review
Here's a fix that adjusts the style context tree. Seems to work.
Attached patch fix missing hunkSplinter Review
Not again ... the previous attachment was missing the change to ua.css
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-
That patch does change GetCorrectedParent
Attachment #132923 - Flags: superreview-
Attachment #132923 - Flags: superreview+
Attachment #132923 - Flags: review-
Attachment #132923 - Flags: review+
Attached file testcase
Here's a testcase I was using that tests background inheritance and bitblit
scrolling of elements with uniform backgrounds.
This checkin caused a Tp hit. I'll try a patch to reduce the hit by making
nsStyleContext::GetPseudoType inline and weak.
Attached patch patch (obsolete) — Splinter Review
check this out...
Attachment #133073 - Flags: superreview?(bzbarsky)
Attachment #133073 - Flags: review?(bzbarsky)
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.)
Keywords: testcase
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.
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.
I think that could be implemented by adding another special case to
nsCSSRendering::FindBackground ... a small change.
ok, cool.
Attached patch complete cleanupSplinter Review
As promised ... I also removed the unnecessary aPresContext parameters
Attachment #133073 - Attachment is obsolete: true
Attachment #133243 - Flags: superreview?(bzbarsky)
Attachment #133243 - Flags: review?(bzbarsky)
Attachment #133073 - Flags: superreview?(bzbarsky)
Attachment #133073 - Flags: review?(bzbarsky)
SyncFrameViewGeometryDependentProperties is still taking an aStyleContext param
and this patch is still switching from aFrame->GetStyleContext() to
aStyleContext in that function....  Why?
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 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+
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.
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?
Yikes. Somehow I forgot to ever land this.
Wrong. I did land it on 10/16/2003.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.