Bug 9121 – REGRESSION: [Incremental Repaint] DHTML movement test failures

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dave Hyatt]

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.

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.

Created attachment 8592 [details]
Reduction for the layout issue

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

[Dave Hyatt]

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?

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

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.

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.

[Dave Hyatt]

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.

I'm going to see if I can make a patch that fixes bug 8057 as well.

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.

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

[Alice Liu]

<rdar://problem/4575254>

[Dave Hyatt]

Comment on attachment 8716 [details]
Revised patch

r=me

[Darin Adler]

Committed revision 14757.

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