Bug 105486

Summary:

Need to re-layout fixed position elements after scale when using settings()->fixedElementsLayoutRelativeToFrame()

Product:

WebKit

Reporter:

Tien-Ren Chen <trchen>

Component:

New Bugs

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aelias, buildbot, dglazkov, eric, jamesr, jchaffraix, ojan.autocc, rniwa, simon.fraser, skyostil, tony, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

110491

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Tien-Ren Chen

Reported 2012-12-19 17:33:09 PST

Need to re-layout fixed position elements after scale when using settings()->fixedElementsLayoutRelativeToFrame()

Attachments
Patch (9.68 KB, patch) 2012-12-19 17:42 PST, Tien-Ren Chen	no flags	Details Formatted Diff Diff
Patch (9.70 KB, patch) 2012-12-20 14:00 PST, Tien-Ren Chen	no flags	Details Formatted Diff Diff
Patch (10.58 KB, patch) 2012-12-20 16:39 PST, Tien-Ren Chen	no flags	Details Formatted Diff Diff
Patch (8.49 KB, patch) 2012-12-20 18:26 PST, Tien-Ren Chen	no flags	Details Formatted Diff Diff
Patch (9.88 KB, patch) 2012-12-20 19:38 PST, Tien-Ren Chen	no flags	Details Formatted Diff Diff
Patch (5.58 KB, patch) 2013-01-28 19:21 PST, Tien-Ren Chen	no flags	Details Formatted Diff Diff
Patch (5.74 KB, patch) 2013-01-29 16:03 PST, Tien-Ren Chen	no flags	Details Formatted Diff Diff
Patch (5.79 KB, patch) 2013-01-30 17:38 PST, Tien-Ren Chen	no flags	Details Formatted Diff Diff
Patch (5.79 KB, patch) 2013-01-30 17:39 PST, Tien-Ren Chen	no flags	Details Formatted Diff Diff
Patch (5.71 KB, patch) 2013-02-04 18:48 PST, Tien-Ren Chen	no flags	Details Formatted Diff Diff
Patch (5.70 KB, patch) 2013-02-04 18:53 PST, Tien-Ren Chen	no flags	Details Formatted Diff Diff
Patch (6.38 KB, patch) 2013-02-05 13:13 PST, Tien-Ren Chen	no flags	Details Formatted Diff Diff
Patch (9.10 KB, patch) 2013-02-19 18:05 PST, Tien-Ren Chen	no flags	Details Formatted Diff Diff
Patch (9.18 KB, patch) 2013-02-19 19:59 PST, Tien-Ren Chen	no flags	Details Formatted Diff Diff
Patch (7.92 KB, patch) 2013-02-20 23:24 PST, Tien-Ren Chen	no flags	Details Formatted Diff Diff
Patch (7.95 KB, patch) 2013-02-21 17:36 PST, Tien-Ren Chen	no flags	Details Formatted Diff Diff
Patch (7.41 KB, patch) 2013-02-27 18:03 PST, Tien-Ren Chen	no flags	Details Formatted Diff Diff
Show Obsolete (16) View All Add attachment proposed patch, testcase, etc.

Tien-Ren Chen

Comment 1 2012-12-19 17:42:29 PST

Created attachment 180254 [details] Patch

Sami Kyöstilä

Comment 2 2012-12-20 03:02:52 PST

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.

Sami Kyöstilä

Comment 3 2012-12-20 03:06:07 PST

Oops, hit submit too soon. I think this fixes the problem nicely -- thanks Tien-Ren.

Tien-Ren Chen

Comment 4 2012-12-20 14:00:41 PST

Created attachment 180405 [details] Patch

Tien-Ren Chen

Comment 5 2012-12-20 14:01:32 PST

(In reply to comment #4) > Created an attachment (id=180405) [details] > Patch Fixed the nits Sami mentioned. Thanks!

Simon Fraser (smfr)

Comment 6 2012-12-20 14:32:01 PST

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.

Tien-Ren Chen

Comment 7 2012-12-20 14:37:34 PST

(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?

Tien-Ren Chen

Comment 8 2012-12-20 16:39:41 PST

Created attachment 180440 [details] Patch

Simon Fraser (smfr)

Comment 9 2012-12-20 17:39:23 PST

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?

Tien-Ren Chen

Comment 10 2012-12-20 18:26:24 PST

Created attachment 180454 [details] Patch

Simon Fraser (smfr)

Comment 11 2012-12-20 19:21:35 PST

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.

Tien-Ren Chen

Comment 12 2012-12-20 19:31:52 PST

(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?

Tien-Ren Chen

Comment 13 2012-12-20 19:38:03 PST

Created attachment 180462 [details] Patch

Simon Fraser (smfr)

Comment 14 2013-01-02 21:01:00 PST

(In reply to comment #12) > Let's just add a Skip in TestExpectations? Better to just list it as an image failure.

Tien-Ren Chen

Comment 15 2013-01-28 19:21:39 PST

Created attachment 185133 [details] Patch

Tien-Ren Chen

Comment 16 2013-01-28 19:25:07 PST

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.

Sami Kyöstilä

Comment 17 2013-01-29 05:35:30 PST

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.

Build Bot

Comment 18 2013-01-29 09:21:34 PST

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

Tien-Ren Chen

Comment 19 2013-01-29 16:03:09 PST

Created attachment 185330 [details] Patch

Tien-Ren Chen

Comment 20 2013-01-29 16:06:14 PST

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.

Tien-Ren Chen

Comment 21 2013-01-30 17:38:29 PST

Created attachment 185636 [details] Patch

Tien-Ren Chen

Comment 22 2013-01-30 17:39:31 PST

Created attachment 185637 [details] Patch

Tien-Ren Chen

Comment 23 2013-01-30 17:40:33 PST

Comment on attachment 185637 [details] Patch Use document.body.offsetTop magic spell as suggested by Sami. I think this patch is ready. PTAL, thanks!

Tien-Ren Chen

Comment 24 2013-02-01 13:40:52 PST

Hello Simon, Do you have time to take a look? I hope the patch doesn't look too sketchy to you. :)

Tien-Ren Chen

Comment 25 2013-02-04 18:48:21 PST

Created attachment 186521 [details] Patch

Tien-Ren Chen

Comment 26 2013-02-04 18:53:14 PST

Created attachment 186522 [details] Patch

Tien-Ren Chen

Comment 27 2013-02-04 18:53:40 PST

Comment on attachment 186522 [details] Patch Last one was a bad rebase. This one is correct.

Alexandre Elias

Comment 28 2013-02-04 18:55:39 PST

Looks good to me, thanks.

Tony Chang

Comment 29 2013-02-05 10:10:39 PST

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?

Tien-Ren Chen

Comment 30 2013-02-05 11:33:50 PST

(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.

Tony Chang

Comment 31 2013-02-05 12:29:00 PST

(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.

Simon Fraser (smfr)

Comment 32 2013-02-05 12:57:54 PST

(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.

Tien-Ren Chen

Comment 33 2013-02-05 13:13:01 PST

Created attachment 186688 [details] Patch

Tien-Ren Chen

Comment 34 2013-02-05 13:13:52 PST

Comment on attachment 186688 [details] Patch Revised ChangeLog as suggested.

Tien-Ren Chen

Comment 35 2013-02-13 20:23:49 PST

Ping. Can I haz a r+ please? :3

Alexandre Elias

Comment 36 2013-02-13 20:25:29 PST

Could you try moving the call into setPageScaleFactor()? This seems like a more indirect way of achieving the same thing.

Tien-Ren Chen

Comment 37 2013-02-13 22:00:57 PST

(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.

James Robinson

Comment 38 2013-02-14 10:42:57 PST

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.

Tien-Ren Chen

Comment 39 2013-02-14 11:16:14 PST

(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.

Tien-Ren Chen

Comment 40 2013-02-19 18:05:35 PST

Created attachment 189217 [details] Patch

Tien-Ren Chen

Comment 41 2013-02-19 18:14:48 PST

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.

James Robinson

Comment 42 2013-02-19 18:14:51 PST

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?

Tien-Ren Chen

Comment 43 2013-02-19 18:17:05 PST

(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.

WebKit Review Bot

Comment 44 2013-02-19 19:13:06 PST

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

Tien-Ren Chen

Comment 45 2013-02-19 19:59:59 PST

Created attachment 189224 [details] Patch

Tien-Ren Chen

Comment 46 2013-02-19 20:03:38 PST

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.

James Robinson

Comment 47 2013-02-20 00:43:18 PST

James Robinson

Comment 48 2013-02-20 00:44:34 PST

(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.

Tien-Ren Chen

Comment 49 2013-02-20 01:30:17 PST

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().

James Robinson

Comment 50 2013-02-20 17:58:43 PST

(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.

James Robinson

Comment 51 2013-02-20 17:59:55 PST

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()

Tien-Ren Chen

Comment 52 2013-02-20 23:24:11 PST

Created attachment 189463 [details] Patch

WebKit Review Bot

Comment 53 2013-02-21 09:33:18 PST

Comment on attachment 189463 [details] Patch Clearing flags on attachment: 189463 Committed r143616: <http://trac.webkit.org/changeset/143616>

WebKit Review Bot

Comment 54 2013-02-21 09:33:24 PST

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 55 2013-02-21 10:46:15 PST

Re-opened since this is blocked by bug 110491

Tien-Ren Chen

Comment 56 2013-02-21 17:36:41 PST

Created attachment 189646 [details] Patch

Tien-Ren Chen

Comment 57 2013-02-21 17:38:08 PST

Comment on attachment 189646 [details] Patch Changes from last patch: * Adding back the fixedElementsLayoutRelativeToFrame() guard. Unnecessary setViewportConstrainedObjectsNeedLayout() causes weird side-effects on chromium-mac.

James Robinson

Comment 58 2013-02-21 17:45:47 PST

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.

Tien-Ren Chen

Comment 59 2013-02-27 18:03:56 PST

Created attachment 190632 [details] Patch

Tien-Ren Chen

Comment 60 2013-02-27 18:09:02 PST

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.

WebKit Review Bot

Comment 61 2013-02-27 21:07:26 PST

Comment on attachment 190632 [details] Patch Clearing flags on attachment: 190632 Committed r144260: <http://trac.webkit.org/changeset/144260>

WebKit Review Bot

Comment 62 2013-02-27 21:07:34 PST

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.