Bug 82990 – REGRESSION (r111456): overflow-scrolling and css clip break with compositing

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Created attachment 135248 [details]
Testcase

The testcase I'll attach shows a bug that started to happen in r111456. The testcase is setting CSS clip in response to a scroll event, and no longer works after that revision.

<rdar://problem/11162020>

[Adrienne Walker]

Not knowing anything about style differences, I wonder if setting the clip style should really force layout and not just a repaint.

I suspect previously the render layer was just incorrectly not being considered clipped and considering more layers for clipping has unearthed this bug.

Sorry for not looking at this sooner, smfr.  I've pretty busy, and it's hard to tell how urgent something is to fix when I can't see rdar links.

[Adrienne Walker]

Ok, I see what the problem is.

Along with considering overflow clip from non-compositing render layers, r111456 also started considering clip.  This test case worked before this patch, because the layer wasn't clipped at all.

In this test case, a scroll handler updates the clip.  When scrolling occurs, the render layer tree is updated (with the old clip) via updateCompositingLayersAfterScroll, and then the style is changed.  Changing the clipping style doesn't cause layout (just StyleDifferenceRepaint).  This won't cause updateLayerPositions() to be called, so the composited layer bounds will not be updated, and the incorrect clip from the previous update will be used.

This could be fixed by:
(1) Change the approach in r111456 to walk the render layer tree and only consider overflow, rather than using cached clip rects.  It seems like a bonus to have a smaller backing, but maybe this isn't intentional.

(2) Having a change in clipping style somehow triggering a call updateLayerPositions.  I'm not totally sure the best way to go about this, but it's also a possibility.  Making a change in clip style cause StyleDifferenceLayout does the right thing for this test case, but that seems too heavy handed.

Thoughts?

[Adrienne Walker]

Created attachment 136588 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 136588 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136588&action=review

> Source/WebCore/rendering/RenderLayer.cpp:4718
> +        compositor()->setCompositingLayersNeedRebuild();

Does this end up doing just a geometry update, or does it do a full hierarchy update?

> LayoutTests/compositing/clip-change.html:35
> +      if (window.layoutTestController)
> +        layoutTestController.display();
> +      // The change in clip style should be reflected immediately in the size
> +      // of the composited clipper layer. After changing the clip, it should
> +      // entirely cover the indicator.
> +      var clipper = document.getElementById("clipper");
> +      clipper.style.clip = "rect(0px, 100px, 100px, 0px)";
> +    }

I think that tests that call display() to get layers to be updated don't always work correctly in Safari (which we should fix).

[Adrienne Walker]

(In reply to comment #4)
> (From update of attachment 136588 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136588&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:4718
> > +        compositor()->setCompositingLayersNeedRebuild();
> 
> Does this end up doing just a geometry update, or does it do a full hierarchy update?

It does a full hierarchy update.  RLB::updateGraphicsLayerGeometry() doesn't call RL::calculateLayerBounds and just uses the existing cached bounds.

> > LayoutTests/compositing/clip-change.html:35
> > +      if (window.layoutTestController)
> > +        layoutTestController.display();
> > +      // The change in clip style should be reflected immediately in the size
> > +      // of the composited clipper layer. After changing the clip, it should
> > +      // entirely cover the indicator.
> > +      var clipper = document.getElementById("clipper");
> > +      clipper.style.clip = "rect(0px, 100px, 100px, 0px)";
> > +    }
> 
> I think that tests that call display() to get layers to be updated don't always work correctly in Safari (which we should fix).

Oh, I didn't know that.  Are there more details about when display() is known to fail or known to succeed in Safari? :(

I'll check to see how this works in Safari so that I don't check in a failing test.

[Adrienne Walker]

(In reply to comment #5)
> >
> > I think that tests that call display() to get layers to be updated don't always work correctly in Safari (which we should fix).
> 
> Oh, I didn't know that.  Are there more details about when display() is known to fail or known to succeed in Safari? :(
> 
> I'll check to see how this works in Safari so that I don't check in a failing test.

On the upside, this test doesn't fail.  On the downside, it didn't fail before this change either.

smfr: Do you want me to make this test fail on Safari as well, or just check in this patch as-is, knowing that it will start working as intended once that bug is fixed?

[Simon Fraser (smfr)]

Go ahead with the current patch.

[WebKit Review Bot]

Comment on attachment 136588 [details]
Patch

Clearing flags on attachment: 136588

Committed r113883: <http://trac.webkit.org/changeset/113883>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.