John Daniels Reporter
	Description • 17 years ago

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.1.10) Gecko/20071128 Ubuntu/8.04 (hardy) Firefox/2.0.0.10
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9b4pre) Gecko/2008021004 Minefield/3.0b4pre

If you set a table with a fixed background image, when you scroll it ends repeating the same lines of the image over and over. It acts weirdly when you select text as well. 

Reproducible: Always

Steps to Reproduce:
1. Go to a webpage that has a table with a fixed image background
2. Scroll up and down
Actual Results:  
The image goes all weird. Instead of moving the entire image along like it's supposed to, it renders the bottom/top of the image properly but instead of redrawing the middle, it simply scrolls the middle part of the image.

Expected Results:  
The image should move with the scrolling.

I have the same problem with Firefox 3 in Vista. My friend claims he did not have the problem, although he said there was an issue with selecting text. Generally, when I select text, it then renders the image behind the text that was just selected properly. 

Also, when the focus changes from the window or I try to take a screenshot or anything, it renders properly again. For this reason, I can only send a picture taken of my screen showing the problem, and not a screen shot. I suppose that also indicates that it's a problem with the drawing itself, and not the layout.

	Ria Klaassen (not reading all bugmail)
	Comment 4 • 17 years ago

Yes, I see it too (Windows XP), looks very weird.

	Ria Klaassen (not reading all bugmail)
	Comment 5 • 17 years ago

It's a kind of crochet pattern. Reporters screen shot is not as bad as mine. Regression range is http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-01-25+16%3A00&maxdate=2006-01-26+10%3A00
could it be Bug 317375 ?

	Damon Sicore (:damons)
	Comment 6 • 17 years ago

+'ing w/ P2.  Problem occurs on mac as well.

	Damon Sicore (:damons)
	Comment 7 • 17 years ago

Roc, can you make sure this bug is in the proper component?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 8 • 17 years ago

The problem is that the nsDisplayTableBorderBackground doesn't check IsVaryingRelativeToFrame properly. It should be returning true when any part of the table (table, rowgroup, row, or cell) has a background-attachment:fixed background.

It's not clear how to fix this efficiently. Scanning all table parts every time we need the value would be bad; table painting uses a row cursor to only examine rows that can actually intersect the dirty area, so a naive IsVaryingRelativeToFrame implementation would be asymptotically more expensive than the actual painting.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 9 • 17 years ago

I think I can solve this by having BuildDisplayList for rowgroups, rows and cells check their background style and set flags in the current table's display list item to indicate if there's a background-attachment:fixed background in the visible part of the table.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 10 • 17 years ago

Unfortunately we currently have no way to test this automatically because it's only an invalidation bug. But this testcase is a comprehensive manual test, testing fixed backgrounds on all table parts with stacking contexts introduced on all table parts as well.

Scrolling the testcase with the keyboard from top to bottom should not show anything untoward. On buggy builds, you get one-line-height strips of the Firefox logo replicated.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 11 • 17 years ago

Here's the fix along the lines I outlined. It's actually reasonably straightforward.

	Gediminas Čekanauskas
	Comment 13 • 17 years ago

Also affects Firefox 3 beta 4 (Windows XP)

	David Baron :dbaron:
	Comment 14 • 17 years ago

Instead of nsAutoSaveCurrentTableItem, maybe you should have an nsAutoPushCurrentTableItem whose constructor:
 * asserts that exactly one of the current and new are null
 * saves the current one
 * pushes on a new one passed in its constructor
and whose destructor:
 * #ifdef DEBUG, checks that GetCurrentTableItem() matches what it pushed
 * restores the old one

...and then make table cells push a null current table item so that assertion in the constructor doesn't fire.  It seems a little safer not to have the pointer hanging around when crawling through arbitrary descendants of table cells.

	David Lawhon
	Comment 15 • 17 years ago

Although it was mentioned above that this bug also occurs on Mac, I wanted to add more specifically that the bug occurs with the testcase for me with a PowerMac G5/Dual running Mac OS X 10.4.11 under Firefox 3.0b4.

	David Baron :dbaron:
	Comment 16 • 17 years ago

Comment on attachment 303976 [details] [diff] [review]
fix

r+sr=dbaron, given my comments above (or a good reason not to address them); please make sure to run reftests before landing (there are a bunch of tests that test this).

	David Baron :dbaron:
	Comment 17 • 17 years ago

Comment on attachment 303976 [details] [diff] [review]
fix

>+  /**
>+   * Helper method to check whether this table part frame or any of its
>+   * descendants which are part of the table (and therefore would
>+   * be rendered by the table background painter) are
>+   * background-attachment:fixed or otherwise vary relative to the
>+   * given ancestor frame.
>+   * @param aDescendantDepth this many levels of descendants are searched
>+   */
>+  static PRBool IsBackgroundVaryingRelativeToFrame(nsDisplayListBuilder* aBuilder,
>+      nsFrame* aFrame, nsIFrame* aAncestorFrame, PRInt32 aDescendantDepth);

You never added or used this method, so don't add the prototype.  But maybe part of the comment belongs on the method on nsDisplayTableItem?

	David Baron :dbaron:
	Comment 18 • 17 years ago

(In reply to comment #14)
>  * asserts that exactly one of the current and new are null

>  * restores the old one

And, er, never mind these... I realize that won't work for the code that handles rows and row groups that establish stacking contexts.  But I think the other points are still valid.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 19 • 17 years ago

Right. I can do the other points.

> You never added or used this method, so don't add the prototype.

Oops.

> But maybe part of the comment belongs on the method on nsDisplayTableItem?

Not really. I managed to avoid having to scan the frame tree explicitly.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 20 • 17 years ago

> Instead of nsAutoSaveCurrentTableItem, maybe you should have an
> nsAutoPushCurrentTableItem whose constructor:
> * saves the current one
> * pushes on a new one passed in its constructor

This is a slight hassle since the push is conditional. So I made the constructor do nothing (except set the builder to null) and did the above in a Push method. The destructor pops only if the stored builder pointer was set to non-null by Push.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 21 • 17 years ago

I will run reftests but I doubt they really test this, since this is just an invalidation problem.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 22 • 17 years ago

Checked in.