We should add a new mode (off by default) where fixed elements stay totally fixed during a rubber-band.
Created attachment 216766 [details] Patch
Comment on attachment 216766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216766&action=review > Source/WebCore/page/FrameView.cpp:1546 > +IntSize FrameView::scrollOffsetForFixedPosition(const IntRect& visibleContentRect, const IntSize& totalContentsSize, const IntPoint& scrollPosition, const IntPoint& scrollOrigin, float frameScaleFactor, bool fixedElementsLayoutRelativeToFrame, bool shouldConstrainPosition, int headerHeight, int footerHeight) Wonder if we should enumify shouldConstrainPosition. The arguments to this function are getting out of hand, too :( > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:269 > + return true; The last line of both cases is shared > Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:234 > + m_scrollingStateTree->setHasChangedProperties(true); Weird, I would expect setPropertyChanged to do this, no? > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:109 > + bool m_setShouldConstrainScrollPositionForFixedElements; I don't think this member should have the word 'set' in it? > Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h:129 > + void setNodeShouldConstrainScrollPositionForFixedElements(bool, ScrollingStateScrollingNode*); setShouldConstrainScollPositionForFixedElementsForNode sounds ridiculous but is more consistent with the other functions and the argument order :(
Comment on attachment 216766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216766&action=review >> Source/WebCore/page/FrameView.cpp:1546 >> +IntSize FrameView::scrollOffsetForFixedPosition(const IntRect& visibleContentRect, const IntSize& totalContentsSize, const IntPoint& scrollPosition, const IntPoint& scrollOrigin, float frameScaleFactor, bool fixedElementsLayoutRelativeToFrame, bool shouldConstrainPosition, int headerHeight, int footerHeight) > > Wonder if we should enumify shouldConstrainPosition. The arguments to this function are getting out of hand, too :( I think we should. > Source/WebCore/page/FrameView.cpp:1569 > + shouldConstrainPosition = !renderView->compositor().mainFrameBackingIsTiledWithMargin(); I'd expect this to check frameViewShouldConstrainScrollPositionForFixedElements() or maybe a Setting, rather than tiggering off mainFrameBackingIsTiledWithMargin, which seems more like a consequence of the behavior rather than the main trigger. > Source/WebCore/page/scrolling/ScrollingCoordinator.h:172 > + bool frameViewShouldConstrainScrollPositionForFixedElements(FrameView*); const? > Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h:114 > + bool shouldConstrainScrollPositionForFixedElements() const { return m_shouldConstrainScrollPositionForFixedElements; } > + void setShouldConstrainScrollPositionForFixedElements(bool); This is a bit of a mouthful. How about "fixedElementsMoveOnRubberband()" or something. > Source/WebCore/rendering/RenderLayerCompositor.cpp:2522 > +bool RenderLayerCompositor::mainFrameBackingIsTiledWithMargin() const > +{ > + // FIXME: Implement. https://bugs.webkit.org/show_bug.cgi?id=124216 > + return false; > +} Not sure it's right for RenderLayerCompositor to be the "owner" of the behavior. Seems like a ScrollView/FrameView thing, initialized from a Setting.
(In reply to comment #2) > (From update of attachment 216766 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=216766&action=review > > > Source/WebCore/page/FrameView.cpp:1546 > > +IntSize FrameView::scrollOffsetForFixedPosition(const IntRect& visibleContentRect, const IntSize& totalContentsSize, const IntPoint& scrollPosition, const IntPoint& scrollOrigin, float frameScaleFactor, bool fixedElementsLayoutRelativeToFrame, bool shouldConstrainPosition, int headerHeight, int footerHeight) > > Wonder if we should enumify shouldConstrainPosition. The arguments to this function are getting out of hand, too :( > Okay. > > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:269 > > + return true; > > The last line of both cases is shared > Will share. > > Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:234 > > + m_scrollingStateTree->setHasChangedProperties(true); > > Weird, I would expect setPropertyChanged to do this, no? > Hrm, it doesn't appear to. This seems due for a clean-up. > > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:109 > > + bool m_setShouldConstrainScrollPositionForFixedElements; > > I don't think this member should have the word 'set' in it? > Ah yes, good catch. > > Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h:129 > > + void setNodeShouldConstrainScrollPositionForFixedElements(bool, ScrollingStateScrollingNode*); > > setShouldConstrainScollPositionForFixedElementsForNode sounds ridiculous but is more consistent with the other functions and the argument order :( :-( yeah. Will change.
Created attachment 216856 [details] Patch This should address all of Tim's and Simon's comments.
Attachment 216856 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/page/FrameView.h', u'Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp', u'Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h', u'Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp', u'Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h', u'Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h', u'Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm', u'Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm', u'Source/WebCore/platform/ScrollTypes.h', u'Source/WebCore/rendering/RenderLayerCompositor.cpp']" exit_code: 1 Source/WebCore/page/FrameView.h:236: The parameter name "behaviorForFixed" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 216861 [details] Patch This should make stylebot happy.
Created attachment 216863 [details] Patch
Thanks Tim! http://trac.webkit.org/changeset/159300