Bug 9121 - REGRESSION: [Incremental Repaint] DHTML movement test failures
Summary: REGRESSION: [Incremental Repaint] DHTML movement test failures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: Nobody
URL: https://bugzilla.mozilla.org/attachme...
Keywords: InRadar, Regression
: 8057 (view as bug list)
Depends on:
Blocks:
 
Reported: 2006-05-26 00:17 PDT by Dave Hyatt
Modified: 2006-06-20 09:51 PDT (History)
2 users (show)

See Also:


Attachments
Reduction for the layout issue (1.11 KB, text/html)
2006-05-29 11:39 PDT, mitz
no flags Details
Sketch: remove positioned objects from their old containing block (2.61 KB, patch)
2006-05-31 15:08 PDT, mitz
no flags Details | Formatted Diff | Diff
Fix for the layout issue (3.94 KB, patch)
2006-06-01 10:10 PDT, mitz
no flags Details | Formatted Diff | Diff
Patch for both problems, including change log and tests (11.37 KB, patch)
2006-06-04 11:51 PDT, mitz
hyatt: review-
Details | Formatted Diff | Diff
Revised patch (35.86 KB, patch)
2006-06-05 10:21 PDT, mitz
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2006-05-26 00:17:40 PDT
Run the test at

https://bugzilla.mozilla.org/attachment.cgi?id=174236

Two of the tests fail, and the red block doesn't move at all.  In shipping Safari they do.

Note in both shipping Safari and TOT that some of the tests have a 1 pixel red stripe epaint glitch at the end of a single test.
Comment 1 mitz 2006-05-29 10:07:29 PDT
I played with it a little and found out that it's relayout that stops happening. There seems to be some race condition leading to the red block and its ancestors up to BODY being marked as needing layout while HTML and above aren't.
Comment 2 mitz 2006-05-29 11:39:44 PDT
Created attachment 8592 [details]
Reduction for the layout issue
Comment 3 mitz 2006-05-31 15:08:13 PDT
Created attachment 8629 [details]
Sketch: remove positioned objects from their old containing block

The layout problem stems from the following situation: if object C is absolutely positioned, its parent B is static, and B's parent A is relative, and then you change B to be relative, C isn't removed from A's list of positioned objects. The patch gives a very rough sketch of a possible fix. It doesn't address the other direction (B going from relative to static), which should be easier, it has obvious deficiencies (indicated by the FIXMEs) and it doesn't mark the positioned objects for layout, which it should.

However, I am not 100% convinced that this approach will also fix the repaint issue. It might be easier, and acceptable, to simply change Node::diff to return Detach upon changes in position to/from static.
Comment 4 mitz 2006-05-31 15:45:12 PDT
Comment on attachment 8629 [details]
Sketch: remove positioned objects from their old containing block

This is lame even for a sketch... I should yank only absolutely-positioned object, not fixed-positioned ones.
Comment 5 Dave Hyatt 2006-05-31 18:07:34 PDT
I wonder if it would be useful to figure out what checkin broke this.... we obviously used to do the right thing, which makes me think there may be an easy way to fix this bug?
Comment 6 mitz 2006-05-31 22:39:43 PDT
(In reply to comment #5)
> I wonder if it would be useful to figure out what checkin broke this.... we
> obviously used to do the right thing, which makes me think there may be an easy
> way to fix this bug?

The reduction (attachment 8592 [details]) is broken in WebKit-418.7 as well (and in the Oct 1 nightly). The test in the URL works in r12668 but not in r12724. The most likely candidate in this range is r12681 (fix for bug 7095). There's also r12712 (fix for bug 5813), but I don't think it's related. I'll try to verify that it's r12681.
Comment 7 mitz 2006-06-01 10:10:37 PDT
Created attachment 8643 [details]
Fix for the layout issue

This passes all layout tests and fixes the "freezing" regression, but not the 1px red stripe repaint issue. I have a theory about the latter, which predicts that you have to detach() for this change after all, but I still need to investigate.
Comment 8 mitz 2006-06-04 11:51:55 PDT
Created attachment 8696 [details]
Patch for both problems, including change log and tests

This patch fixes both the "freezing" and the red 1px stripe.

The repaint test shows excessive painting being done. That's not a result of this patch, though, so I think it can be left for a separate bug. The same goes for the buttons in the original test showing a repaint glitch when their content changes.
Comment 9 Dave Hyatt 2006-06-04 11:59:59 PDT
Comment on attachment 8696 [details]
Patch for both problems, including change log and tests

This check:

if (isRenderBlock() && (!isInline() || isReplaced()))

is probably overkill.  The only time a RenderBlock becomes inline (without being an inline-block) is when it's a compact or run-in.

I'd just do:

if (isRenderBlock())

instead and not worry about optimizing for the compact/run-in case.
Comment 10 mitz 2006-06-04 14:00:36 PDT
I'm going to see if I can make a patch that fixes bug 8057 as well.
Comment 11 mitz 2006-06-05 10:21:23 PDT
Created attachment 8716 [details]
Revised patch

I changed the condition as Hyatt's suggested. I also fixed a bug in removePositionedObjects where it advanced the iterator after removing an object, and added a return in removePositionedObject. This version of the patch fixes bug 8057 as well.
Comment 12 mitz 2006-06-05 10:22:28 PDT
*** Bug 8057 has been marked as a duplicate of this bug. ***
Comment 13 Alice Liu 2006-06-06 09:47:39 PDT
<rdar://problem/4575254>
Comment 14 Dave Hyatt 2006-06-06 12:37:23 PDT
Comment on attachment 8716 [details]
Revised patch

r=me
Comment 15 Darin Adler 2006-06-06 20:09:03 PDT
Committed revision 14757.
Comment 16 mitz 2006-06-20 09:51:29 PDT
(In reply to comment #8)
>
> The repaint test shows excessive painting being done. That's not a result of
> this patch, though, so I think it can be left for a separate bug.

The proposed patch for bug 9497 fixes that.