Bug 116994

Summary: Setting overflow:hidden does not always repaint clipped content.
Product: WebKit Reporter: Luis de Bethencourt <luis>
Component: Layout and RenderingAssignee: 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 Flags
simple testcase
none
Patch
none
Patch
none
Patch
none
Patch
none
Narrowed content for WebInspector selection problem
none
Patch
none
Test reduction
none
Test reduction
none
Patch
none
Patch none

Description Luis de Bethencourt 2013-05-29 13:47:21 PDT
Created attachment 203271 [details]
simple testcase

When updating an inline element, the browser does not redraw the screen until a block-level change happens in the DOM, frame is scrolled or window is resized. I have searched around online and seen many web developers having this problem and forcing a refresh to work around it. By changing the zoom level to 1.1 and back to 1.0, by using eloffsetHeight in the element, or with el.hide().show().

I have added a simple testcase to easily reproduce the bug.

This has been happening for some time, as can be seen in the following pages:
http://stackoverflow.com/questions/3485365/how-can-i-force-webkit-to-redraw-repaint-to-propagate-style-changes
http://mir.aculo.us/2009/09/25/force-redraw-dom-technique-for-webkit-based-browsers/
https://gist.github.com/madrobby/1362093
Comment 1 Simon Fraser (smfr) 2013-05-29 14:16:15 PDT
This seems to be a repaint issue overflow and position (nothing to do with inlines).
Comment 2 Luis de Bethencourt 2013-05-29 14:19:53 PDT
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.
Comment 3 Simon Fraser (smfr) 2013-05-29 21:00:03 PDT
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.
Comment 4 Luis de Bethencourt 2013-05-30 08:09:33 PDT
I understand, I will try to write more tests.

Any idea why the overflow and position bug happens?
Comment 5 Yuki Sekiguchi 2013-05-31 04:37:16 PDT
Created attachment 203434 [details]
Patch
Comment 6 Yuki Sekiguchi 2013-05-31 04:42:44 PDT
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 7 Robert Hogan 2013-06-02 02:51:52 PDT
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 8 Simon Fraser (smfr) 2013-06-02 09:27:43 PDT
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.
Comment 9 Yuki Sekiguchi 2013-06-04 00:47:40 PDT
Created attachment 203661 [details]
Patch
Comment 10 Yuki Sekiguchi 2013-06-04 05:26:46 PDT
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.
Comment 11 Yuki Sekiguchi 2013-06-04 05:27:48 PDT
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.
Comment 12 Yuki Sekiguchi 2013-06-04 05:35:08 PDT
(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 13 Simon Fraser (smfr) 2013-06-06 17:27:33 PDT
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?
Comment 14 Yuki Sekiguchi 2013-06-09 20:20:10 PDT
Created attachment 204133 [details]
Patch
Comment 15 Yuki Sekiguchi 2013-06-09 20:28:09 PDT
(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.
Comment 16 Simon Fraser (smfr) 2013-06-09 22:05:17 PDT
(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.
Comment 17 Yuki Sekiguchi 2013-06-10 22:19:59 PDT
Created attachment 204286 [details]
Patch
Comment 18 Yuki Sekiguchi 2013-06-11 04:20:07 PDT
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 19 WebKit Commit Bot 2013-06-13 08:37:20 PDT
Comment on attachment 204286 [details]
Patch

Clearing flags on attachment: 204286

Committed r151549: <http://trac.webkit.org/changeset/151549>
Comment 20 WebKit Commit Bot 2013-06-13 08:37:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Simon Fraser (smfr) 2013-06-18 10:31:24 PDT
This broke selection painting in the web inspector source view. I'm going to revert it.
Comment 22 WebKit Commit Bot 2013-06-18 10:33:24 PDT
Re-opened since this is blocked by bug 117741
Comment 23 Yuki Sekiguchi 2013-06-18 19:51:50 PDT
Created attachment 204963 [details]
Narrowed content for WebInspector selection problem
Comment 24 Yuki Sekiguchi 2013-06-18 19:52:56 PDT
Thank you for reverting.
I created narrowed content for WebInspector selection problem.
I'll investigate it and fix it.
Comment 25 Yuki Sekiguchi 2013-06-19 22:49:18 PDT
Created attachment 205060 [details]
Patch
Comment 26 Alexey Proskuryakov 2013-06-20 10:46:02 PDT
The previous patch also broke beta.icloud.com, bug 117743. Could you please test that, too?
Comment 27 Alexey Proskuryakov 2013-06-20 10:50:14 PDT
And bug 117739, too.
Comment 28 Yuki Sekiguchi 2013-06-20 20:07:02 PDT
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 ?
Comment 29 Yuki Sekiguchi 2013-07-01 02:13:49 PDT
I could sign in beta.icloud.com and reproduced those bugs on old Safari.
I confirmed that new patch fixed those bugs.
Comment 30 Radu Stavila 2014-05-29 04:38:52 PDT
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 31 Brady Eidson 2016-05-24 22:01:06 PDT
Comment on attachment 205060 [details]
Patch

Assuming that patches for review since 2013 are stale, r-
Comment 32 zalan 2016-05-25 09:47:47 PDT
Created attachment 279779 [details]
Test reduction
Comment 33 zalan 2016-05-25 09:49:07 PDT
Created attachment 279780 [details]
Test reduction
Comment 34 zalan 2016-05-25 13:32:12 PDT
rdar://problem/26476697
Comment 35 zalan 2016-05-25 13:37:10 PDT
Created attachment 279802 [details]
Patch
Comment 36 Dave Hyatt 2016-05-25 13:50:29 PDT
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.
Comment 37 zalan 2016-05-25 14:45:56 PDT
Created attachment 279819 [details]
Patch
Comment 38 WebKit Commit Bot 2016-05-25 16:00:42 PDT
Comment on attachment 279819 [details]
Patch

Clearing flags on attachment: 279819

Committed r201407: <http://trac.webkit.org/changeset/201407>
Comment 39 WebKit Commit Bot 2016-05-25 16:00:50 PDT
All reviewed patches have been landed.  Closing bug.