Bug 84979

Summary: Infinite backgroundClipRect should not be scrolled.
Product: WebKit Reporter: Shawn Singh <shawnsingh>
Component: Layout and RenderingAssignee: Shawn Singh <shawnsingh>
Severity: Normal CC: enne, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Addressed reviewers comments none

Description Shawn Singh 2012-04-26 11:37:00 PDT
It is possible for the backgroundClipRect to be an infinite rect  (equal to PaintInfo::InfiniteRect()).

However, in RenderLayer::backgroundClipRect() this backgroundClipRect is being scrolled when it is fixed position on the root layer.  By scrolling the backgroundClipRect, it no longer equals PaintInfo::InfiniteRect() and later in the code, goes through incorrect code paths where that rect is not considered infinite.  As a result, chromium was receiving overflowed bounds on some layers which caused stuttering when scrolling fixed-position layers.  (for example, 2011.beercamp.com)

For now, the correct fix is to avoid scrolling a rect that is considered to be infinite.   A better solution in the long term is to robustify the semantics of infinite rects so that moving them will not change them, since they are already infinite.  However, this requires pervading all clipRects / layoutRects / IntRects with the notion of being infinite.

I have the fix, just need to create a layout test before submitting a patch!
Comment 1 Shawn Singh 2012-04-26 15:24:15 PDT
Created attachment 139083 [details]
Comment 2 Shawn Singh 2012-04-26 15:27:52 PDT
(In reply to comment #1)
> Created an attachment (id=139083) [details]
> Patch

- includes new layout test
- tested on all unit tests and layout tests, no obvious regressions
Comment 3 Adrienne Walker 2012-04-26 16:38:40 PDT
Comment on attachment 139083 [details]

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

Nice test! A few small nits:

> Source/WebCore/rendering/RenderLayer.cpp:3911
> +    if (parentRects.fixed() && rootLayer->renderer() == view && !(backgroundClipRect == PaintInfo::infiniteRect()))

!(x == y) => x != y

If ClipRect somehow doesn't support !=, then maybe it should.

> LayoutTests/compositing/iframes/resources/fixed-position-transformed-subframe.html:1
> +<html>

<!DOCTYPE html> at the top, since this isn't about quirks mode.

> LayoutTests/compositing/iframes/resources/fixed-position-transformed-subframe.html:33
> +    if (window.internals)
> +      window.internals.settings.setMockScrollbarsEnabled(true);

(Aren't mock scrollbars always on in compositing/ for Chromium and Safari?)

> LayoutTests/compositing/iframes/resources/fixed-position-transformed-subframe.html:36
> +      layoutTestController.waitUntilDone();

(I'm pretty sure you don't need to do waitUntilDone/notifyDone if you do everything in the onload handler.)

> LayoutTests/compositing/iframes/scroll-fixed-transformed-element.html:1
> +<html>

Same here.
Comment 4 Shawn Singh 2012-04-26 17:39:31 PDT
Created attachment 139104 [details]
Addressed reviewers comments
Comment 5 Adrienne Walker 2012-04-26 17:43:16 PDT
Comment on attachment 139104 [details]
Addressed reviewers comments

Nice, thanks!  R=me.
Comment 6 Shawn Singh 2012-04-27 09:37:05 PDT
Comment on attachment 139104 [details]
Addressed reviewers comments

Thanks for reviewing, as always =)
Comment 7 WebKit Review Bot 2012-04-27 12:34:31 PDT
Comment on attachment 139104 [details]
Addressed reviewers comments

Clearing flags on attachment: 139104

Committed r115471: <http://trac.webkit.org/changeset/115471>
Comment 8 WebKit Review Bot 2012-04-27 12:34:35 PDT
All reviewed patches have been landed.  Closing bug.