Bug 124260 - Add a new mode where fixed elements don't constrain their positions during a rubber band
Summary: Add a new mode where fixed elements don't constrain their positions during a ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-12 19:36 PST by Beth Dakin
Modified: 2013-11-14 12:02 PST (History)
16 users (show)

See Also:


Attachments
Patch (19.58 KB, patch)
2013-11-12 19:41 PST, Beth Dakin
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (17.77 KB, patch)
2013-11-13 14:16 PST, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (17.76 KB, patch)
2013-11-13 14:25 PST, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (17.76 KB, patch)
2013-11-13 14:27 PST, Beth Dakin
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2013-11-12 19:36:16 PST
We should add a new mode (off by default) where fixed elements stay totally fixed during a rubber-band.
Comment 1 Beth Dakin 2013-11-12 19:41:10 PST
Created attachment 216766 [details]
Patch
Comment 2 Tim Horton 2013-11-13 03:17:29 PST
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 3 Simon Fraser (smfr) 2013-11-13 12:09:01 PST
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.
Comment 4 Beth Dakin 2013-11-13 13:10:33 PST
(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.
Comment 5 Beth Dakin 2013-11-13 14:16:22 PST
Created attachment 216856 [details]
Patch

This should address all of Tim's and Simon's comments.
Comment 6 WebKit Commit Bot 2013-11-13 14:18:37 PST
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.
Comment 7 Beth Dakin 2013-11-13 14:25:20 PST
Created attachment 216861 [details]
Patch

This should make stylebot happy.
Comment 8 Beth Dakin 2013-11-13 14:27:29 PST
Created attachment 216863 [details]
Patch
Comment 9 Beth Dakin 2013-11-14 12:02:16 PST
Thanks Tim! http://trac.webkit.org/changeset/159300