Bug 89216

Created attachment 147825 [details] Patch

(In reply to comment #0) > Render layers with CSS transforms should become containers for any fixed positioned descendants. This surprises me a bit - why is there a relationship between CSS transforms and fixed positioned descendants? Is this in a spec somewhere? > However, because this check is done with RenderLayer::hasTransform(), we also end up marking the RenderLayer for the RenderView as fixed position container if a non-identity page scale factor is used. This is because page scale is applied as a transform for that layer. > > This breaks fixed position layers, because they become fixed relative to the RenderView layer instead of outer scroll clip layer. > > The fix is to avoid marking any RenderView layers as fixed position containers.

http://dev.w3.org/csswg/css3-transforms/ -- just search for the word "fixed". I'm sure Simon will have something more authoritative to say, though. Perhaps that part of the spec is still in a bit of flux?

" The object acts as a containing block for fixed positioned descendants" Yep, it's like that because Dave Hyatt said it would be really hard to do any other way.

Comment on attachment 147825 [details] Patch r- for lack of a test.

Wow, that's weird (although I guess doing otherwise would be a pain in the butt). OK.

Created attachment 148091 [details] Patch - Added testing framework for ScrollingCoordinator. - Added layout test to verify fix.

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Comment on attachment 148091 [details] Patch Attachment 148091 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12979166

Created attachment 148101 [details] Patch - Fixed efl build failure.

Comment on attachment 148101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148101&action=review > Source/WebCore/ChangeLog:3 > + RenderView layer is marked as fixed position container if page scale != 1 Normally when we talked about "fixed position containers", we're talking about the render tree, but this bug only exists in the scrolling tree. The bug title should say this. > Source/WebCore/ChangeLog:22 > + This patch also adds a testing framework for ScrollingCoordinator. Layout > + tests can call testRunner.scrollingTreeAsText() to retrieve a text > + representation of the scrolling tree of the current page. This feature is > + currently only implemented for the Chromium scrolling coordinator back-end. Do you think that the scrolling tree output is comparable across implementations? If not, the utility of this way of testing is reduced. > LayoutTests/platform/chromium/compositing/fixed-position-container-with-page-scale.html:22 > + testRunner.dumpAsText(true); Why dumpAsText(true)? The pixel result doesn't seem useful.

Comment on attachment 148101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148101&action=review Thanks Simon. >> Source/WebCore/ChangeLog:3 >> + RenderView layer is marked as fixed position container if page scale != 1 > > Normally when we talked about "fixed position containers", we're talking about the render tree, but this bug only exists in the scrolling tree. The bug title should say this. Good point. I'll change this. >> Source/WebCore/ChangeLog:22 >> + currently only implemented for the Chromium scrolling coordinator back-end. > > Do you think that the scrolling tree output is comparable across implementations? If not, the utility of this way of testing is reduced. The current format certainly isn't, but I think I can make a few tweaks to make it more feasible for all implementations to produce the same output. I think we can also adjust the format later if there is need. >> LayoutTests/platform/chromium/compositing/fixed-position-container-with-page-scale.html:22 >> + testRunner.dumpAsText(true); > > Why dumpAsText(true)? The pixel result doesn't seem useful. Ah, I think I got confused by the dumpAsText arguments again. Will fix.

Created attachment 148129 [details] Patch - Made scrollingTreeAsText() output less Chromium-specific. - Only save text reference in the test. - Updated bug title.

Created attachment 148530 [details] Patch Rebased. Another look please?

Comment on attachment 148530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148530&action=review > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:295 > +String ScrollingCoordinator::scrollingTreeAsText() const > +{ > + ASSERT(isMainThread()); > +#if USE(ACCELERATED_COMPOSITING) > + if (!m_page || !m_page->mainFrame()) > + return String(); > + > + RenderView* renderView = m_page->mainFrame()->contentRenderer(); > + if (!renderView || !renderView->compositor() || !renderView->compositor()->rootGraphicsLayer()) > + return String(); > + > + TextStream textStream; > + dumpLayer(textStream, renderView->compositor()->rootGraphicsLayer(), 0); > + return textStream.release(); > +#else > + return String(); > +#endif > +} I'd expect this to walk the ScrollingTree nodes, not to just go to GraphicsLayers. Not all platforms will necessarily store scrolling tree info under GraphicsLayers. I think you should split outs the scrollingTreeAsText changes into a separate bug.

(In reply to comment #15) > I'd expect this to walk the ScrollingTree nodes, not to just go to GraphicsLayers. Not all platforms will necessarily store scrolling tree info under GraphicsLayers. The problem is that the Chromium ScrollingCoordinator implementation doesn't really have a concept of a scrolling tree, so this is the best it can do. I'm open to suggestions on how to make the coordinator testable while allowing for multiple back-end implementations. > I think you should split outs the scrollingTreeAsText changes into a separate bug. Ok, I'll do that.

Created attachment 148580 [details] Patch - Removed scrolling tree test framework and test.

Comment on attachment 148580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148580&action=review > Source/WebCore/ChangeLog:19 > + No new test because the scrolling tree isn't currently testable. But can't you make a pixel test or ref test that would show the bug?

(In reply to comment #18) > But can't you make a pixel test or ref test that would show the bug? The problem is that the bug is triggered only if the scrolling coordinator tries to position the fixed layer independently of the main thread. In Chromium's case this happens if the compositor processes scroll input events and renders *before* it gets a chance to synchronize the layer tree with the main thread. Since the layout tests are driven from the main thread, I'm not sure how to capture a snapshot of this intermediate state. For manual testing I usually tie up the main thread with some blocking JS to allow the compositor thread to work independently. Cases like this are why I wanted to have a more formal way to test ScrollingCoordinator.

Testing the scrolling tree is now split off into bug 89576.

Comment on attachment 148580 [details] Patch There seems to be some controversy in Bug 89576 about how to test the scrolling tree. Would it make senes to land this fix and continue discussing the testing issues in Bug 89576?

Any opinions about comment #21?

Comment on attachment 148580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148580&action=review > Source/WebCore/ChangeLog:8 > + Render layers with CSS transforms should become containers for anyfixed any/fixed > Source/WebCore/rendering/RenderLayerBacking.cpp:455 > + bool isContainer = m_owningLayer->hasTransform() && !renderer()->isRenderView(); We also use isRootLayer() for the renderview's layer.

Created attachment 151727 [details] Patch Fixed typo and used isRootLayer() instead of isRenderView()

Comment on attachment 151727 [details] Patch Clearing flags on attachment: 151727 Committed r122343: <http://trac.webkit.org/changeset/122343>

All reviewed patches have been landed. Closing bug.