Bug 34913 – Unpainted white area appears at the edge of the page when body has bg color

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Kinuko Yasuda]

Created attachment 48677 [details]
Test case

The attached test page sometime shows unpainted white border areas at the left, bottom and right edge of the page on my latest WebKit build and Nightly build on Mac.
In most cases it renders ok, but if you intentionally move the mouse on the browser while loading the page (thus forcing a repaint for hitTest during the load) it often leaves the edges unpainted.

My understanding in repainting flow is limited, but to me it appears like a 
timing issue.  And also it's because our user-agent CSS (html.css) defines the body has 8px 
margin by default, makes the area unpainted in some cases.

What I've observed is:
1. when a page is being loaded, WebKit first invalidates whole page area several times (e.g. when stylesheet is loaded and etc) and then invalidates smaller rects for DOM nodes when they are laid out.
2. RenderView asks WebKit to paint the invalidated rects, and WebKit first tries to paint the root box area (i.e. #document node) with document->body()'s background color.  This usually works well, but if the body hasn't been loaded yet at this time WebKit doesn't do actual painting.
3. RenderView clears the invalidated rects after asking WebKit to paint whether WebKit fill the whole page area or not.  Later WebKit generates another set of invalidates for body and other nodes when they're being laid out, but the body has 8px margin and WebKit doesn't paint the area.

Original issue is reported at: http://crbug.com/29880

[Kinuko Yasuda]

I'm attaching a workaround patch that worked for me, but am not sure if it does the right thing in the whole repainting flow.  It has no tests either for now.

[Alexey Proskuryakov]

Confirmed with Safari 4.0.4 on Mac OS X.

Also happens in TOT. Nice to finally see a reduction for this bug. Thank you!

[Kinuko Yasuda]

Created attachment 48862 [details]
a patch that worked for me

[Darin Adler]

It seems a shame to have to add yet another data member to resolve this, but perhaps this is the best solution.

Can we create a regression test or not? Saying “for now” makes me want to do review- and wait for you to create the test. But perhaps that is impractical.

[Kinuko Yasuda]

(In reply to comment #5)
> It seems a shame to have to add yet another data member to resolve this, but
> perhaps this is the best solution.
> 
> Can we create a regression test or not? Saying “for now” makes me want to do
> review- and wait for you to create the test. But perhaps that is impractical.

Well, sorry the patch description may have been inadequate - I tried a bit after sending a patch, but so far have not been able to make up a good test for this one.  If you guys think it's ok to have a manual-test only (and if the patch itself looks ok), I'd like it to be review+'ed.
(And I've removed the 'for now' part from the attachment description...:))

[Adam Barth]

Comment on attachment 48862 [details]
a patch that worked for me

This sounds like an important bug to fix, given the simplicity (and apparent generality) of the manual test.  Like Darin, I wish there was an automated way to test this patch, but one doesn't appear to be forthcoming.

It's a shame this patch has been sitting around for so long.

[Dave Hyatt]

Comment on attachment 48862 [details]
a patch that worked for me

This is not the right fix.  It's just a workaround hack.  A real fix would make sure the right repainting happens when the body becomes available (i.e., detect propagation and then make sure the larger area is repainted).

[Simon Fraser (smfr)]

I wonder if bug 27531 is the same issue.

[Dave Hyatt]

Yeah I suspect so.

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

The likely duplicates above will need to be double-checked when this bug is fixed.

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

See also bug 16400.

<rdar://problem/8108786>

Didn’t realize that this was already in Radar by way of bug 27531.
<rdar://problem/7232109>

Created attachment 61897 [details]
Repaint the view when the body renderer appears or goes away

[Oliver Hunt]

Comment on attachment 61897 [details]
Repaint the view when the body renderer appears or goes away

> Index: WebCore/rendering/RenderObjectChildList.cpp
> ===================================================================
> --- WebCore/rendering/RenderObjectChildList.cpp	(revision 63603)
> +++ WebCore/rendering/RenderObjectChildList.cpp	(working copy)
> @@ -62,9 +62,11 @@ RenderObject* RenderObjectChildList::rem
>      // disappears gets repainted properly.
>      if (!owner->documentBeingDestroyed() && fullRemove && oldChild->m_everHadLayout) {
>          oldChild->setNeedsLayoutAndPrefWidthsRecalc();
> +        if (oldChild->isBody())
> +            owner->view()->repaint();
>          oldChild->repaint();

Shouldn't this get an else rather than arbitrarily repainting oldChild? Otherwise we end up computing the dirty rect for all intersecting elements unnecessarily don't we?

(In reply to comment #20)
> (From update of attachment 61897 [details])
> > Index: WebCore/rendering/RenderObjectChildList.cpp
> > ===================================================================
> > --- WebCore/rendering/RenderObjectChildList.cpp	(revision 63603)
> > +++ WebCore/rendering/RenderObjectChildList.cpp	(working copy)
> > @@ -62,9 +62,11 @@ RenderObject* RenderObjectChildList::rem
> >      // disappears gets repainted properly.
> >      if (!owner->documentBeingDestroyed() && fullRemove && oldChild->m_everHadLayout) {
> >          oldChild->setNeedsLayoutAndPrefWidthsRecalc();
> > +        if (oldChild->isBody())
> > +            owner->view()->repaint();
> >          oldChild->repaint();
> 
> Shouldn't this get an else rather than arbitrarily repainting oldChild? Otherwise we end up computing the dirty rect for all intersecting elements unnecessarily don't we?

If oldChild is the body renderer, the oldChild->repaint() computes that renderer’s and only that renderer’s repaint rect, a computation whose complexity is linear in the number of ancestors (typically 2 in this case). I can add “else” when landing.

[Simon Fraser (smfr)]

Comment on attachment 61897 [details]
Repaint the view when the body renderer appears or goes away

r=me but I'd like to see HTML comments in the test files explaining what the expected results are.

Fixed in <http://trac.webkit.org/projects/webkit/changeset/63672>.