Need to re-layout fixed position elements after scale when using settings()->fixedElementsLayoutRelativeToFrame()
Created attachment 180254 [details] Patch
Comment on attachment 180254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180254&action=review > Source/WebCore/rendering/RenderView.cpp:1042 > + RenderBox* r = *it; Nit: use full word variables. > Source/WebCore/rendering/RenderView.h:231 > + void markFixedPositionDescendantsNeedsMovementLayout(); Bikeshed: markFixedPositionDescendantsNeedMovementLayout sounds less awkward to me. > LayoutTests/fast/repaint/relayout-fixed-position-after-scale.html:32 > + window.internals.settings.setFixedElementsLayoutRelativeToFrame(true) Nit: missing semicolon.
Oops, hit submit too soon. I think this fixes the problem nicely -- thanks Tien-Ren.
Created attachment 180405 [details] Patch
(In reply to comment #4) > Created an attachment (id=180405) [details] > Patch Fixed the nits Sami mentioned. Thanks!
Comment on attachment 180405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180405&action=review > Source/WebCore/rendering/RenderView.cpp:1045 > + TrackedRendererListHashSet* positionedDescendants = positionedObjects(); > + if (!positionedDescendants) > + return; > + TrackedRendererListHashSet::iterator end = positionedDescendants->end(); > + for (TrackedRendererListHashSet::iterator it = positionedDescendants->begin(); it != end; ++it) { > + RenderBox* renderer = *it; > + if (renderer->style()->position() != FixedPosition) > + continue; > + renderer->setNeedsPositionedMovementLayout(); You should use FrameView's list of fixed objects, not this one.
(In reply to comment #6) > (From update of attachment 180405 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180405&action=review > > > Source/WebCore/rendering/RenderView.cpp:1045 > > + TrackedRendererListHashSet* positionedDescendants = positionedObjects(); > > + if (!positionedDescendants) > > + return; > > + TrackedRendererListHashSet::iterator end = positionedDescendants->end(); > > + for (TrackedRendererListHashSet::iterator it = positionedDescendants->begin(); it != end; ++it) { > > + RenderBox* renderer = *it; > > + if (renderer->style()->position() != FixedPosition) > > + continue; > > + renderer->setNeedsPositionedMovementLayout(); > > You should use FrameView's list of fixed objects, not this one. Ah, that's a really good suggestion! I'll change it. By the way, I'm thinking to remove the USE(ACCELERATED_COMPOSITING) guard on Frame::deviceOrPageScaleFactorChanged() so the re-layout will be triggered from there instead of Page. Do you think that sounds like a better plan?
Created attachment 180440 [details] Patch
Comment on attachment 180440 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180440&action=review > LayoutTests/fast/repaint/relayout-fixed-position-after-scale.html:1 > +<!DOCTYPE html> The image result has non-mock scrollbars, which will not match most platforms. Can this be a ref test?
Created attachment 180454 [details] Patch
Comment on attachment 180454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180454&action=review > Source/WebCore/ChangeLog:17 > + (WebCore): Remove useless lines like this. > LayoutTests/fast/repaint/relayout-fixed-position-after-scale.html:9 > +::-webkit-scrollbar { > + width:0; > + height:0; > +} This doesn't work in Mac WK1. body { overflow: hidden; } is a better way to hide scrollbars.
(In reply to comment #11) > (From update of attachment 180454 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180454&action=review > > > Source/WebCore/ChangeLog:17 > > + (WebCore): > > Remove useless lines like this. Roger that. > > LayoutTests/fast/repaint/relayout-fixed-position-after-scale.html:9 > > +::-webkit-scrollbar { > > + width:0; > > + height:0; > > +} > > This doesn't work in Mac WK1. body { overflow: hidden; } is a better way to hide scrollbars. This doesn't work with scaled page. I guess the reason is that overflow:hidden only shrink <body> to the layout viewport size. Let's just add a Skip in TestExpectations?
Created attachment 180462 [details] Patch
(In reply to comment #12) > Let's just add a Skip in TestExpectations? Better to just list it as an image failure.
Created attachment 185133 [details] Patch
Changes from last patch: * Changed the hook point from Frame::deviceOrPageScaleFactorChanged() to FrameView::visibleContentsResized(). This makes more sense and make code much cleaner. * In the layout test, added a trick to force re-layout before changing scale factor again so we can avoid scrollbars. * Rebased.
Comment on attachment 185133 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185133&action=review I think the fix itself looks good. Just one tiny comment about the test. > LayoutTests/fast/repaint/relayout-fixed-position-after-scale.html:36 > + window.internals.boundingBox(document.getElementsByTagName("body")[0]); Nit: the other tests I've seen use something like element.offsetTop to force a layout. I think that would be a little less obscure.
Comment on attachment 185133 [details] Patch Attachment 185133 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16200216 New failing tests: fast/repaint/relayout-fixed-position-after-scale.html
Created attachment 185330 [details] Patch
Let's make the test chromium-specific. The mac build doesn't seem to update visibleContentRect when setPageScaleFactor is called. They use a significant different approach anyway.
Created attachment 185636 [details] Patch
Created attachment 185637 [details] Patch
Comment on attachment 185637 [details] Patch Use document.body.offsetTop magic spell as suggested by Sami. I think this patch is ready. PTAL, thanks!
Hello Simon, Do you have time to take a look? I hope the patch doesn't look too sketchy to you. :)
Created attachment 186521 [details] Patch
Created attachment 186522 [details] Patch
Comment on attachment 186522 [details] Patch Last one was a bad rebase. This one is correct.
Looks good to me, thanks.
Comment on attachment 186522 [details] Patch Why is the new test in platform/chromium when the code change is in WebCore? Is there a reason that this test won't pass on other platforms?
(In reply to comment #29) > (From update of attachment 186522 [details]) > Why is the new test in platform/chromium when the code change is in WebCore? Is there a reason that this test won't pass on other platforms? One reason is because there is no reliable way to completely hide scrollbar on Mac. Setting <body style="overflow:hidden"> won't work when a page is scaled, and the Mac doesn't support -webkit-scrollbar. Being unable to hide scrollbar, the visible content size becomes an odd number (785, 585), and rounding errors come up when page is scaled, making it impossible to write a ref-test that works. Another reason is that Mac seems to treat visibleContentRect differently. When I ran the test on a Macbook, FrameView::visibleContentsResized() wasn't called when setPageScaleFactor is called. I don't usually make tests platform-specific, but the Mac port simply works too differently in this case.
(In reply to comment #30) > One reason is because there is no reliable way to completely hide scrollbar on Mac. Setting <body style="overflow:hidden"> won't work when a page is scaled, and the Mac doesn't support -webkit-scrollbar. Being unable to hide scrollbar, the visible content size becomes an odd number (785, 585), and rounding errors come up when page is scaled, making it impossible to write a ref-test that works. You could try to use overflow:overlay, which should draw scrollbars that don't take up space. > Another reason is that Mac seems to treat visibleContentRect differently. When I ran the test on a Macbook, FrameView::visibleContentsResized() wasn't called when setPageScaleFactor is called. I don't know enough about this code or whether this is a bug or not. > I don't usually make tests platform-specific, but the Mac port simply works too differently in this case. Please mention the reasons for a platform specific test in the ChangeLog.
(In reply to comment #31) > (In reply to comment #30) > > One reason is because there is no reliable way to completely hide scrollbar on Mac. Setting <body style="overflow:hidden"> won't work when a page is scaled, and the Mac doesn't support -webkit-scrollbar. Being unable to hide scrollbar, the visible content size becomes an odd number (785, 585), and rounding errors come up when page is scaled, making it impossible to write a ref-test that works. > > You could try to use overflow:overlay, which should draw scrollbars that don't take up space. Please don't, we're going to obsolete that value at some point. Custom scrollbars work in WK2, so use them if you have to but you'll have to mark WK1 tests as expected failures.
Created attachment 186688 [details] Patch
Comment on attachment 186688 [details] Patch Revised ChangeLog as suggested.
Ping. Can I haz a r+ please? :3
Could you try moving the call into setPageScaleFactor()? This seems like a more indirect way of achieving the same thing.
(In reply to comment #36) > Could you try moving the call into setPageScaleFactor()? This seems like a more indirect way of achieving the same thing. I think visibleContentsResized is the right place, since settings()->fixedElementsLayoutRelativeToFrame() == true means fixed-position elements should stick to the visible content rect. This is the perfect single place to add this check rather than adding it everywhere that can potentially change the visible content size. The bug title may not accurately describe what this patch does, but it reflects a user-visible bug, and the test reproduces the bug well.
Comment on attachment 186688 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186688&action=review > Source/WebCore/page/FrameView.cpp:2026 > + if (fixedElementsLayoutRelativeToFrame()) > + setViewportConstrainedObjectsNeedLayout(); I don't think this is the right fix for the problem you are seeing. visibleContentsResized() is called from ScrollView::updateScrollbars() so that the view can react to the changes in visible content size due to overflow controls being created/destroyed. In your case, you have overlay scrollbars so the overflow controls do not change the amount of content space or visible content space. Thus there's no reason to react to visibleContentsResized since they haven't actually resized. I suspect you observe that this patch papers over the bug you have since it's called frequently and happens to trigger in the case where you actually do need additional layout. You need to figure out what is actually changing and why that is not triggering the layout you need.
(In reply to comment #38) > (From update of attachment 186688 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186688&action=review > > > Source/WebCore/page/FrameView.cpp:2026 > > + if (fixedElementsLayoutRelativeToFrame()) > > + setViewportConstrainedObjectsNeedLayout(); > > I don't think this is the right fix for the problem you are seeing. visibleContentsResized() is called from ScrollView::updateScrollbars() so that the view can react to the changes in visible content size due to overflow controls being created/destroyed. In your case, you have overlay scrollbars so the overflow controls do not change the amount of content space or visible content space. Thus there's no reason to react to visibleContentsResized since they haven't actually resized. I suspect you observe that this patch papers over the bug you have since it's called frequently and happens to trigger in the case where you actually do need additional layout. > > You need to figure out what is actually changing and why that is not triggering the layout you need. I consider that as a separate issue. If visibleContentsResized() is not called when it should be called, we should fix that as well, or we should mark it as obsolete if it's broken beyond fix. I agree that currently how the event propagates to visibleContentsResized is under the brittle assumption that changing the frame rect always need to recompute the scrollbar and just do it on the way. Actually I think call it in ScrollView::updateScrollbars() isn't such a wrong way to do it, as non-overlay scrollbars do change visibleContentsRect(false). It is perfectly correct to react to visibleContentsResized. If they haven't actually resized, then we don't need to re-layout fixed position elements either. My point is that if visible content size is not resized when it should, we should fix that in a separate patch. If you ask my opinion about how to do visible contents size correctly, I would say we should make it a stored value instead of a computed value. I personally hate computed value when you need to get the change notifications. If we make it a stored value, we can emit the notifications in setVisibleContentsSize(). Or another way of thinking is that we should avoid to source fixed-position layout from multiple value, but using just one authoritative FrameView::viewportConstrainedVisibleContentRect (make it a stored value). That was what I proposed in the comments of webkit.org/b/108323 . Either way, it won't be in the scope of this patch. fixedElementsLayoutRelativeToFrame()==true means use the visibleContentsSize to layout fixed position elements, so we re-layout fixed-position elements when it changes. Simple. I strongly suggest this code should be in, unless we're going to change the semantics of fixedElementsLayoutRelativeToFrame() in near future.
Created attachment 189217 [details] Patch
Changes from last patch: * Added FrameView::visibleContentScaleFactorDidChange() notification to make sure FrameView::visibleContentsResized() is called during Page::setPageScaleFactor. * Added guard against unnecessary re-layout (In reply to comment #38) > (From update of attachment 186688 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186688&action=review > > > Source/WebCore/page/FrameView.cpp:2026 > > + if (fixedElementsLayoutRelativeToFrame()) > > + setViewportConstrainedObjectsNeedLayout(); > > I don't think this is the right fix for the problem you are seeing. visibleContentsResized() is called from ScrollView::updateScrollbars() so that the view can react to the changes in visible content size due to overflow controls being created/destroyed. In your case, you have overlay scrollbars so the overflow controls do not change the amount of content space or visible content space. Thus there's no reason to react to visibleContentsResized since they haven't actually resized. I suspect you observe that this patch papers over the bug you have since it's called frequently and happens to trigger in the case where you actually do need additional layout. > > You need to figure out what is actually changing and why that is not triggering the layout you need. FrameView::visibleContentsResized() is currently called from this path as a side effect: Page::setPageScaleFactor FrameView::setScrollPosition ScrollView::setScrollPosition ScrollView::updateScrollbars FrameView::visibleContentsResized I agree although it works in current shape it is not necessarily correct. Adding another notification for visible content scale change would make the code more robust.
Comment on attachment 189217 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189217&action=review > Source/WebCore/page/Page.cpp:751 > + view->visibleContentScaleFactorDidChange(); this line will only be reached if the page scale actually changed (see line 726), so why do another round of checks on the visible viewport size? when will that stay the same?
(In reply to comment #42) > (From update of attachment 189217 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189217&action=review > > > Source/WebCore/page/Page.cpp:751 > > + view->visibleContentScaleFactorDidChange(); > > this line will only be reached if the page scale actually changed (see line 726), so why do another round of checks on the visible viewport size? when will that stay the same? FrameView::visibleContentsResized() gets called a lot. Scrolling without changing scrollbar visibility? Still get called. Changing overlay scrollbar visibility? Still get called. I would say we'd better apply the same guard on the whole function, but I don't know what are the possible side effects.
Comment on attachment 189217 [details] Patch Attachment 189217 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16640112 New failing tests: platform/chromium/fast/repaint/relayout-fixed-position-after-scale.html
Created attachment 189224 [details] Patch
Changes from last patch: * Corrects old pinch mode (applyPageScaleFactorInCompositor() == false) compatibility for the viewport-constrained visible content size guard. (In reply to comment #44) > (From update of attachment 189217 [details]) > Attachment 189217 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/16640112 > > New failing tests: > platform/chromium/fast/repaint/relayout-fixed-position-after-scale.html The newly added viewport-constrained visible content size guard didn't consider frame scale factor, thus the test failed to detect the visible content size change.
(In reply to comment #42) > (From update of attachment 189217 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189217&action=review > > > Source/WebCore/page/Page.cpp:751 > > + view->visibleContentScaleFactorDidChange(); > > this line will only be reached if the page scale actually changed (see line 726), so why do another round of checks on the visible viewport size? when will that stay the same? Still waiting for an answer to ^^
(In reply to comment #43) > (In reply to comment #42) > > (From update of attachment 189217 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=189217&action=review > > > > > Source/WebCore/page/Page.cpp:751 > > > + view->visibleContentScaleFactorDidChange(); > > > > this line will only be reached if the page scale actually changed (see line 726), so why do another round of checks on the visible viewport size? when will that stay the same? > > FrameView::visibleContentsResized() gets called a lot. Scrolling without changing scrollbar visibility? Still get called. Changing overlay scrollbar visibility? Still get called. > > I would say we'd better apply the same guard on the whole function, but I don't know what are the possible side effects. I don't really understand this comment. It seems to be a general statement and not an answer to my question re page scale changes.
Comment on attachment 189217 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189217&action=review >>>>> Source/WebCore/page/Page.cpp:751 >>>>> + view->visibleContentScaleFactorDidChange(); >>>> >>>> this line will only be reached if the page scale actually changed (see line 726), so why do another round of checks on the visible viewport size? when will that stay the same? >>> >>> FrameView::visibleContentsResized() gets called a lot. Scrolling without changing scrollbar visibility? Still get called. Changing overlay scrollbar visibility? Still get called. >>> >>> I would say we'd better apply the same guard on the whole function, but I don't know what are the possible side effects. >> >> Still waiting for an answer to ^^ > > I don't really understand this comment. It seems to be a general statement and not an answer to my question re page scale changes. For example, when you scroll a page, although visible content size stays the same, but FrameView::visibleContentsResized() will still get called by ScrollView::updateScrollbars(). Check on the visible viewport size is not really a necessity, but an optimization. I can remove that if you want. If your question is why not just call setViewportConstrainedObjectsNeedLayout() directly, I'd say it is better to avoid code duplication. If fixed-position elements need layout if and only if visible content size changes, then the only place we should do that is in FrameView::visibleContentsResized().
(In reply to comment #49) > (From update of attachment 189217 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189217&action=review > > >>>>> Source/WebCore/page/Page.cpp:751 > >>>>> + view->visibleContentScaleFactorDidChange(); > >>>> > >>>> this line will only be reached if the page scale actually changed (see line 726), so why do another round of checks on the visible viewport size? when will that stay the same? > >>> > >>> FrameView::visibleContentsResized() gets called a lot. Scrolling without changing scrollbar visibility? Still get called. Changing overlay scrollbar visibility? Still get called. > >>> > >>> I would say we'd better apply the same guard on the whole function, but I don't know what are the possible side effects. > >> > >> Still waiting for an answer to ^^ > > > > I don't really understand this comment. It seems to be a general statement and not an answer to my question re page scale changes. > > For example, when you scroll a page, although visible content size stays the same, but FrameView::visibleContentsResized() will still get called by ScrollView::updateScrollbars(). > Check on the visible viewport size is not really a necessity, but an optimization. I can remove that if you want. Please do > > If your question is why not just call setViewportConstrainedObjectsNeedLayout() directly, I'd say it is better to avoid code duplication. If fixed-position elements need layout if and only if visible content size changes, then the only place we should do that is in FrameView::visibleContentsResized(). No, I mean just calling visibleContentsResized() when the page scale factor changed.
Comment on attachment 189224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189224&action=review > Source/WebCore/page/FrameView.cpp:2040 > + LayoutSize viewportConstrainedVisibleContentSize = viewportConstrainedVisibleContentRect().size(); > + viewportConstrainedVisibleContentSize.scale(1 / frame()->frameScaleFactor()); > + if (fixedElementsLayoutRelativeToFrame() && m_lastSeenViewportConstrainedVisibleContentsSize != viewportConstrainedVisibleContentSize) nuke the guards > Source/WebCore/page/FrameView.cpp:2042 > + m_lastSeenViewportConstrainedVisibleContentsSize = viewportConstrainedVisibleContentSize; don't need this > Source/WebCore/page/FrameView.cpp:2882 > +void FrameView::visibleContentScaleFactorDidChange() > +{ > + if (!m_frame || !m_frame->page()) > + return; > + > + if (!m_frame->settings()->applyPageScaleFactorInCompositor() || m_frame != m_frame->page()->mainFrame()) > + return; > + > + visibleContentsResized(); nuke this > Source/WebCore/page/FrameView.h:186 > + void visibleContentScaleFactorDidChange(); don't need this > Source/WebCore/page/FrameView.h:592 > + // For fixedElementsLayoutRelativeToFrame() == true. > + // We don't want unnecessary layout when visible contents size didn't actually change. > + LayoutSize m_lastSeenViewportConstrainedVisibleContentsSize; don't need this > Source/WebCore/page/Page.cpp:751 > + view->visibleContentScaleFactorDidChange(); just view->visibleContentsResized()
Created attachment 189463 [details] Patch
Comment on attachment 189463 [details] Patch Clearing flags on attachment: 189463 Committed r143616: <http://trac.webkit.org/changeset/143616>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 110491
Created attachment 189646 [details] Patch
Comment on attachment 189646 [details] Patch Changes from last patch: * Adding back the fixedElementsLayoutRelativeToFrame() guard. Unnecessary setViewportConstrainedObjectsNeedLayout() causes weird side-effects on chromium-mac.
In https://bugs.webkit.org/show_bug.cgi?id=110491#c5 you said: "Very weird. In this particular test, the frame reserved space for a vertical scrollbar (resizing the RenderView to 785x600) when it doesn't need to. This leaves a gray background (which doesn't look like scrollbar) on the right hand side of the screen, with a shadow near document boundary. I can avoid triggering this in my patch, but it feels like a bug to me." I agree that this is probably and bug and we should find it and fix it.
Created attachment 190632 [details] Patch
Comment on attachment 190632 [details] Patch Changes from last patch: * Revert the change in FrameView::visibleContentsResized It turns out we can't use FrameView::visibleContentsResized after all. This function will be reached during FrameView::layout from this calling chain: FrameView::layout ScrollView::setScrollbarMode ScrollView::updateScrollbars FrameView::visibleContentsResized And this breaks the render tree invariant that when FrameView::layout() ends, no RenderObjects within that frame should need layout. The chromium-mac svg/foreignObject/fixed-position.svg failure is another side-effect of the change.
Comment on attachment 190632 [details] Patch Clearing flags on attachment: 190632 Committed r144260: <http://trac.webkit.org/changeset/144260>