Summary: | Setting overflow:hidden does not always repaint clipped content. | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Luis de Bethencourt <luis> | ||||||||||||||||||||||||
Component: | Layout and Rendering | Assignee: | zalan <zalan> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | aestes, cbiesinger, commit-queue, esprehn+autocc, glenn, gustavo, kondapallykalyan, robert, simon.fraser, stavila, yuki.sekiguchi, zalan | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||
Bug Depends on: | 117741 | ||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||
Attachments: |
|
Description
Luis de Bethencourt
2013-05-29 13:47:21 PDT
This seems to be a repaint issue overflow and position (nothing to do with inlines). Simon, have you looked at the links? Unless I'm missunderstanding things this overflow issue is just an example of undrawn propagated style changes. Correct me if I'm wrong. Unfortunately the links have code snippet but not a full example to attach here. I looked at the links. It's quite possible that every link is talking about a different bug; unless we get specific reports with tests, it's hard to generalize. I understand, I will try to write more tests. Any idea why the overflow and position bug happens? Created attachment 203434 [details]
Patch
As Simon says, we don't have general test case, so we cannot fix all bugs. I try to fix a bug of this simple testcase. I think a cause of this bug is very rare condition, so my patch doen't fix many issues. If you find another repaint problem, please file another bug. Comment on attachment 203434 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203434&action=review Seems like a nice catch! > Source/WebCore/rendering/RenderBox.cpp:2036 > + if (!hasSelfPaintingLayer() && o->hasOverflowClip()) { This pattern pops up in a couple of other places too. Why not make a helper called shouldUseOverflowClip() or similar - that would also be a good place to add a comment about why we only need to worry about clipping on layers that aren't self-painting, something from the Changelog in r41203 for example. I say this because I had to dig around a bit to find out for myself. You also need to fix this function in RenderObject. Comment on attachment 203434 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203434&action=review > Source/WebCore/rendering/RenderBox.cpp:2034 > + // If this RenderBox has self painting layer, visial overflow clip of "o" doesn't contain a rect of the RenderBox. "visial" -> "visual" > Source/WebCore/rendering/RenderBox.cpp:2035 > + // Clipping this painting rect make the rect narrower. Not sure what you mean here. Created attachment 203661 [details]
Patch
Hi Robert, (In reply to comment #7) > (From update of attachment 203434 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=203434&action=review > > Seems like a nice catch! > > > Source/WebCore/rendering/RenderBox.cpp:2036 > > + if (!hasSelfPaintingLayer() && o->hasOverflowClip()) { > > This pattern pops up in a couple of other places too. Why not make a helper called shouldUseOverflowClip() or similar - that would also be a good place to add a comment about why we only need to worry about clipping on layers that aren't self-painting, something from the Changelog in r41203 for example. I say this because I had to dig around a bit to find out for myself. > > You also need to fix this function in RenderObject. I found two method using this pattern, but all of them (including my patch) check different conditions. * RenderBlock::nodeAtPoint() want probability that children overflow, so it only checks whether it clips children or not. * RenderBox::addOverflowFromChild() want to know it should add a child to its visual overflow, so it checks that it has overflow clip (never overflow) and it should contain the child as visual overflow(if the child have self painting layer, it never overflow). * My patch checks that parent contains it as visual overflow. Yes. We can rewrite addOverflowFromChild() to the following: > if ((!child->hasSelfPaintingLayer() && hasOverflowClip()) || child->hasSelfPaintingLayer()) Then, we can refactor the code to use helper function. However, I think original code is more clear than refactored code. Hi Simon, (In reply to comment #8) > (From update of attachment 203434 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=203434&action=review > > > Source/WebCore/rendering/RenderBox.cpp:2034 > > + // If this RenderBox has self painting layer, visial overflow clip of "o" doesn't contain a rect of the RenderBox. > > "visial" -> "visual" Fixed. > > > Source/WebCore/rendering/RenderBox.cpp:2035 > > + // Clipping this painting rect make the rect narrower. > > Not sure what you mean here. Changed a sentence. (In reply to comment #10) > Hi Robert, > > (In reply to comment #7) > > (From update of attachment 203434 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=203434&action=review > > > > Seems like a nice catch! > > > > > Source/WebCore/rendering/RenderBox.cpp:2036 > > > + if (!hasSelfPaintingLayer() && o->hasOverflowClip()) { > > > > This pattern pops up in a couple of other places too. Why not make a helper called shouldUseOverflowClip() or similar - that would also be a good place to add a comment about why we only need to worry about clipping on layers that aren't self-painting, something from the Changelog in r41203 for example. I say this because I had to dig around a bit to find out for myself. > > > > You also need to fix this function in RenderObject. > > I found two method using this pattern, but all of them (including my patch) check different conditions. > * RenderBlock::nodeAtPoint() want probability that children overflow, so it only checks whether it clips children or not. > * RenderBox::addOverflowFromChild() want to know it should add a child to its visual overflow, so it checks that it has overflow clip (never overflow) and it should contain the child as visual overflow(if the child have self painting layer, it never overflow). > * My patch checks that parent contains it as visual overflow. > > Yes. We can rewrite addOverflowFromChild() to the following: > > if ((!child->hasSelfPaintingLayer() && hasOverflowClip()) || child->hasSelfPaintingLayer()) > Then, we can refactor the code to use helper function. > However, I think original code is more clear than refactored code. I forgot to added the following... However, making condition more clear using method is good! I added the method. Comment on attachment 203661 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203661&action=review > Source/WebCore/ChangeLog:18 > + In the reproduced content, overflow:hidden parent(called P) has display:none replaced child(called C). > + If we hover a element, P becomes overflow:visible, and C becomes display:block. > + If we mouse out from the element, P becomes hidden, and C becomes none, and part of C is not repainted. > + > + When we mouse out from the element, P and C request repaint. > + Before requesting repaint, P becomes overflow: hidden, and it makes hasOverflowClip() true. > + Requesting repaint from C is clipped by P at RenderBox::computeRectForRepaint(). > + The rect which P requests repaint is content rect of P, so the rect which is overflowed from P is not requested. > + > + If C is not relative, visual overflow rect of P has the rect of C, so we can repaint well. > + If C doesn't become display:none, layouting C requests repaint the rect of C, so we can repaint well. I don't think this long explanation is necessary. > Source/WebCore/rendering/RenderLayerModelObject.cpp:82 > + return !hasSelfPaintingLayer() && container->hasOverflowClip(); You should ASSERT that o is the container. > LayoutTests/fast/repaint/change-overflow-and-display-of-relative-expected.html:2 > +<html><head><meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> Don't need the <meta> tag. > LayoutTests/fast/repaint/change-overflow-and-display-of-relative.html:32 > + }, 50); Can this be < 50ms? Created attachment 204133 [details]
Patch
(In reply to comment #13) > (From update of attachment 203661 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=203661&action=review > > > Source/WebCore/ChangeLog:18 > > + In the reproduced content, overflow:hidden parent(called P) has display:none replaced child(called C). > > + If we hover a element, P becomes overflow:visible, and C becomes display:block. > > + If we mouse out from the element, P becomes hidden, and C becomes none, and part of C is not repainted. > > + > > + When we mouse out from the element, P and C request repaint. > > + Before requesting repaint, P becomes overflow: hidden, and it makes hasOverflowClip() true. > > + Requesting repaint from C is clipped by P at RenderBox::computeRectForRepaint(). > > + The rect which P requests repaint is content rect of P, so the rect which is overflowed from P is not requested. > > + > > + If C is not relative, visual overflow rect of P has the rect of C, so we can repaint well. > > + If C doesn't become display:none, layouting C requests repaint the rect of C, so we can repaint well. > > I don't think this long explanation is necessary. Removed. > > > Source/WebCore/rendering/RenderLayerModelObject.cpp:82 > > + return !hasSelfPaintingLayer() && container->hasOverflowClip(); > > You should ASSERT that o is the container. Added. > > > LayoutTests/fast/repaint/change-overflow-and-display-of-relative-expected.html:2 > > +<html><head><meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> > > Don't need the <meta> tag. Removed, and added closing tag of head. > > > LayoutTests/fast/repaint/change-overflow-and-display-of-relative.html:32 > > + }, 50); > > Can this be < 50ms? No, this can't. I tried 0ms and used "overflow.offsetHeight;" to force to redraw. However, if I open this test from Finder, the problem is sometimes not reproduced. I think 1st drawing is combined with 2nd drawing. (In reply to comment #15) > > > > > LayoutTests/fast/repaint/change-overflow-and-display-of-relative.html:32 > > > + }, 50); > > > > Can this be < 50ms? > > No, this can't. > I tried 0ms and used "overflow.offsetHeight;" to force to redraw. > However, if I open this test from Finder, the problem is sometimes not reproduced. > I think 1st drawing is combined with 2nd drawing. If it's this timing sensitive on your machine, it's likely it will fail on a bot, or EWS. You should try to find a way to make the test reliable with a setTimeout. Does testRunner.display() help? Thinking about the test some more, I think it should not be a ref test, and instead be a repaint test. See tests that call repaintRectsAsText. Created attachment 204286 [details]
Patch
Hi Simon, (In reply to comment #16) > (In reply to comment #15) > > > > > > > LayoutTests/fast/repaint/change-overflow-and-display-of-relative.html:32 > > > > + }, 50); > > > > > > Can this be < 50ms? > > > > No, this can't. > > I tried 0ms and used "overflow.offsetHeight;" to force to redraw. > > However, if I open this test from Finder, the problem is sometimes not reproduced. > > I think 1st drawing is combined with 2nd drawing. > > If it's this timing sensitive on your machine, it's likely it will fail on a bot, or EWS. You should try to find a way to make the test reliable with a setTimeout. Does testRunner.display() help? > > Thinking about the test some more, I think it should not be a ref test, and instead be a repaint test. See tests that call repaintRectsAsText. testRunner.display() doesn't work well for pixel and ref test, but it works well for text based repaint test. Therefore, I changed the test to use repaintRectsAsText for DumpRenderTree and setTimeout() for Safari. Could you review this? Comment on attachment 204286 [details] Patch Clearing flags on attachment: 204286 Committed r151549: <http://trac.webkit.org/changeset/151549> All reviewed patches have been landed. Closing bug. This broke selection painting in the web inspector source view. I'm going to revert it. Re-opened since this is blocked by bug 117741 Created attachment 204963 [details]
Narrowed content for WebInspector selection problem
Thank you for reverting. I created narrowed content for WebInspector selection problem. I'll investigate it and fix it. Created attachment 205060 [details]
Patch
The previous patch also broke beta.icloud.com, bug 117743. Could you please test that, too? And bug 117739, too. Hi Alexey, (In reply to comment #26) > The previous patch also broke beta.icloud.com, bug 117743. Could you please test that, too? Hmm.. I could not sign in to beta.icloud.com. It says "Developer Account Required" and "Using beta.icloud.com requires a developer Apple ID that has been set up on the latest version of iOS or OS X." I use iCloud on MacOS X 10.8.4 and iOS 6.1.3 and have free developer account. i.e. I can download old Xcode. Do I need iOS 7 or paid developer account to use beta.icloud.com ? I could sign in beta.icloud.com and reproduced those bugs on old Safari. I confirmed that new patch fixed those bugs. Comment on attachment 205060 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205060&action=review > Source/WebCore/rendering/RenderLayerModelObject.cpp:80 > +bool RenderLayerModelObject::shouldUseClipForRepaint(RenderObject* container) const this->container() is RenderElement, so the paramater passed to this function should also be RenderElemenet. Comment on attachment 205060 [details]
Patch
Assuming that patches for review since 2013 are stale, r-
Created attachment 279779 [details]
Test reduction
Created attachment 279780 [details]
Test reduction
Created attachment 279802 [details]
Patch
Comment on attachment 279802 [details]
Patch
r=me, but I think you need to explain why this code is necessary. It's because repaints issued by the removal of descendants before layout() gets called to updateLayerPositions (which would have issued the right repaints) use the updated style and so clip when they shouldn't. This is why overflow changes have to result in immediate repaints of the entire layout overflow area.
Created attachment 279819 [details]
Patch
Comment on attachment 279819 [details] Patch Clearing flags on attachment: 279819 Committed r201407: <http://trac.webkit.org/changeset/201407> All reviewed patches have been landed. Closing bug. |