Bug 84979

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

Shawn Singh
Reported 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!
Attachments
Patch (6.81 KB, patch)
2012-04-26 15:24 PDT, Shawn Singh 		no flags
DetailsFormatted DiffDiff
Addressed reviewers comments (7.35 KB, patch)
2012-04-26 17:39 PDT, Shawn Singh 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Shawn Singh
Comment 1 2012-04-26 15:24:15 PDT
Created attachment 139083 [details] Patch
Shawn Singh
Comment 2 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
Adrienne Walker
Comment 3 2012-04-26 16:38:40 PDT
Comment on attachment 139083 [details] Patch 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.
Shawn Singh
Comment 4 2012-04-26 17:39:31 PDT
Created attachment 139104 [details] Addressed reviewers comments
Adrienne Walker
Comment 5 2012-04-26 17:43:16 PDT
Comment on attachment 139104 [details] Addressed reviewers comments Nice, thanks! R=me.
Shawn Singh
Comment 6 2012-04-27 09:37:05 PDT
Comment on attachment 139104 [details] Addressed reviewers comments Thanks for reviewing, as always =)
WebKit Review Bot
Comment 7 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>
WebKit Review Bot
Comment 8 2012-04-27 12:34:35 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.