Bug 59863 - Incomplete repaint of boxes with inset box-shadow and padding when resized
Summary: Incomplete repaint of boxes with inset box-shadow and padding when resized
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.6
: P2 Normal
Assignee: Andrei Bucur
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-30 04:01 PDT by Tushar Pokle
Modified: 2012-10-05 02:09 PDT (History)
9 users (show)

See Also:


Attachments
Open in webkit and wait half a second to see the artifacts appear (526 bytes, text/html)
2011-04-30 04:01 PDT, Tushar Pokle
no flags Details
Patch and layout test for the bug (24.44 KB, patch)
2011-05-17 08:09 PDT, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch and new layout test (29.87 KB, patch)
2011-05-17 08:50 PDT, Andrei Bucur
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.18 MB, application/zip)
2011-05-17 09:27 PDT, WebKit Review Bot
no flags Details
Third patch and test attempt (30.07 KB, patch)
2011-05-17 09:49 PDT, Andrei Bucur
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.23 MB, application/zip)
2011-05-17 10:27 PDT, WebKit Review Bot
no flags Details
Patch without text in the test (24.99 KB, patch)
2011-05-30 05:49 PDT, Andrei Bucur
eric: review-
Details | Formatted Diff | Diff
Updated patch and test (9.80 KB, patch)
2012-05-24 02:15 PDT, Andrei Bucur
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (479.08 KB, application/zip)
2012-05-24 04:28 PDT, WebKit Review Bot
no flags Details
Patch (13.88 KB, patch)
2012-09-25 08:08 PDT, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch (15.69 KB, patch)
2012-10-02 05:55 PDT, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch for landing (15.68 KB, patch)
2012-10-05 00:34 PDT, Andrei Bucur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tushar Pokle 2011-04-30 04:01:27 PDT
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.
Comment 1 Alexey Proskuryakov 2011-04-30 15:51:50 PDT
Confirmed with r84740, not a regression from Safari 5.0.5.
Comment 2 Mihnea Ovidenie 2011-05-11 06:47:16 PDT
If the first div (the one with the shadow) is put in a separate layer (-webkit-transform: translateZ(0);), the problem cannot be reproduced.
Comment 3 Andrei Bucur 2011-05-17 08:09:13 PDT
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.
Comment 4 Simon Fraser (smfr) 2011-05-17 08:19:49 PDT
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).
Comment 5 Andrei Bucur 2011-05-17 08:50:52 PDT
Created attachment 93772 [details]
Patch and new layout test

Fix for the previous layout test to match the other repaint tests.
Comment 6 Andrei Bucur 2011-05-17 08:53:07 PDT
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 7 mitz 2011-05-17 09:00:50 PDT
Comment on attachment 93772 [details]
Patch and new layout test

This shifts the repaint rectangles. Shouldn't it make them larger?
Comment 8 WebKit Review Bot 2011-05-17 09:26:58 PDT
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
Comment 9 WebKit Review Bot 2011-05-17 09:27:03 PDT
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
Comment 10 Andrei Bucur 2011-05-17 09:49:19 PDT
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.
Comment 11 WebKit Review Bot 2011-05-17 10:27:21 PDT
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
Comment 12 WebKit Review Bot 2011-05-17 10:27:26 PDT
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
Comment 13 Andrei Bucur 2011-05-20 04:30:26 PDT
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!
Comment 14 Hajime Morrita 2011-05-24 21:06:05 PDT
> 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 :-)
Comment 15 Andrei Bucur 2011-05-30 05:49:01 PDT
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.
Comment 16 Darin Adler 2011-10-17 12:16:37 PDT
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.
Comment 17 Eric Seidel (no email) 2011-12-21 15:27:58 PST
Comment on attachment 95339 [details]
Patch without text in the test

Turning darin's comments into a r-, since Andrei is not a committer.
Comment 18 Andrei Bucur 2012-05-24 02:15:24 PDT
Created attachment 143767 [details]
Updated patch and test

Reviving this patch, it has grown too old :).
Comment 19 WebKit Review Bot 2012-05-24 04:28:19 PDT
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
Comment 20 WebKit Review Bot 2012-05-24 04:28:23 PDT
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
Comment 21 Simon Fraser (smfr) 2012-05-24 08:32:17 PDT
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.
Comment 22 Andrei Bucur 2012-05-28 04:52:56 PDT
(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?
Comment 23 Simon Fraser (smfr) 2012-05-28 21:37:33 PDT
(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.
Comment 24 Andrei Bucur 2012-09-25 08:08:21 PDT
Created attachment 165615 [details]
Patch
Comment 25 Eric Seidel (no email) 2012-09-25 08:15:22 PDT
Comment on attachment 165615 [details]
Patch

Judging from the Mac results, are we overpainting the top?
Comment 26 WebKit Review Bot 2012-09-25 10:24:45 PDT
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
Comment 27 WebKit Review Bot 2012-09-25 11:27:01 PDT
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
Comment 28 Andrei Bucur 2012-09-26 04:23:02 PDT
(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?
Comment 29 Simon Fraser (smfr) 2012-10-01 11:00:56 PDT
(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.
Comment 30 Andrei Bucur 2012-10-02 05:55:46 PDT
Created attachment 166679 [details]
Patch
Comment 31 Andrei Bucur 2012-10-02 06:58:19 PDT
(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).
Comment 32 WebKit Review Bot 2012-10-02 12:16:14 PDT
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
Comment 33 Andrei Bucur 2012-10-05 00:34:36 PDT
Created attachment 167270 [details]
Patch for landing
Comment 34 WebKit Review Bot 2012-10-05 02:09:19 PDT
Comment on attachment 167270 [details]
Patch for landing

Clearing flags on attachment: 167270

Committed r130489: <http://trac.webkit.org/changeset/130489>
Comment 35 WebKit Review Bot 2012-10-05 02:09:25 PDT
All reviewed patches have been landed.  Closing bug.