Bug 84979 - Infinite backgroundClipRect should not be scrolled.
: Infinite backgroundClipRect should not be scrolled.
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-04-26 11:37 PST by
Modified: 2012-04-27 12:34 PST (History)


Attachments
Patch (6.81 KB, patch)
2012-04-26 15:24 PST, Shawn Singh
no flags Review Patch | Details | Formatted Diff | Diff
Addressed reviewers comments (7.35 KB, patch)
2012-04-26 17:39 PST, Shawn Singh
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-26 11:37:00 PST
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 From 2012-04-26 15:24:15 PST -------
Created an attachment (id=139083) [details]
Patch
------- Comment #2 From 2012-04-26 15:27:52 PST -------
(In reply to comment #1)
> Created an attachment (id=139083) [details] [details]
> Patch

- includes new layout test
- tested on all unit tests and layout tests, no obvious regressions
------- Comment #3 From 2012-04-26 16:38:40 PST -------
(From update of 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 From 2012-04-26 17:39:31 PST -------
Created an attachment (id=139104) [details]
Addressed reviewers comments
------- Comment #5 From 2012-04-26 17:43:16 PST -------
(From update of attachment 139104 [details])
Nice, thanks!  R=me.
------- Comment #6 From 2012-04-27 09:37:05 PST -------
(From update of attachment 139104 [details])

Thanks for reviewing, as always =)
------- Comment #7 From 2012-04-27 12:34:31 PST -------
(From update of attachment 139104 [details])
Clearing flags on attachment: 139104

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