Bug 59863 – Incomplete repaint of boxes with inset box-shadow and padding when resized

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

[Tushar Pokle]

Created attachment 91800 [details]
Open in webkit and wait half a second to see the artifacts appear

Open the attached bgbug.html in Webkit, and after half a second, you'll see two red shadow bands (I expect there to be just one). If you resize the browser or swith to a different application, the artifacts disappear.

[Alexey Proskuryakov]

Confirmed with r84740, not a regression from Safari 5.0.5.

[Mihnea Ovidenie]

If the first div (the one with the shadow) is put in a separate layer (-webkit-transform: translateZ(0);), the problem cannot be reproduced.

[Andrei Bucur]

Created attachment 93766 [details]
Patch and layout test for the bug

The patch adds a new method to RenderStyle used to query the extent of an inset box-shadow. The extent is then used in RenderObject::repaintAfterLayoutIfNeeded to adjust the repaint rectangle size to include the shadow. The attached pixel layout test checks if this behavior is available.

[Simon Fraser (smfr)]

Comment on attachment 93766 [details]
Patch and layout test for the bug

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

> LayoutTests/fast/repaint/box-shadow-inset-repaint.html:16
> +				var dumpPixels = true;
> +				layoutTestController.dumpAsText(dumpPixels);
> +				layoutTestController.waitUntilDone();

This should be repaint test (see other tests in this dir).

[Andrei Bucur]

Created attachment 93772 [details]
Patch and new layout test

Fix for the previous layout test to match the other repaint tests.

[Andrei Bucur]

Thanks for pointing this out. I initially placed the test in the fast/box-shadow folder, then moved it to fast/repaint, but I haven't checked how those tests were designed.

Comment on attachment 93772 [details]
Patch and new layout test

This shifts the repaint rectangles. Shouldn't it make them larger?

[WebKit Review Bot]

Comment on attachment 93772 [details]
Patch and new layout test

Attachment 93772 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8706007

New failing tests:
fast/repaint/box-shadow-inset-repaint.html

[WebKit Review Bot]

Created attachment 93778 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Andrei Bucur]

Created attachment 93781 [details]
Third patch and test attempt

The previous patch was shifting the repaint rectangles, not resizing it as it was supposed to do.

[WebKit Review Bot]

Comment on attachment 93781 [details]
Third patch and test attempt

Attachment 93781 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8705398

New failing tests:
fast/repaint/box-shadow-inset-repaint.html

[WebKit Review Bot]

Created attachment 93789 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Andrei Bucur]

Could some Chromium folks give me some guidance on what's the best solution to make the chromium EWS bot pass the test? I don't think that setting it to FAIL in test_expectations.txt is the desired option.
Thanks!

[Hajime Morrita]

> test_expectations.txt is the desired option
Not desired, but reasonable if you file a bug for it.
Please CC me on the bug so I can update expectations for it.
Thanks for caring Chromium port :-)

[Andrei Bucur]

Created attachment 95339 [details]
Patch without text in the test

The commit bot failure seems to have been caused by different text size computation for the Chromium port. Because this is a repaint test for box-shadow, a reduction to remove the unnecessary text could be the appropriate solution.

[Darin Adler]

Comment on attachment 95339 [details]
Patch without text in the test

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

> Source/WebCore/rendering/RenderObject.cpp:1300
> +    // insetShadowTop and insetShadowLeft are not used because the box didn't change its location

Needs a period.

Confusing comment because it describes the code below, not this code. I think instead of "are not used" you could say "will not be used below".

Or you could leave the comment out entirely.

> Source/WebCore/rendering/style/RenderStyle.h:1337
> +    void getShadowInsetExtent(const ShadowData*, int &top, int &right, int &bottom, int &left) const;

Same formatting problem. Notice the mismatch with the next line of code.

[Eric Seidel (no email)]

Comment on attachment 95339 [details]
Patch without text in the test

Turning darin's comments into a r-, since Andrei is not a committer.

[Andrei Bucur]

Created attachment 143767 [details]
Updated patch and test

Reviving this patch, it has grown too old :).

[WebKit Review Bot]

Comment on attachment 143767 [details]
Updated patch and test

Attachment 143767 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12804113

New failing tests:
fast/repaint/box-shadow-inset-repaint.html

[WebKit Review Bot]

Created attachment 143782 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Simon Fraser (smfr)]

Comment on attachment 143767 [details]
Updated patch and test

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

> LayoutTests/ChangeLog:3
> +        Incomplete repaint with box-shadow and padding when Javascript updates document

"when Javascript updates document" is too vague. The bug title and change log should be changed to be more specific (what's being updated?)

> LayoutTests/fast/repaint/box-shadow-inset-repaint.html:14
> +		document.getElementById('container').innerHTML = "<div style='width:100px; height:100px; background-color:green'></div>";

I suspect the bug could be reproduced just by changing style, rather than setting innerHTML. This would result in a cleaner test case.

> Source/WebCore/rendering/RenderObject.cpp:1446
> +        LayoutUnit borderWidth = max<LayoutUnit>(-outlineStyle->outlineOffset(), max<LayoutUnit>(borderRight, max<LayoutUnit>(valueForLength(style()->borderTopRightRadius().width(), boxWidth, v), valueForLength(style()->borderBottomRightRadius().width(), boxWidth, v)))) + max<LayoutUnit>(ow, shadowRight) - insetShadowRight;

This line is incredibly hard to read. Maybe break it somehow, or use local variables.

> Source/WebCore/rendering/style/RenderStyle.cpp:1178
> +void RenderStyle::getShadowInsetExtent(const ShadowData* shadow, LayoutUnit &top, LayoutUnit &right, LayoutUnit &bottom, LayoutUnit &left) const
> +{

FractionalLayoutBoxExtent was added in 118076. Perhaps that should be used here.

[Andrei Bucur]

(In reply to comment #21)
> (From update of attachment 143767 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143767&action=review
> 
> > LayoutTests/ChangeLog:3
> > +        Incomplete repaint with box-shadow and padding when Javascript updates document
> 
> "when Javascript updates document" is too vague. The bug title and change log should be changed to be more specific (what's being updated?)
Will do.

> 
> > LayoutTests/fast/repaint/box-shadow-inset-repaint.html:14
> > +		document.getElementById('container').innerHTML = "<div style='width:100px; height:100px; background-color:green'></div>";
> 
> I suspect the bug could be reproduced just by changing style, rather than setting innerHTML. This would result in a cleaner test case.
I remember trying to use style change a few months back, when I started looking into this. The bug stopped reproducing because the style diff would've triggered a full repaint. This seemed like the simplest way to do it.

> 
> > Source/WebCore/rendering/RenderObject.cpp:1446
> > +        LayoutUnit borderWidth = max<LayoutUnit>(-outlineStyle->outlineOffset(), max<LayoutUnit>(borderRight, max<LayoutUnit>(valueForLength(style()->borderTopRightRadius().width(), boxWidth, v), valueForLength(style()->borderBottomRightRadius().width(), boxWidth, v)))) + max<LayoutUnit>(ow, shadowRight) - insetShadowRight;
> 
> This line is incredibly hard to read. Maybe break it somehow, or use local variables.
Will do. While braking this out, I think I've noticed some possible improvements to the rectangle size.

> 
> > Source/WebCore/rendering/style/RenderStyle.cpp:1178
> > +void RenderStyle::getShadowInsetExtent(const ShadowData* shadow, LayoutUnit &top, LayoutUnit &right, LayoutUnit &bottom, LayoutUnit &left) const
> > +{
> 
> FractionalLayoutBoxExtent was added in 118076. Perhaps that should be used here.
What should I do with the other similar methods in RenderStyle (e.g. getShadowExtent)? Should I rewrite those as well?

[Simon Fraser (smfr)]

(In reply to comment #22)
> (In reply to comment #21)
> > FractionalLayoutBoxExtent was added in 118076. Perhaps that should be used here.

> What should I do with the other similar methods in RenderStyle (e.g. getShadowExtent)? Should I rewrite those as well?

That would be best done in a separate patch.

[Andrei Bucur]

Created attachment 165615 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 165615 [details]
Patch

Judging from the Mac results, are we overpainting the top?

[WebKit Review Bot]

Comment on attachment 165615 [details]
Patch

Attachment 165615 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14026158

New failing tests:
fast/repaint/box-shadow-inset-repaint.html

[WebKit Review Bot]

Comment on attachment 165615 [details]
Patch

Attachment 165615 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14036118

New failing tests:
fast/repaint/box-shadow-inset-repaint.html

[Andrei Bucur]

(In reply to comment #25)
> (From update of attachment 165615 [details])
> Judging from the Mac results, are we overpainting the top?

Yes, this is true most likely. It happens because the container is initially 30px tall because of the padding and then expanded to include the green div.
The inset shadow is initially larger (60px) than the container, but it gets clipped inside the padding box. When computing the new repaint rectangle the fact that the shadow was only 30px, not 60px, is ignored.
I can try to optimise the math to use the exact values based on the old and new rectangles, but I'm not sure it's worth the code complexity. Another option is just adding a check in mustRepaintBackgroundOrBorder() to look for inset shadows and trigger a full repaint. Simon, Eric, what do you think?

[Simon Fraser (smfr)]

(In reply to comment #28)
> (In reply to comment #25)
> > (From update of attachment 165615 [details] [details])
> > Judging from the Mac results, are we overpainting the top?
> 
> Yes, this is true most likely. It happens because the container is initially 30px tall because of the padding and then expanded to include the green div.
> The inset shadow is initially larger (60px) than the container, but it gets clipped inside the padding box. When computing the new repaint rectangle the fact that the shadow was only 30px, not 60px, is ignored.
> I can try to optimise the math to use the exact values based on the old and new rectangles, but I'm not sure it's worth the code complexity. Another option is just adding a check in mustRepaintBackgroundOrBorder() to look for inset shadows and trigger a full repaint. Simon, Eric, what do you think?

Probably OK, but do we use inset shadows for text fields? We would not want to full-repaint those.

[Andrei Bucur]

Created attachment 166679 [details]
Patch

[Andrei Bucur]

(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #25)
> > > (From update of attachment 165615 [details] [details] [details])
> > > Judging from the Mac results, are we overpainting the top?
> > 
> > Yes, this is true most likely. It happens because the container is initially 30px tall because of the padding and then expanded to include the green div.
> > The inset shadow is initially larger (60px) than the container, but it gets clipped inside the padding box. When computing the new repaint rectangle the fact that the shadow was only 30px, not 60px, is ignored.
> > I can try to optimise the math to use the exact values based on the old and new rectangles, but I'm not sure it's worth the code complexity. Another option is just adding a check in mustRepaintBackgroundOrBorder() to look for inset shadows and trigger a full repaint. Simon, Eric, what do you think?
> 
> Probably OK, but do we use inset shadows for text fields? We would not want to full-repaint those.

In the end I've used a better formula for the decorations width. When repainting we are interested only in the size of the inset shadow for the minimum bounds rect. The other inset shadow (that can be bigger for the larger bounds rect because it's not clipped/not clipped as much) is covered by the delta width/height value (because the inset shadow is inside the content box).

[WebKit Review Bot]

Comment on attachment 166679 [details]
Patch

Rejecting attachment 166679 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
rce/patched-yasm --revision 154708 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
51>At revision 154708.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...
Total errors found: 0 in 1 files

Full output: http://queues.webkit.org/results/14124477

[Andrei Bucur]

Created attachment 167270 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 167270 [details]
Patch for landing

Clearing flags on attachment: 167270

Committed r130489: <http://trac.webkit.org/changeset/130489>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.