Bug 112749 - [CSS Grid Layout] Improper repainting when grid item change their position
Summary: [CSS Grid Layout] Improper repainting when grid item change their position
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: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2013-03-19 15:21 PDT by Julien Chaffraix
Modified: 2013-03-20 11:52 PDT (History)
9 users (show)

See Also:


Attachments
Proposed fix 1. Added a call to repaintDuringLayoutIfMoved along with extra repaint testing. (11.95 KB, patch)
2013-03-19 15:36 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Fixed failure by switching the text to Ahem with a fixed font size. (12.30 KB, patch)
2013-03-20 11:17 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.