Bug 112749 – [CSS Grid Layout] Improper repainting when grid item change their position

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

[Julien Chaffraix]

If you change a grid item's row or column, we currently don't repaint properly and will leave some artifacts behind.

[Julien Chaffraix]

Created attachment 193937 [details]
Proposed fix 1. Added a call to repaintDuringLayoutIfMoved along with extra repaint testing.

[Build Bot]

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

[Ojan Vafai]

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.

[Build Bot]

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

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 194090 [details]
Fixed failure by switching the text to Ahem with a fixed font size.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.