[Webkit-unassigned] [Bug 105486] Need to re-layout fixed position elements after scale when using settings()->fixedElementsLayoutRelativeToFrame()

[bugzilla-daemon at webkit.org]

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





--- Comment #39 from Tien-Ren Chen <trchen at chromium.org>  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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.