84965 – Composited fixed-position elements are wrongly positioned when scroll position is restored from history

RESOLVED FIXED 84965

Composited fixed-position elements are wrongly positioned when scroll position is restored from history

https://bugs.webkit.org/show_bug.cgi?id=84965

Summary Composited fixed-position elements are wrongly positioned when scroll positio...

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	Details Formatted Diff Diff
Added layout test (4.63 KB, patch) 2012-04-26 11:32 PDT, Iain Merrick	no flags	Details Formatted Diff Diff
Added baseline (5.95 KB, patch) 2012-04-27 03:59 PDT, Iain Merrick	no flags	Details Formatted Diff Diff
Addressed Sami's comments (6.01 KB, patch) 2012-04-27 04:47 PDT, Iain Merrick	no flags	Details Formatted Diff Diff
Changed condition to 'm_nestedLayoutCount <= 1' (9.97 KB, patch) 2012-06-12 05:16 PDT, Iain Merrick	no flags	Details Formatted Diff Diff
Clarified the logistics of the test helper page (9.96 KB, patch) 2012-06-12 09:01 PDT, Iain Merrick	no flags	Details Formatted Diff Diff
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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component Layout and Rendering

Assignee

Iain Merrick

Reported

2012-04-26 08:10 PDT

Modified

2012-06-18 09:34 PDT History

CC List

6 users Show

URL

Keywords

Depends on

Blocks