Bug 112749

Summary: [CSS Grid Layout] Improper repainting when grid item change their position
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: Layout and RenderingAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, eric, esprehn+autocc, ojan.autocc, ojan, rniwa, tony, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 60731    
Attachments:
Description Flags
Proposed fix 1. Added a call to repaintDuringLayoutIfMoved along with extra repaint testing.
none
Fixed failure by switching the text to Ahem with a fixed font size. none

Description Julien Chaffraix 2013-03-19 15:21:56 PDT
If you change a grid item's row or column, we currently don't repaint properly and will leave some artifacts behind.
Comment 1 Julien Chaffraix 2013-03-19 15:36:10 PDT
Created attachment 193937 [details]
Proposed fix 1. Added a call to repaintDuringLayoutIfMoved along with extra repaint testing.
Comment 2 Build Bot 2013-03-19 16:21:59 PDT
Comment on attachment 193937 [details]
Proposed fix 1. Added a call to repaintDuringLayoutIfMoved along with extra repaint testing.

Attachment 193937 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17257072

New failing tests:
fast/css-grid-layout/grid-element-change-rows-repaint.html
fast/css-grid-layout/grid-item-change-column-repaint.html
fast/css-grid-layout/grid-item-change-row-repaint.html
fast/css-grid-layout/grid-element-change-columns-repaint.html
Comment 3 Ojan Vafai 2013-03-19 20:04:07 PDT
Comment on attachment 193937 [details]
Proposed fix 1. Added a call to repaintDuringLayoutIfMoved along with extra repaint testing.

View in context: https://bugs.webkit.org/attachment.cgi?id=193937&action=review

> Source/WebCore/rendering/RenderGrid.cpp:677
> +        if (!selfNeedsLayout() && child->checkForRepaintDuringLayout())
> +            child->repaintDuringLayoutIfMoved(oldChildRect);

It bugs me that we're copy-pasting this pattern all over the place. Would be nice if we had a function on RenderBox:
void RenderBox::repaintChildDuringLayoutIfMoved(RenderBox* child, LayoutRect oldChildRect)
{
    // If the child moved, we have to repaint it as well as any floating/positioned
    // descendants.  An exception is if we need a layout.  In this case, we know we're going to
    // repaint ourselves (and the child) anyway.
    if (!selfNeedsLayout() && child->checkForRepaintDuringLayout())
        child->repaintDuringLayoutIfMoved(oldChildRect);
}

I copy-pasted that comment from RenderFlexibleBox since I needed that extra explanation for why the !selfNeedsLayout() check is correct.
Comment 4 Build Bot 2013-03-19 23:12:00 PDT
Comment on attachment 193937 [details]
Proposed fix 1. Added a call to repaintDuringLayoutIfMoved along with extra repaint testing.

Attachment 193937 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17133425

New failing tests:
fast/css-grid-layout/grid-element-change-rows-repaint.html
fast/css-grid-layout/grid-item-change-column-repaint.html
fast/css-grid-layout/grid-item-change-row-repaint.html
fast/css-grid-layout/grid-element-change-columns-repaint.html
Comment 5 Julien Chaffraix 2013-03-20 11:16:49 PDT
Comment on attachment 193937 [details]
Proposed fix 1. Added a call to repaintDuringLayoutIfMoved along with extra repaint testing.

View in context: https://bugs.webkit.org/attachment.cgi?id=193937&action=review

>> Source/WebCore/rendering/RenderGrid.cpp:677
>> +            child->repaintDuringLayoutIfMoved(oldChildRect);
> 
> It bugs me that we're copy-pasting this pattern all over the place. Would be nice if we had a function on RenderBox:
> void RenderBox::repaintChildDuringLayoutIfMoved(RenderBox* child, LayoutRect oldChildRect)
> {
>     // If the child moved, we have to repaint it as well as any floating/positioned
>     // descendants.  An exception is if we need a layout.  In this case, we know we're going to
>     // repaint ourselves (and the child) anyway.
>     if (!selfNeedsLayout() && child->checkForRepaintDuringLayout())
>         child->repaintDuringLayoutIfMoved(oldChildRect);
> }
> 
> I copy-pasted that comment from RenderFlexibleBox since I needed that extra explanation for why the !selfNeedsLayout() check is correct.

I totally agree with you and that's actually a deeper issue than that. Our current repainting code (LayoutRepainter / RenderLayer based + such ad-hoc code) doesn't work as well as it should. If you look at the invalidated rectangles, we actually have several unneeded and plain wrong invalidation because of LayoutRepainter. On top of that, it's too easy to forget to invalidate some children.

I will add the comment as it can be confusing for people.
Comment 6 Julien Chaffraix 2013-03-20 11:17:24 PDT
Created attachment 194090 [details]
Fixed failure by switching the text to Ahem with a fixed font size.
Comment 7 WebKit Review Bot 2013-03-20 11:52:24 PDT
Comment on attachment 194090 [details]
Fixed failure by switching the text to Ahem with a fixed font size.

Clearing flags on attachment: 194090

Committed r146371: <http://trac.webkit.org/changeset/146371>
Comment 8 WebKit Review Bot 2013-03-20 11:52:28 PDT
All reviewed patches have been landed.  Closing bug.