416073 - [FIX]table repaint is funky

Status: VERIFIED FIXED

(Core :: Web Painting, defect)

Description

[aja+bugzilla]

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.

Comment 1

[philippe (part-time)]

Attachment: screenshot

Firefox tinderbox, the tables finished loading.

Comment 2

[Mathieu Pellerin]

Gut feeling: Bug 414298 (Any change that causes a table reflow invalidates the whole table)

Comment 3

[Steve England [:stevee]]

A positive regression range would probably be useful:
- http://hourly-archive.localgho.st/

Comment 4

[Steve England [:stevee]]

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

Comment 5

[Steve England [:stevee]]

Attachment: table corruption

Comment 6

[[e-mail] (:bhearsum)]

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.

Comment 7

[rsx11m]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

If someone can create a testcase that reproduces this reliably, that would be very helpful...

Comment 9

[Martijn Wargers (dead)]

Attachment: testcase

Comment 10

[Martijn Wargers (dead)]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Awesome.  Thanks!

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase 2

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase 3

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase 4

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

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

Comment 16

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

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

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

If we can hide the check in a few helper methods, yeah.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I was going to put the check in nsFrame::Invalidate().

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: With those changes

Comment 21

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

Comment on attachment 302098 [details] [diff] [review]
With those changes

bit of a rubber-stamp to be honest, but it's you.

Comment 22

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

clearly a blocker

Comment 23

[Boris Zbarsky [:bzbarsky]]

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.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 302098 [details] [diff] [review]
With those changes

Ah, it's a blocker.  Nevermind.  ;)

Comment 25

[aja+bugzilla]

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

Comment 28

[(not currently active) Ted Mielczarek]

We should probably figure out how to reftest this.

Comment 29

[Boris Zbarsky [:bzbarsky]]

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.

Comment 30

[Boris Zbarsky [:bzbarsky]]

I checked this in last night.

Comment 31

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b4pre) Gecko/2008020803 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

V.Fixed.

Comment 32

[Aaron Train [:aaronmt]]

Going to write some invalidation tests for the test cases provided

Comment 33

[Aaron Train [:aaronmt]]

Attachment: reftest suite bug416073

Comment 34

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

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.

Comment 35

[Aaron Train [:aaronmt]]

Attachment: reftest suite bug416073-fix

Comment 36

[Boris Zbarsky [:bzbarsky]]

Pushed test as http://hg.mozilla.org/mozilla-central/rev/7b43a7943849