Bug 124260 – Add a new mode where fixed elements don't constrain their positions during a rubber band

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

We should add a new mode (off by default) where fixed elements stay totally fixed during a rubber-band.

[Beth Dakin]

Created attachment 216766 [details]
Patch

[Tim Horton]

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

[Simon Fraser (smfr)]

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.

[Beth Dakin]

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

[Beth Dakin]

Created attachment 216856 [details]
Patch

This should address all of Tim's and Simon's comments.

[WebKit Commit Bot]

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.

[Beth Dakin]

Created attachment 216861 [details]
Patch

This should make stylebot happy.

[Beth Dakin]

Created attachment 216863 [details]
Patch

[Beth Dakin]

Thanks Tim! http://trac.webkit.org/changeset/159300