Bug 84965

Summary: Composited fixed-position elements are wrongly positioned when scroll position is restored from history
Product: WebKit Reporter: Iain Merrick <husky>
Component: Layout and RenderingAssignee: Iain Merrick <husky>
Status: RESOLVED FIXED    
Severity: Normal CC: enne, husky, jamesr, simon.fraser, skyostil, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Added layout test
none
Added baseline
none
Addressed Sami's comments
none
Changed condition to 'm_nestedLayoutCount <= 1'
none
Clarified the logistics of the test helper page none

Iain Merrick
Reported 2012-04-26 08:10:33 PDT
Forked off from https://bugs.webkit.org/show_bug.cgi?id=70103 because it's a bug in existing code. Steps to reproduce: - Open any page - Open another page with a composited fixed-position element (e.g. one with a 3D transform) - Scroll down - Go back to the previous page - Go forward The scroll position is restored, but the fixed-position element will be in the wrong place. When you scroll further it pops back to the correct position. I think this is because the scroll position is restored while we're inside FrameView::layout(), and updateFixedElementsAfterScrolling() looks like this: void FrameView::updateFixedElementsAfterScrolling() { #if USE(ACCELERATED_COMPOSITING) if (!m_nestedLayoutCount && hasFixedObjects()) { if (RenderView* root = rootRenderer(this)) { root->compositor()->updateCompositingLayers(CompositingUpdateOnScroll); } } #endif } m_nestedLayoutCount is 1, so we don't call updateCompositingLayers().
Attachments
Patch (1.30 KB, patch)
2012-04-26 08:21 PDT, Iain Merrick 		no flags
DetailsFormatted DiffDiff
Added layout test (4.63 KB, patch)
2012-04-26 11:32 PDT, Iain Merrick 		no flags
DetailsFormatted DiffDiff
Added baseline (5.95 KB, patch)
2012-04-27 03:59 PDT, Iain Merrick 		no flags
DetailsFormatted DiffDiff
Addressed Sami's comments (6.01 KB, patch)
2012-04-27 04:47 PDT, Iain Merrick 		no flags
DetailsFormatted DiffDiff
Changed condition to 'm_nestedLayoutCount <= 1' (9.97 KB, patch)
2012-06-12 05:16 PDT, Iain Merrick 		no flags
DetailsFormatted DiffDiff
Clarified the logistics of the test helper page (9.96 KB, patch)
2012-06-12 09:01 PDT, Iain Merrick 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Iain Merrick
Comment 1 2012-04-26 08:13:49 PDT
updateFixedElementsAfterScrolling() was added recently in https://bugs.webkit.org/show_bug.cgi?id=80647, but it doesn't handle the case where scrolling takes place during layout. CCing jamesr who reviewed that. If we just remove the "!m_nestedLayoutCount" condition, that seems to fix the bug. But I don't know if that's safe. I'll upload a patch and make a layout test, anyway.
Iain Merrick
Comment 2 2012-04-26 08:21:32 PDT
Created attachment 139009 [details] Patch
Iain Merrick
Comment 3 2012-04-26 11:32:20 PDT
Created attachment 139032 [details] Added layout test
Iain Merrick
Comment 4 2012-04-26 11:33:24 PDT
Layout test needs baselining. It works in the browser but not yet in DumpRenderTree -- seems to be something unusual about scrolling and/or history there.
Simon Fraser (smfr)
Comment 5 2012-04-26 11:48:18 PDT
Comment on attachment 139032 [details] Added layout test View in context: https://bugs.webkit.org/attachment.cgi?id=139032&action=review > Source/WebCore/page/FrameView.cpp:1773 > - if (!m_nestedLayoutCount && hasFixedObjects()) { > + if (hasFixedObjects()) { Do we really want to do this in nested layouts, or should we just fix things up at the end?
Iain Merrick
Comment 6 2012-04-27 02:22:58 PDT
I'm concerned about potentially displaying an incorrectly-positioned frame before the fixup happens. However, I don't think this is actually a nested layout -- m_nestedLayoutCount is 1. Would it make sense to change the condition to "if (m_nestedLayoutCount <= 1 && hasFixedObjects())"? But there might be a problem in the following sequence: - layout() - layout() - scrollTo() - updateFixedElementsAfterScrolling() -- ignored because of nesting It seems like the outermost layout() won't call updateFixedElementsAfterScrolling() so we'll end up with incorrect positioning.
Iain Merrick
Comment 7 2012-04-27 03:59:27 PDT
Created attachment 139167 [details] Added baseline
Sami Kyostila
Comment 8 2012-04-27 04:18:43 PDT
Comment on attachment 139167 [details] Added baseline View in context: https://bugs.webkit.org/attachment.cgi?id=139167&action=review > LayoutTests/compositing/fixed-position-scroll-offset-history-restore.html:17 > +setTimeout(function() { Would this be more reliable as an onload function? > LayoutTests/compositing/resources/fixed-position-scroll-offset-history-restore-2.html:7 > +{ Nit: brace should be on the previous line?
Iain Merrick
Comment 9 2012-04-27 04:36:25 PDT
Comment on attachment 139167 [details] Added baseline View in context: https://bugs.webkit.org/attachment.cgi?id=139167&action=review >> LayoutTests/compositing/fixed-position-scroll-offset-history-restore.html:17 >> +setTimeout(function() { > > Would this be more reliable as an onload function? I'll give it a try >> LayoutTests/compositing/resources/fixed-position-scroll-offset-history-restore-2.html:7 >> +{ > > Nit: brace should be on the previous line? Oops, yes
Iain Merrick
Comment 10 2012-04-27 04:47:27 PDT
Created attachment 139170 [details] Addressed Sami's comments
James Robinson
Comment 11 2012-04-27 10:36:34 PDT
(In reply to comment #6) > It seems like the outermost layout() won't call updateFixedElementsAfterScrolling() so we'll end up with incorrect positioning. Why not?
Iain Merrick
Comment 12 2012-04-29 03:09:28 PDT
(In reply to comment #11) > (In reply to comment #6) > > It seems like the outermost layout() won't call updateFixedElementsAfterScrolling() so we'll end up with incorrect positioning. > > Why not? Because updateFixedElementsAfterScrolling is called from scrollTo, and scrollTo might be called from a nested layout(). I don't think the outermost layout() would also call scrollTo, would it?
Iain Merrick
Comment 13 2012-06-12 05:16:12 PDT
Created attachment 147059 [details] Changed condition to 'm_nestedLayoutCount <= 1'
Iain Merrick
Comment 14 2012-06-12 05:18:52 PDT
This bug still exists so I'd like to get this patch in. | I don't think this is actually a nested layout -- m_nestedLayoutCount is 1. | Would it make sense to change the condition to "if (m_nestedLayoutCount <= 1 | && hasFixedObjects())"? I tried this out and the test still passes. Simon or James, can you take another look at this?
Simon Fraser (smfr)
Comment 15 2012-06-12 08:31:22 PDT
Comment on attachment 147059 [details] Changed condition to 'm_nestedLayoutCount <= 1' View in context: https://bugs.webkit.org/attachment.cgi?id=147059&action=review > LayoutTests/compositing/resources/fixed-position-scroll-offset-history-restore-2.html:4 > +<p>This page just immediately hits the "back" button. Not sure how a page can "hit the back button". It does go back, though.
Iain Merrick
Comment 16 2012-06-12 09:01:50 PDT
Created attachment 147094 [details] Clarified the logistics of the test helper page
Iain Merrick
Comment 17 2012-06-12 10:18:20 PDT
(In reply to comment #15) > (From update of attachment 147059 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147059&action=review > > > LayoutTests/compositing/resources/fixed-position-scroll-offset-history-restore-2.html:4 > > +<p>This page just immediately hits the "back" button. > > Not sure how a page can "hit the back button". It does go back, though. Fixed
Iain Merrick
Comment 18 2012-06-18 02:42:33 PDT
James or Simon, can you review this, or suggest another reviewer? Revision history says you were the reviewers for previous changes to this method.
WebKit Review Bot
Comment 19 2012-06-18 09:33:57 PDT
Comment on attachment 147094 [details] Clarified the logistics of the test helper page Clearing flags on attachment: 147094 Committed r120601: <http://trac.webkit.org/changeset/120601>
WebKit Review Bot
Comment 20 2012-06-18 09:34:04 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.