Closed Bug 416073 Opened 16 years ago Closed 16 years ago

[FIX]table repaint is funky

Categories

(Core :: Web Painting, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9beta4

People

(Reporter: aja+bugzilla, Assigned: bzbarsky)

References

()

Details

(Keywords: regression, testcase)

Attachments

(8 files, 2 obsolete files)

Weird repainting issues on tinderbox status page (perhaps since bz's bug 414298 checkin this afternoon)?

To reproduce: 
scroll down a few lines, 
then scroll all the way to right, 
then hit reload  

While the table(s) are loading, there is some sort of border/background 'breaking' here.

Looks okay once to whole page has reloaded.
Attached image screenshot
Firefox tinderbox, the tables finished loading.
Gut feeling: Bug 414298 (Any change that causes a table reflow invalidates the whole table)
Flags: blocking1.9?
Keywords: regression
A positive regression range would probably be useful:
- http://hourly-archive.localgho.st/
1. New profile, start Firefox
2. Go to http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox
3. Press CTRL-F5 if you see no corruption

No problem: 20080206_1337_firefox-3.0b4pre.en-US.win32
Yes problem: 20080206_1432_firefox-3.0b4pre.en-US.win32

Checkins to module PhoenixTinderbox between 2008-02-06 13:37 and 2008-02-06 14:31 : 
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1202333820&maxdate=1202337119

Pretty sure due to bug 414298
Blocks: 414298
Attached image table corruption
I just filed bug 416129. I _think_ it's a separate bug (because there is no corruption, only misplacement), but just in case it's the same I wanted to mention it here.
Seeing this for today's Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.9b4pre) Gecko/2008020702 SeaMonkey/2.0a1pre build as well, 20080205 version was fine.
The page starts loading correctly, then when not yet loaded contents within the table (e.g., images) are inserted, the table becomes misaligned in that region. This happens with both column and row alignments and stays after the page has loaded completely. Scrolling down and up resolves the issue, whereas simply refreshing the display doesn't.
If someone can create a testcase that reproduces this reliably, that would be very helpful...
Keywords: qawanted
Attached file testcase
The 100ms timeout might be a bit too tight, to see the issue, if you don't see the bug, then make the timer a bit larger.
Keywords: qawantedtestcase
Awesome.  Thanks!
Target Milestone: --- → mozilla1.9beta4
Attached file Testcase 2
Attached file Testcase 3
Attached file Testcase 4
Attached patch Fix (obsolete) — Splinter Review
I clearly overestimated how correctly tables did their invalidation....

This fixes all the testcases in this bug (and tinderbox, where I finally managed to reproduce it).
Attachment #302066 - Flags: superreview?(roc)
Attachment #302066 - Flags: review?(roc)
Assignee: roc → bzbarsky
Summary: table repaint is funky → [FIX]table repaint is funky
Add an nsLayoutUtils helper method to invalidate the overflow rect of a frame? There's a lot of repetitive code here to check whether a frame has moved, if so, invalidate its overflow area otherwise invalidate the difference in rectangles. Can you share that?

I wonder if this will hurt Tp. We're going to call Invalidate a lot...
> Add an nsLayoutUtils helper method to invalidate the overflow rect of a frame?

I can do that, yeah.

> Can you share that?

I can try, yeah... I can also try to have a helper for "invalidate the difference of these two rects" in nsFrame, I guess.

> I wonder if this will hurt Tp.

Yeah, I do too.  :(  We were invalidating all this stuff already, of course; now we'll have more calls but covering a greater area.

It might make sense to ignore Invalidate() calls on a frame that still has NS_FRAME_FIRST_REFLOW set, perhaps.  Would you like me to do that?
If we can hide the check in a few helper methods, yeah.
I was going to put the check in nsFrame::Invalidate().
Attachment #302066 - Attachment is obsolete: true
Attachment #302098 - Flags: superreview?(roc)
Attachment #302098 - Flags: review?(roc)
Attachment #302066 - Flags: superreview?(roc)
Attachment #302066 - Flags: review?(roc)
Comment on attachment 302098 [details] [diff] [review]
With those changes

bit of a rubber-stamp to be honest, but it's you.
Attachment #302098 - Flags: superreview?(roc)
Attachment #302098 - Flags: superreview+
Attachment #302098 - Flags: review?(roc)
Attachment #302098 - Flags: review+
QA Contact: ian → layout.view-rendering
clearly a blocker
Flags: blocking1.9? → blocking1.9+
Comment on attachment 302098 [details] [diff] [review]
With those changes

Requesting approval... This actually makes tables do invalidation in various cases when they currently don't.

Risk is moderate, mostly to Tp.  If it turns out that this regresses Tp, or if other issues arise, we can back this and bug 414298 out. If we don't take this, we need to back bug 414298 out instead.
Attachment #302098 - Flags: approval1.9?
Comment on attachment 302098 [details] [diff] [review]
With those changes

Ah, it's a blocker.  Nevermind.  ;)
Attachment #302098 - Flags: approval1.9?
No longer seeing the oddities on the tinderbox waterfall page:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008020801 Minefield/3.0b4pre ID:2008020801
No longer blocks: 416129
We should probably figure out how to reftest this.
The problem is that reftest paints the whole page into a canvas, so it doesn't need invalidates at all....  :(

In fact we have no test suite right now that can test invalidation bugs.
I checked this in last night.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b4pre) Gecko/2008020803 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

V.Fixed.
Status: RESOLVED → VERIFIED
No longer blocks: 416181
Flags: in-testsuite?
Depends on: 451332
Going to write some invalidation tests for the test cases provided
Attached patch reftest suite bug416073 (obsolete) — Splinter Review
Attachment #377261 - Flags: review?(roc)
You should be using the MozReftestInvalidate event instead of the load event to trigger your invalidating changes. See http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/README.txt

I think you had some otehr tests where I failed to catch this, so please fix them up too.
Attachment #377261 - Attachment is obsolete: true
Attachment #377777 - Flags: review?(roc)
Attachment #377261 - Flags: review?(roc)
Pushed test as http://hg.mozilla.org/mozilla-central/rev/7b43a7943849
Flags: in-testsuite? → in-testsuite+
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: