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().
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.
Created attachment 139009 [details] Patch
Created attachment 139032 [details] Added layout test
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.
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?
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.
Created attachment 139167 [details] Added baseline
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?
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
Created attachment 139170 [details] Addressed Sami's comments
(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?
(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?
Created attachment 147059 [details] Changed condition to 'm_nestedLayoutCount <= 1'
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?
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.
Created attachment 147094 [details] Clarified the logistics of the test helper page
(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
James or Simon, can you review this, or suggest another reviewer? Revision history says you were the reviewers for previous changes to this method.
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>
All reviewed patches have been landed. Closing bug.