Bug 103452

Summary: [EFL][Qt][WK2] Fixed position elements are not always fixed
Product: WebKit Reporter: Yael <yael>
Component: Layout and RenderingAssignee: Yael <yael>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, eric, hausmann, jturcotte, kenneth, leviw, michael.bruning, noam, ojan, simon.fraser, skyostil, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://trac.webkit.org/export/135909/trunk/ManualTests/fixed-position.html
Bug Depends on:    
Bug Blocks: 103105, 103747    
Attachments:
Description Flags
Patch simon.fraser: review+, webkit.review.bot: commit-queue-

Description Yael 2012-11-27 13:24:01 PST
SSIA
Comment 1 Kenneth Rohde Christiansen 2012-12-03 03:37:11 PST
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.
Comment 2 Kenneth Rohde Christiansen 2012-12-03 03:39:36 PST
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     }
Comment 3 Kenneth Rohde Christiansen 2012-12-03 03:45:21 PST
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;
Comment 4 Michael Brüning 2012-12-03 05:08:52 PST
Qt is affected by this as well.
Comment 5 Kenneth Rohde Christiansen 2012-12-03 05:58:52 PST
Created attachment 177240 [details]
Patch
Comment 6 Simon Fraser (smfr) 2012-12-03 08:41:31 PST
What's the difference between visibleContentRect().location() and scrollOffsetForFixedPosition()?
Comment 7 Kenneth Rohde Christiansen 2012-12-03 09:22:06 PST
(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.
Comment 8 Beth Dakin 2012-12-03 11:30:35 PST
(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.
Comment 9 Simon Fraser (smfr) 2012-12-03 13:02:05 PST
Comment on attachment 177240 [details]
Patch

Please survey the code for other places which might get this wrong. Maybe scrollOffsetForFixedPosition() needs some comments next to it in the header.
Comment 10 Kenneth Rohde Christiansen 2012-12-03 14:02:20 PST
(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.
Comment 11 Beth Dakin 2012-12-03 15:16:45 PST
I don't think we want this change on the Mac!
Comment 12 Kenneth Rohde Christiansen 2012-12-03 15:19:14 PST
(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.
Comment 13 Beth Dakin 2012-12-03 15:23:31 PST
(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!
Comment 14 Kenneth Rohde Christiansen 2012-12-03 15:25:06 PST
> 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.
Comment 15 WebKit Review Bot 2012-12-03 15:56:13 PST
Comment on attachment 177240 [details]
Patch

Rejecting attachment 177240 [details] 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
Comment 16 Kenneth Rohde Christiansen 2012-12-03 16:05:54 PST
Landed in 136452
Comment 17 Sami Kyöstilä 2012-12-11 06:39:25 PST
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.
Comment 18 Kenneth Rohde Christiansen 2012-12-11 06:42:06 PST
(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)
Comment 19 Sami Kyöstilä 2012-12-11 09:40:54 PST
FYI, Xianzhu has a fix here: https://bugs.webkit.org/show_bug.cgi?id=104303.