| Summary: | Incomplete repaint of boxes with inset box-shadow and padding when resized | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tushar Pokle <tushar.pokle> | ||||||||||||||||||||||||||
| Component: | Layout and Rendering | Assignee: | Andrei Bucur <abucur> | ||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
| Severity: | Normal | CC: | abucur, dglazkov, eric, mihnea, mitz, morrita, robert, simon.fraser, webkit.review.bot | ||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||
| OS: | OS X 10.6 | ||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||
Confirmed with r84740, not a regression from Safari 5.0.5. If the first div (the one with the shadow) is put in a separate layer (-webkit-transform: translateZ(0);), the problem cannot be reproduced. 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 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). Created attachment 93772 [details]
Patch and new layout test
Fix for the previous layout test to match the other repaint tests.
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?
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 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
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 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 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
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! > 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 :-)
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 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 on attachment 95339 [details]
Patch without text in the test
Turning darin's comments into a r-, since Andrei is not a committer.
Created attachment 143767 [details]
Updated patch and test
Reviving this patch, it has grown too old :).
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 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 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. (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? (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. Created attachment 165615 [details]
Patch
Comment on attachment 165615 [details]
Patch
Judging from the Mac results, are we overpainting the top?
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 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 (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? (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. Created attachment 166679 [details]
Patch
(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 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 Created attachment 167270 [details]
Patch for landing
Comment on attachment 167270 [details] Patch for landing Clearing flags on attachment: 167270 Committed r130489: <http://trac.webkit.org/changeset/130489> All reviewed patches have been landed. Closing bug. |
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.