Bug 34913 - Unpainted white area appears at the edge of the page when body has bg color
Summary: Unpainted white area appears at the edge of the page when body has bg color
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: HasReduction, InRadar
: 27531 28607 30258 40368 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-02-12 16:38 PST by Kinuko Yasuda
Modified: 2010-07-19 09:53 PDT (History)
10 users (show)

See Also:


Attachments
Test case (200 bytes, text/html)
2010-02-12 16:38 PST, Kinuko Yasuda
no flags Details
a patch that worked for me (3.57 KB, patch)
2010-02-16 22:49 PST, Kinuko Yasuda
hyatt: review-
Details | Formatted Diff | Diff
Repaint the view when the body renderer appears or goes away (35.46 KB, patch)
2010-07-17 19:49 PDT, mitz
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2010-02-12 16:38:18 PST
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
Comment 1 Kinuko Yasuda 2010-02-12 16:47:36 PST
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.
Comment 2 Alexey Proskuryakov 2010-02-12 22:16:29 PST
Confirmed with Safari 4.0.4 on Mac OS X.
Comment 3 mitz 2010-02-12 22:19:44 PST
Also happens in TOT. Nice to finally see a reduction for this bug. Thank you!
Comment 4 Kinuko Yasuda 2010-02-16 22:49:08 PST
Created attachment 48862 [details]
a patch that worked for me
Comment 5 Darin Adler 2010-02-17 09:12:07 PST
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.
Comment 6 Kinuko Yasuda 2010-02-17 14:33:45 PST
(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...:))
Comment 7 Adam Barth 2010-03-22 08:57:34 PDT
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.
Comment 8 Dave Hyatt 2010-03-22 11:18:19 PDT
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).
Comment 9 Simon Fraser (smfr) 2010-03-22 11:53:00 PDT
I wonder if bug 27531 is the same issue.
Comment 10 Dave Hyatt 2010-03-22 12:46:58 PDT
Yeah I suspect so.
Comment 11 Alexey Proskuryakov 2010-04-26 13:23:10 PDT
*** Bug 30258 has been marked as a duplicate of this bug. ***
Comment 12 Alexey Proskuryakov 2010-04-26 13:23:20 PDT
*** Bug 28607 has been marked as a duplicate of this bug. ***
Comment 13 Alexey Proskuryakov 2010-04-26 13:24:24 PDT
*** Bug 27531 has been marked as a duplicate of this bug. ***
Comment 14 Alexey Proskuryakov 2010-04-26 13:25:12 PDT
The likely duplicates above will need to be double-checked when this bug is fixed.
Comment 15 mitz 2010-06-10 00:53:48 PDT
*** Bug 40368 has been marked as a duplicate of this bug. ***
Comment 16 mitz 2010-07-02 10:28:12 PDT
See also bug 16400.
Comment 17 mitz 2010-07-02 10:28:35 PDT
<rdar://problem/8108786>
Comment 18 mitz 2010-07-02 11:18:21 PDT
Didn’t realize that this was already in Radar by way of bug 27531.
<rdar://problem/7232109>
Comment 19 mitz 2010-07-17 19:49:38 PDT
Created attachment 61897 [details]
Repaint the view when the body renderer appears or goes away
Comment 20 Oliver Hunt 2010-07-18 19:49:14 PDT
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?
Comment 21 mitz 2010-07-18 19:53:29 PDT
(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.
Comment 22 Simon Fraser (smfr) 2010-07-19 08:40:14 PDT
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.
Comment 23 mitz 2010-07-19 09:53:27 PDT
Fixed in <http://trac.webkit.org/projects/webkit/changeset/63672>.