103452 2012-11-27 13:24:01 -0800 [EFL][Qt][WK2] Fixed position elements are not always fixed 2012-12-11 09:40:54 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED http://trac.webkit.org/export/135909/trunk/ManualTests/fixed-position.html P2 Normal --- 103105 103747 1 yael yael bdakin eric hausmann jturcotte kenneth leviw michael.bruning noam ojan simon.fraser skyostil webkit.review.bot oldest_to_newest 777167 0 yael 2012-11-27 13:24:01 -0800 SSIA 781303 1 kenneth 2012-12-03 03:37:11 -0800 fixed layer fount, but do we have backing? (nil) fixed layer fount, but do we have backing? 0x7fd4e2768380 In static void updateOffsetFromViewportForSelf(RenderLayer* renderLayer) the fixed elements fail because they have no backing. 781306 2 kenneth 2012-12-03 03:39:36 -0800 This is the check which fails in 1951 bool RenderLayerCompositor::requiresCompositingForPosition(RenderObject* renderer, const RenderLayer* layer) const 1977 // Fixed position elements that are invisible in the current view don't get their own layer. 1978 if (FrameView* frameView = m_renderView->frameView()) { 1979 IntRect viewBounds = IntRect(IntPoint(frameView->scrollOffsetForFixedPosition()), frameView->layoutSize()); 1980 IntRect layerBounds = calculateCompositedBounds(layer, rootRenderLayer()); 1981 if (!viewBounds.intersects(layerBounds)) 1982 return false; 1983 } 781311 3 kenneth 2012-12-03 03:45:21 -0800 This fixes it, but I don't know why things were done the other way: --- a/Source/WebCore/rendering/RenderLayerCompositor.cpp +++ b/Source/WebCore/rendering/RenderLayerCompositor.cpp @@ -1974,7 +1974,8 @@ bool RenderLayerCompositor::requiresCompositingForPosition(RenderObject* rendere // Fixed position elements that are invisible in the current view don't get their own layer. if (FrameView* frameView = m_renderView->frameView()) { - IntRect viewBounds = IntRect(IntPoint(frameView->scrollOffsetForFixedPosition()), frameView->layoutSize()); + //IntRect viewBounds = IntRect(IntPoint(frameView->scrollOffsetForFixedPosition()), frameView->layoutSize()); + IntRect viewBounds = frameView->visibleContentRect(); IntRect layerBounds = calculateCompositedBounds(layer, rootRenderLayer()); if (!viewBounds.intersects(layerBounds)) return false; 781349 4 michael.bruning 2012-12-03 05:08:52 -0800 Qt is affected by this as well. 781377 5 177240 kenneth 2012-12-03 05:58:52 -0800 Created attachment 177240 Patch 781489 6 simon.fraser 2012-12-03 08:41:31 -0800 What's the difference between visibleContentRect().location() and scrollOffsetForFixedPosition()? 781526 7 kenneth 2012-12-03 09:22:06 -0800 (In reply to comment #6) > What's the difference between visibleContentRect().location() and scrollOffsetForFixedPosition()? I am not really sure, so it would be good with Beth's comments here. I think that is relates t how you are doing scrolling with the scrolling coordinator which we don't use. The method actually calls a similar name method in ScrollingCoordinator. Maybe we could ifdef it for platforms not using ScrollingCoordinator. But I would like to first understand if there is something that I am missing here. 781699 8 bdakin 2012-12-03 11:30:35 -0800 (In reply to comment #6) > What's the difference between visibleContentRect().location() and scrollOffsetForFixedPosition()? scrollOffsetForFixedPosition() is adjusted for scroll origin, frame scale factor, and he Settings::fixedElementsLayoutRelativeToFrame(). I'm not sure why the code wouldn't work on other platforms. In case it's not clear from the svn-blaming, I did not write that code, I just moved it so that it could also be called from the scrolling thread. 781795 9 177240 simon.fraser 2012-12-03 13:02:05 -0800 Comment on attachment 177240 Patch Please survey the code for other places which might get this wrong. Maybe scrollOffsetForFixedPosition() needs some comments next to it in the header. 781864 10 kenneth 2012-12-03 14:02:20 -0800 (In reply to comment #8) > (In reply to comment #6) > > What's the difference between visibleContentRect().location() and scrollOffsetForFixedPosition()? > > scrollOffsetForFixedPosition() is adjusted for scroll origin, frame scale factor, and he Settings::fixedElementsLayoutRelativeToFrame(). I'm not sure why the code wouldn't work on other platforms. In case it's not clear from the svn-blaming, I did not write that code, I just moved it so that it could also be called from the scrolling thread. For one, Qt and EFL doesn't use frameScaleFactor() but does scaling outside of WebCore. We might consider using it, if there are compelling reasons for doing so, beyond sharing more code paths. 781933 11 bdakin 2012-12-03 15:16:45 -0800 I don't think we want this change on the Mac! 781937 12 kenneth 2012-12-03 15:19:14 -0800 (In reply to comment #11) > I don't think we want this change on the Mac! Ok, I can ifdef it then. Can you explain me in that situations IntRect(IntPoint(frameView->scrollOffsetForFixedPosition()), frameView->layoutSize()); is different that the visible contents rect on mac? Maybe there are some cases we need to be aware of as well. 781940 13 bdakin 2012-12-03 15:23:31 -0800 (In reply to comment #12) > (In reply to comment #11) > > I don't think we want this change on the Mac! > > Ok, I can ifdef it then. Can you explain me in that situations IntRect(IntPoint(frameView->scrollOffsetForFixedPosition()), frameView->layoutSize()); is different that the visible contents rect on mac? > > Maybe there are some cases we need to be aware of as well. I take it back! scrollOffsetForFixedPosition() is needed sometimes in the Mac code because visibleContentRect() will return negative offsets when you are in the rubber-band phase of a scroll on the Mac. However, we don't need to worry about that in the RenderLayerCompositor code, so I think it is safe to use visibleContentRect here. Sorry for the confusion! 781942 14 kenneth 2012-12-03 15:25:06 -0800 > I take it back! scrollOffsetForFixedPosition() is needed sometimes in the Mac code because visibleContentRect() will return negative offsets when you are in the rubber-band phase of a scroll on the Mac. However, we don't need to worry about that in the RenderLayerCompositor code, so I think it is safe to use visibleContentRect here. > > Sorry for the confusion! OK, thanks for the explanation! I will land it then. 781986 15 177240 webkit.review.bot 2012-12-03 15:56:13 -0800 Comment on attachment 177240 Patch Rejecting attachment 177240 from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ripts/update-webkit line 152. Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2 Updating OpenSource From http://git.chromium.org/external/Webkit ae22de8..2a054f9 HEAD -> origin/HEAD error: Ref refs/remotes/origin/master is at 2a054f9cb1bc438521d4764c001d072db3d434b7 but expected ae22de8513848241262c5d0b497139a3847cd99d ! ae22de8..2a054f9 master -> origin/master (unable to update local ref) Died at Tools/Scripts/update-webkit line 152. Full output: http://queues.webkit.org/results/15132151 781999 16 kenneth 2012-12-03 16:05:54 -0800 Landed in 136452 788397 17 skyostil 2012-12-11 06:39:25 -0800 This change seems to be causing some trouble when page pageScaleFactor != 1. This is because viewBounds is now in logical pixels while layerBounds is in CSS pixels. I'll put together a patch. 788401 18 kenneth 2012-12-11 06:42:06 -0800 (In reply to comment #17) > This change seems to be causing some trouble when page pageScaleFactor != 1. This is because viewBounds is now in logical pixels while layerBounds is in CSS pixels. I'll put together a patch. Ah sorry about that. For Qt and EFL the viewport contents rect is in css pixels as we do scaling outside of webcore (not using frameScaleFactor) 788516 19 skyostil 2012-12-11 09:40:54 -0800 FYI, Xianzhu has a fix here: https://bugs.webkit.org/show_bug.cgi?id=104303. 177240 2012-12-03 05:58:52 -0800 2012-12-03 16:06:08 -0800 Patch bug-103452-20121203145617.patch text/plain 2068 kenneth U3VidmVyc2lvbiBSZXZpc2lvbjogMTM2Mzg3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNjgzZDk2YjQ3YjUzMzlh MjliNDJlZWNhMzNmM2Y1ZDIwM2UzY2E3ZC4uMGEzZGI0YzJiNmEzNDU3ZWFlOGU1MzMwZjFkNzM5 MTY4ZjQ3ZmVlYiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDEyLTEyLTAzICBLZW5u ZXRoIFJvaGRlIENocmlzdGlhbnNlbiAgPGtlbm5ldGhAd2Via2l0Lm9yZz4KKworICAgICAgICBb RUZMXVtRdF1bV0syXSBGaXhlZCBwb3NpdGlvbiBlbGVtZW50cyBhcmUgbm90IGFsd2F5cyBmaXhl ZAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAzNDUy CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhlIGNv ZGUgZmlndXJpbmcgb3V0IHdoZXRoZXIgZml4ZWQgcG9zaXRpb24gbGF5ZXJzIGFyZSBpbnNpZGUK KyAgICAgICAgdGhlIHZpc2libGUgdmlld3BvcnQsIGFzc3VtZXMgdGhhdCB0aGUgdmlzaWJsZSB2 aWV3cG9ydCBpcyBhbHdheXMKKyAgICAgICAgdGhlIHNpemUgb2YgdGhlIGxheW91dCB2aWV3cG9y dC4gVGhpcyBhc3N1bXB0aW9uIGRvZXNuJ3QgaG9sZCB3aXRoCisgICAgICAgIGhvdyB0aGUgUXQg YW5kIEVGTCB0aWxlZCBiYWNraW5nIHN0b3JlIGFuZCBjb29yZGluYXRlZCBncmFwaGljcworICAg ICAgICB3b3Jrcywgc28gaW5zdGVhZCB1c2luZyB0aGUgdmlzaWJsZUNvbnRlbnRzUmVjdCgpLCB3 aGljaCBwcm92aWRlcworICAgICAgICB0aGUgcmlnaHQgdmFsdWVzIGluIGFsbCBjYXNlcy4KKwor ICAgICAgICAqIHJlbmRlcmluZy9SZW5kZXJMYXllckNvbXBvc2l0b3IuY3BwOgorICAgICAgICAo V2ViQ29yZTo6UmVuZGVyTGF5ZXJDb21wb3NpdG9yOjpyZXF1aXJlc0NvbXBvc2l0aW5nRm9yUG9z aXRpb24pOgorCiAyMDEyLTEyLTAzICBTaGVyaWZmIEJvdCAgPHdlYmtpdC5yZXZpZXcuYm90QGdt YWlsLmNvbT4KIAogICAgICAgICBVbnJldmlld2VkLCByb2xsaW5nIG91dCByMTM2Mzc3LgpkaWZm IC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckxheWVyQ29tcG9zaXRvci5j cHAgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyTGF5ZXJDb21wb3NpdG9yLmNwcApp bmRleCAzZGRmZTQ1MzgxMjRkYTBjMzU4Y2QxZWEyZWJhNzQxNmY3NTBjZmFlLi5iNjYxMDA0MWQ0 ZmE3OTZmN2NlMTBiZWZjMzAyYmM4MjAwZWMzYzM4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9yZW5kZXJpbmcvUmVuZGVyTGF5ZXJDb21wb3NpdG9yLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29y ZS9yZW5kZXJpbmcvUmVuZGVyTGF5ZXJDb21wb3NpdG9yLmNwcApAQCAtMTk3NCw3ICsxOTc0LDcg QEAgYm9vbCBSZW5kZXJMYXllckNvbXBvc2l0b3I6OnJlcXVpcmVzQ29tcG9zaXRpbmdGb3JQb3Np dGlvbihSZW5kZXJPYmplY3QqIHJlbmRlcmUKIAogICAgIC8vIEZpeGVkIHBvc2l0aW9uIGVsZW1l bnRzIHRoYXQgYXJlIGludmlzaWJsZSBpbiB0aGUgY3VycmVudCB2aWV3IGRvbid0IGdldCB0aGVp ciBvd24gbGF5ZXIuCiAgICAgaWYgKEZyYW1lVmlldyogZnJhbWVWaWV3ID0gbV9yZW5kZXJWaWV3 LT5mcmFtZVZpZXcoKSkgewotICAgICAgICBJbnRSZWN0IHZpZXdCb3VuZHMgPSBJbnRSZWN0KElu dFBvaW50KGZyYW1lVmlldy0+c2Nyb2xsT2Zmc2V0Rm9yRml4ZWRQb3NpdGlvbigpKSwgZnJhbWVW aWV3LT5sYXlvdXRTaXplKCkpOworICAgICAgICBJbnRSZWN0IHZpZXdCb3VuZHMgPSBmcmFtZVZp ZXctPnZpc2libGVDb250ZW50UmVjdCgpOwogICAgICAgICBJbnRSZWN0IGxheWVyQm91bmRzID0g Y2FsY3VsYXRlQ29tcG9zaXRlZEJvdW5kcyhsYXllciwgcm9vdFJlbmRlckxheWVyKCkpOwogICAg ICAgICBpZiAoIXZpZXdCb3VuZHMuaW50ZXJzZWN0cyhsYXllckJvdW5kcykpCiAgICAgICAgICAg ICByZXR1cm4gZmFsc2U7Cg==