165149 - no-repeat backgrounds do not scroll correctly - overflow backgrounds

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect, P2)

Description

[Tom Gilder]

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 1

[Malcolm Rowe]

Confirmed on 1.1/Win2K

Comment 2

[Lasse Marøen]

Related to bug 152373?

Comment 3

[Boris Zbarsky [:bzbarsky]]

this is compositor.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, happens on Linux too.

Comment 5

[Kevin McCluskey (gone)]

-> dcone

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

taking?  ;)

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

slow down boris, you're scaring me.

I eat background bugs for breakfast.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Testcase

Slightly modified testcase

Comment 10

[Boris Zbarsky [:bzbarsky]]

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?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, I just noticed that. But that is a different bug.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Boris Zbarsky [:bzbarsky]]

IMO we should center the background on the canvas...

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

this is not for fixed, this is for background-attachment: scroll.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Even better testcase

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 :-).

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: oops, reattaching real testcase

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: the fix

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

[Hixie (not reading bugmail)]

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.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> 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

[Eric A. Meyer (dead account)]

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.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Eric A. Meyer (dead account)]

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.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Christopher Aillon (sabbatical, not receiving bugmail)]

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.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Christopher Aillon (sabbatical, not receiving bugmail)]

> -- 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)

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> 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

[Eric A. Meyer (dead account)]

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?

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Boris Zbarsky [:bzbarsky]]

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

[Hixie (not reading bugmail)]

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. :-)

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Michel Plungjan]

Is this related?

background-repeat: repeat-y

repeats x too!

Comment 36

[Christopher Aillon (sabbatical, not receiving bugmail)]

No, it is not related (and repeat-y works just fine for me).

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Easy fix!

This turned out to be veeery easy.

Comment 38

[Boris Zbarsky [: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....)

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 112025 [details] [diff] [review]
Easy fix!

Sorry, this is wrong. I'll have to think about it some more.

Comment 40

[Boris Zbarsky [:bzbarsky]]

*** Bug 214034 has been marked as a duplicate of this bug. ***

Comment 41

[Kris Maglione [:kmag]]

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

[Hixie (not reading bugmail)]

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

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hixie, maybe you should change the text in those testcases to say what the
behaviour should be?

Comment 44

[Hixie (not reading bugmail)]

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).

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This is the real fix as explained in the above comment.

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: oops

Sorry, an extra hunk crept in by mistake

Comment 48

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.)

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> 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.

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> 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.

Comment 51

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

> 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

[Hixie (not reading bugmail)]

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.

Comment 53

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'd hate to break pages until the style context chain was fixed. I'd rather fix
it for real myself.

Comment 55

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Do any real pages use 'background: inherit'?

Comment 56

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: better fix

Here's a fix that adjusts the style context tree. Seems to work.

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix missing hunk

Not again ... the previous attachment was missing the change to ua.css

Comment 58

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That patch does change GetCorrectedParent

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in. Thanks!!

Comment 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase

Here's a testcase I was using that tests background inheritance and bitblit
scrolling of elements with uniform backgrounds.

Comment 62

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This checkin caused a Tp hit. I'll try a patch to reduce the hit by making
nsStyleContext::GetPseudoType inline and weak.

Comment 63

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: patch

check this out...

Comment 64

[Boris Zbarsky [: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....

Comment 65

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 66

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Hixie (not reading bugmail)]

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.

Comment 68

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think that could be implemented by adding another special case to
nsCSSRendering::FindBackground ... a small change.

Comment 69

[Hixie (not reading bugmail)]

ok, cool.

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: complete cleanup

As promised ... I also removed the unnecessary aPresContext parameters

Comment 71

[Boris Zbarsky [:bzbarsky]]

SyncFrameViewGeometryDependentProperties is still taking an aStyleContext param
and this patch is still switching from aFrame->GetStyleContext() to
aStyleContext in that function....  Why?

Comment 72

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 74

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Boris Zbarsky [:bzbarsky]]

Yeah, it was just something that seemed worth looking into... if it's not easy
to make it work well, that's fine.

Comment 76

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Are you planning to land this for 1.6alpha?

Comment 77

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yikes. Somehow I forgot to ever land this.

Comment 78

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Wrong. I did land it on 10/16/2003.