Bug 116994 – Setting overflow:hidden does not always repaint clipped content.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Luis de Bethencourt]

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

[Simon Fraser (smfr)]

This seems to be a repaint issue overflow and position (nothing to do with inlines).

[Luis de Bethencourt]

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.

[Simon Fraser (smfr)]

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.

[Luis de Bethencourt]

I understand, I will try to write more tests.

Any idea why the overflow and position bug happens?

[Yuki Sekiguchi]

Created attachment 203434 [details]
Patch

[Yuki Sekiguchi]

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.

[Robert Hogan]

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.

[Simon Fraser (smfr)]

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.

[Yuki Sekiguchi]

Created attachment 203661 [details]
Patch

[Yuki Sekiguchi]

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.

[Yuki Sekiguchi]

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.

[Yuki Sekiguchi]

(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.

[Simon Fraser (smfr)]

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?

[Yuki Sekiguchi]

Created attachment 204133 [details]
Patch

[Yuki Sekiguchi]

(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.

[Simon Fraser (smfr)]

(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.

[Yuki Sekiguchi]

Created attachment 204286 [details]
Patch

[Yuki Sekiguchi]

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?

[WebKit Commit Bot]

Comment on attachment 204286 [details]
Patch

Clearing flags on attachment: 204286

Committed r151549: <http://trac.webkit.org/changeset/151549>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This broke selection painting in the web inspector source view. I'm going to revert it.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 117741

[Yuki Sekiguchi]

Created attachment 204963 [details]
Narrowed content for WebInspector selection problem

[Yuki Sekiguchi]

Thank you for reverting.
I created narrowed content for WebInspector selection problem.
I'll investigate it and fix it.

[Yuki Sekiguchi]

Created attachment 205060 [details]
Patch

[Alexey Proskuryakov]

The previous patch also broke beta.icloud.com, bug 117743. Could you please test that, too?

[Alexey Proskuryakov]

And bug 117739, too.

[Yuki Sekiguchi]

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 ?

[Yuki Sekiguchi]

I could sign in beta.icloud.com and reproduced those bugs on old Safari.
I confirmed that new patch fixed those bugs.

[Radu Stavila]

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.

[Brady Eidson]

Comment on attachment 205060 [details]
Patch

Assuming that patches for review since 2013 are stale, r-

[zalan]

Created attachment 279779 [details]
Test reduction

[zalan]

Created attachment 279780 [details]
Test reduction

[zalan]

rdar://problem/26476697

[zalan]

Created attachment 279802 [details]
Patch

[Dave Hyatt]

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.

[zalan]

Created attachment 279819 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 279819 [details]
Patch

Clearing flags on attachment: 279819

Committed r201407: <http://trac.webkit.org/changeset/201407>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.