⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 246083 in webkit


Ignore:
Timestamp:
Jun 4, 2019, 2:53:57 PM (7 years ago)
Author:
Antti Koivisto
Message:

Sticky positioning is jumpy in many overflow cases
​https://bugs.webkit.org/show_bug.cgi?id=198532
<rdar://problem/51400532>

Reviewed by Simon Fraser.

Source/WebCore:

Tests: scrollingcoordinator/ios/sticky-overflow-no-stacking-context-no-stick-1.html

scrollingcoordinator/ios/sticky-overflow-no-stacking-context-no-stick-2.html
scrollingcoordinator/ios/sticky-overflow-no-stacking-context-stick-1.html
scrollingcoordinator/ios/sticky-overflow-no-stacking-context-stick-2.html
scrollingcoordinator/ios/sticky-overflow-stacking-context-no-stick.html
scrollingcoordinator/ios/sticky-overflow-stacking-context-stick.html

  • page/scrolling/ScrollingTree.cpp:

(WebCore::ScrollingTree::notifyRelatedNodesAfterScrollPositionChange):
(WebCore::ScrollingTree::notifyRelatedNodesRecursive):

Simplify for relatedNodeScrollPositionDidChange removal.

  • page/scrolling/ScrollingTree.h:
  • page/scrolling/ScrollingTreeNode.cpp:

(WebCore::ScrollingTreeNode::relatedNodeScrollPositionDidChange): Deleted.

  • page/scrolling/ScrollingTreeNode.h:
  • page/scrolling/cocoa/ScrollingTreeFixedNode.mm:

(WebCore::ScrollingTreeFixedNode::applyLayerPositions):

  • page/scrolling/cocoa/ScrollingTreePositionedNode.h:
  • page/scrolling/cocoa/ScrollingTreePositionedNode.mm:

(WebCore::ScrollingTreePositionedNode::scrollOffsetSinceLastCommit const):

Factor into a function.

(WebCore::ScrollingTreePositionedNode::applyLayerPositions):
(WebCore::ScrollingTreePositionedNode::relatedNodeScrollPositionDidChange): Deleted.

We can't bail out based on changed node as that makes us compute different positions based on what the change root is.
Since all relatedNodeScrollPositionDidChange functions now always simply call applyLayerPositions we can remove the whole thing.

  • page/scrolling/cocoa/ScrollingTreeStickyNode.mm:

(WebCore::ScrollingTreeStickyNode::applyLayerPositions):

Implement taking into account that the containing scroller may not be our ancestor.

LayoutTests:

  • scrollingcoordinator/ios/sticky-overflow-no-stacking-context-no-stick-1-expected.html: Added.
  • scrollingcoordinator/ios/sticky-overflow-no-stacking-context-no-stick-1.html: Added.
  • scrollingcoordinator/ios/sticky-overflow-no-stacking-context-no-stick-2-expected.html: Added.
  • scrollingcoordinator/ios/sticky-overflow-no-stacking-context-no-stick-2.html: Added.
  • scrollingcoordinator/ios/sticky-overflow-no-stacking-context-stick-1-expected.html: Added.
  • scrollingcoordinator/ios/sticky-overflow-no-stacking-context-stick-1.html: Added.
  • scrollingcoordinator/ios/sticky-overflow-no-stacking-context-stick-2-expected.html: Added.
  • scrollingcoordinator/ios/sticky-overflow-no-stacking-context-stick-2.html: Added.
  • scrollingcoordinator/ios/sticky-overflow-stacking-context-no-stick-expected.html: Added.
  • scrollingcoordinator/ios/sticky-overflow-stacking-context-no-stick.html: Added.
  • scrollingcoordinator/ios/sticky-overflow-stacking-context-stick-expected.html: Added.
  • scrollingcoordinator/ios/sticky-overflow-stacking-context-stick.html: Added.
Location:
trunk
Files:
12 added
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r246076 r246083  
     12019-06-04  Antti Koivisto  <antti@apple.com>
     2
     3        Sticky positioning is jumpy in many overflow cases
     4        https://bugs.webkit.org/show_bug.cgi?id=198532
     5        <rdar://problem/51400532>
     6
     7        Reviewed by Simon Fraser.
     8
     9        * scrollingcoordinator/ios/sticky-overflow-no-stacking-context-no-stick-1-expected.html: Added.
     10        * scrollingcoordinator/ios/sticky-overflow-no-stacking-context-no-stick-1.html: Added.
     11        * scrollingcoordinator/ios/sticky-overflow-no-stacking-context-no-stick-2-expected.html: Added.
     12        * scrollingcoordinator/ios/sticky-overflow-no-stacking-context-no-stick-2.html: Added.
     13        * scrollingcoordinator/ios/sticky-overflow-no-stacking-context-stick-1-expected.html: Added.
     14        * scrollingcoordinator/ios/sticky-overflow-no-stacking-context-stick-1.html: Added.
     15        * scrollingcoordinator/ios/sticky-overflow-no-stacking-context-stick-2-expected.html: Added.
     16        * scrollingcoordinator/ios/sticky-overflow-no-stacking-context-stick-2.html: Added.
     17        * scrollingcoordinator/ios/sticky-overflow-stacking-context-no-stick-expected.html: Added.
     18        * scrollingcoordinator/ios/sticky-overflow-stacking-context-no-stick.html: Added.
     19        * scrollingcoordinator/ios/sticky-overflow-stacking-context-stick-expected.html: Added.
     20        * scrollingcoordinator/ios/sticky-overflow-stacking-context-stick.html: Added.
     21
    1222019-06-04  Takashi Komori  <Takashi.Komori@sony.com>
    223

  • trunk/Source/WebCore/ChangeLog

    r246076 r246083  
     12019-06-04  Antti Koivisto  <antti@apple.com>
     2
     3        Sticky positioning is jumpy in many overflow cases
     4        https://bugs.webkit.org/show_bug.cgi?id=198532
     5        <rdar://problem/51400532>
     6
     7        Reviewed by Simon Fraser.
     8
     9        Tests: scrollingcoordinator/ios/sticky-overflow-no-stacking-context-no-stick-1.html
     10               scrollingcoordinator/ios/sticky-overflow-no-stacking-context-no-stick-2.html
     11               scrollingcoordinator/ios/sticky-overflow-no-stacking-context-stick-1.html
     12               scrollingcoordinator/ios/sticky-overflow-no-stacking-context-stick-2.html
     13               scrollingcoordinator/ios/sticky-overflow-stacking-context-no-stick.html
     14               scrollingcoordinator/ios/sticky-overflow-stacking-context-stick.html
     15
     16        * page/scrolling/ScrollingTree.cpp:
     17        (WebCore::ScrollingTree::notifyRelatedNodesAfterScrollPositionChange):
     18        (WebCore::ScrollingTree::notifyRelatedNodesRecursive):
     19
     20        Simplify for relatedNodeScrollPositionDidChange removal.
     21
     22        * page/scrolling/ScrollingTree.h:
     23        * page/scrolling/ScrollingTreeNode.cpp:
     24        (WebCore::ScrollingTreeNode::relatedNodeScrollPositionDidChange): Deleted.
     25        * page/scrolling/ScrollingTreeNode.h:
     26        * page/scrolling/cocoa/ScrollingTreeFixedNode.mm:
     27        (WebCore::ScrollingTreeFixedNode::applyLayerPositions):
     28        * page/scrolling/cocoa/ScrollingTreePositionedNode.h:
     29        * page/scrolling/cocoa/ScrollingTreePositionedNode.mm:
     30        (WebCore::ScrollingTreePositionedNode::scrollOffsetSinceLastCommit const):
     31
     32        Factor into a function.
     33
     34        (WebCore::ScrollingTreePositionedNode::applyLayerPositions):
     35        (WebCore::ScrollingTreePositionedNode::relatedNodeScrollPositionDidChange): Deleted.
     36
     37        We can't bail out based on changed node as that makes us compute different positions based on what the change root is.
     38        Since all relatedNodeScrollPositionDidChange functions now always simply call applyLayerPositions we can remove the whole thing.
     39
     40        * page/scrolling/cocoa/ScrollingTreeStickyNode.mm:
     41        (WebCore::ScrollingTreeStickyNode::applyLayerPositions):
     42
     43        Implement taking into account that the containing scroller may not be our ancestor.
     44
    1452019-06-04  Takashi Komori  <Takashi.Komori@sony.com>
    246

  • trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp

    r245818 r246083  
    295295        additionalUpdateRoots = overflowRelatedNodes().get(changedNode.scrollingNodeID());
    296296
    297     notifyRelatedNodesRecursive(changedNode, changedNode);
     297    notifyRelatedNodesRecursive(changedNode);
    298298   
    299299    for (auto positionedNodeID : additionalUpdateRoots) {
    300300        auto* positionedNode = nodeForID(positionedNodeID);
    301301        if (positionedNode)
    302             notifyRelatedNodesRecursive(changedNode, *positionedNode);
    303     }
    304 }
    305 
    306 void ScrollingTree::notifyRelatedNodesRecursive(ScrollingTreeScrollingNode& changedNode, ScrollingTreeNode& currNode)
    307 {
    308     currNode.relatedNodeScrollPositionDidChange(changedNode);
    309 
    310     if (!currNode.children())
     302            notifyRelatedNodesRecursive(*positionedNode);
     303    }
     304}
     305
     306void ScrollingTree::notifyRelatedNodesRecursive(ScrollingTreeNode& node)
     307{
     308    node.applyLayerPositions();
     309
     310    if (!node.children())
    311311        return;
    312312
    313     for (auto& child : *currNode.children()) {
     313    for (auto& child : *node.children()) {
    314314        // Never need to cross frame boundaries, since scroll layer adjustments are isolated to each document.
    315315        if (is<ScrollingTreeFrameScrollingNode>(child))
    316316            continue;
    317317
    318         notifyRelatedNodesRecursive(changedNode, *child);
     318        notifyRelatedNodesRecursive(*child);
    319319    }
    320320}

  • trunk/Source/WebCore/page/scrolling/ScrollingTree.h

    r245818 r246083  
    168168    void applyLayerPositionsRecursive(ScrollingTreeNode&);
    169169
    170     void notifyRelatedNodesRecursive(ScrollingTreeScrollingNode& changedNode, ScrollingTreeNode& currNode);
     170    void notifyRelatedNodesRecursive(ScrollingTreeNode&);
    171171
    172172    Lock m_treeMutex; // Protects the scrolling tree.

  • trunk/Source/WebCore/page/scrolling/ScrollingTreeNode.cpp

    r245818 r246083  
    7878}
    7979
    80 void ScrollingTreeNode::relatedNodeScrollPositionDidChange(const ScrollingTreeScrollingNode&)
    81 {
    82     applyLayerPositions();
    83 }
    84 
    8580void ScrollingTreeNode::dumpProperties(TextStream& ts, ScrollingStateTreeAsTextBehavior behavior) const
    8681{

  • trunk/Source/WebCore/page/scrolling/ScrollingTreeNode.h

    r245818 r246083  
    8585    ScrollingTree& scrollingTree() const { return m_scrollingTree; }
    8686
    87     WEBCORE_EXPORT virtual void relatedNodeScrollPositionDidChange(const ScrollingTreeScrollingNode& changedNode);
    88 
    8987    virtual void applyLayerPositions() = 0;
    9088

  • trunk/Source/WebCore/page/scrolling/cocoa/ScrollingTreePositionedNode.h

    r245818 r246083  
    4747    const Vector<ScrollingNodeID>& relatedOverflowScrollingNodes() const { return m_relatedOverflowScrollingNodes; }
    4848
     49    FloatSize scrollOffsetSinceLastCommit() const;
     50
    4951private:
    5052    ScrollingTreePositionedNode(ScrollingTree&, ScrollingNodeID);
    5153
    5254    void commitStateBeforeChildren(const ScrollingStateNode&) override;
    53     void relatedNodeScrollPositionDidChange(const ScrollingTreeScrollingNode& changedNode) override;
    5455
    5556    void applyLayerPositions() override;

  • trunk/Source/WebCore/page/scrolling/cocoa/ScrollingTreePositionedNode.mm

    r245818 r246083  
    7777}
    7878
    79 void ScrollingTreePositionedNode::applyLayerPositions()
     79FloatSize ScrollingTreePositionedNode::scrollOffsetSinceLastCommit() const
    8080{
    81     FloatSize scrollOffsetSinceLastCommit;
     81    FloatSize offset;
    8282    for (auto nodeID : m_relatedOverflowScrollingNodes) {
    8383        if (auto* node = scrollingTree().nodeForID(nodeID)) {
    8484            if (is<ScrollingTreeOverflowScrollingNode>(node)) {
    8585                auto& overflowNode = downcast<ScrollingTreeOverflowScrollingNode>(*node);
    86                 scrollOffsetSinceLastCommit += overflowNode.lastCommittedScrollPosition() - overflowNode.currentScrollPosition();
     86                offset += overflowNode.currentScrollPosition() - overflowNode.lastCommittedScrollPosition();
    8787            }
    8888        }
    8989    }
    90     auto layerOffset = -scrollOffsetSinceLastCommit;
    9190    if (m_constraints.scrollPositioningBehavior() == ScrollPositioningBehavior::Stationary) {
    9291        // Stationary nodes move in the opposite direction.
    93         layerOffset = -layerOffset;
     92        return -offset;
    9493    }
    9594
    96     FloatPoint layerPosition = m_constraints.layerPositionAtLastLayout() - layerOffset;
     95    return offset;
     96}
    9797
    98     LOG_WITH_STREAM(Scrolling, stream << "ScrollingTreePositionedNode " << scrollingNodeID() << " applyLayerPositions: overflow delta " << scrollOffsetSinceLastCommit << " moving layer to " << layerPosition);
     98void ScrollingTreePositionedNode::applyLayerPositions()
     99{
     100    auto offset = scrollOffsetSinceLastCommit();
     101    auto layerPosition = m_constraints.layerPositionAtLastLayout() - offset;
     102
     103    LOG_WITH_STREAM(Scrolling, stream << "ScrollingTreePositionedNode " << scrollingNodeID() << " applyLayerPositions: overflow delta " << offset << " moving layer to " << layerPosition);
    99104
    100105    [m_layer _web_setLayerTopLeftPosition:layerPosition - m_constraints.alignmentOffset()];
    101 }
    102 
    103 void ScrollingTreePositionedNode::relatedNodeScrollPositionDidChange(const ScrollingTreeScrollingNode& changedNode)
    104 {
    105     if (!m_relatedOverflowScrollingNodes.contains(changedNode.scrollingNodeID()))
    106         return;
    107 
    108     applyLayerPositions();
    109106}
    110107

  • trunk/Source/WebCore/page/scrolling/cocoa/ScrollingTreeStickyNode.mm

    r245818 r246083  
    3232#import "ScrollingStateStickyNode.h"
    3333#import "ScrollingTree.h"
     34#import "ScrollingTreeFixedNode.h"
    3435#import "ScrollingTreeFrameScrollingNode.h"
    3536#import "ScrollingTreeOverflowScrollingNode.h"
    … …  
    6869void ScrollingTreeStickyNode::applyLayerPositions()
    6970{
    70     FloatRect constrainingRect;
     71    auto computeLayerPositionForScrollingNode = [&](ScrollingTreeNode& scrollingNode) {
     72        FloatRect constrainingRect;
     73        if (is<ScrollingTreeFrameScrollingNode>(scrollingNode)) {
     74            auto& frameScrollingNode = downcast<ScrollingTreeFrameScrollingNode>(scrollingNode);
     75            constrainingRect = frameScrollingNode.layoutViewport();
     76        } else {
     77            auto& overflowScrollingNode = downcast<ScrollingTreeOverflowScrollingNode>(scrollingNode);
     78            constrainingRect = FloatRect(overflowScrollingNode.currentScrollPosition(), m_constraints.constrainingRectAtLastLayout().size());
     79        }
     80        return m_constraints.layerPositionForConstrainingRect(constrainingRect);
     81    };
    7182
    72     auto* enclosingScrollingNode = parent();
    73     if (is<ScrollingTreeOverflowScrollingNode>(enclosingScrollingNode))
    74         constrainingRect = FloatRect(downcast<ScrollingTreeOverflowScrollingNode>(*enclosingScrollingNode).currentScrollPosition(), m_constraints.constrainingRectAtLastLayout().size());
    75     else if (is<ScrollingTreeFrameScrollingNode>(enclosingScrollingNode))
    76         constrainingRect = downcast<ScrollingTreeFrameScrollingNode>(enclosingScrollingNode)->layoutViewport();
    77     else
    78         return;
     83    auto computeLayerPosition = [&] {
     84        for (auto* ancestor = parent(); ancestor; ancestor = ancestor->parent()) {
     85            if (is<ScrollingTreePositionedNode>(*ancestor)) {
     86                auto& positioningAncestor = downcast<ScrollingTreePositionedNode>(*ancestor);
    7987
    80     FloatPoint layerPosition = m_constraints.layerPositionForConstrainingRect(constrainingRect) - m_constraints.alignmentOffset();
     88                // FIXME: Do we need to do anything for ScrollPositioningBehavior::Stationary?
     89                if (positioningAncestor.scrollPositioningBehavior() == ScrollPositioningBehavior::Moves) {
     90                    if (positioningAncestor.relatedOverflowScrollingNodes().isEmpty())
     91                        break;
     92                    auto overflowNode = scrollingTree().nodeForID(positioningAncestor.relatedOverflowScrollingNodes()[0]);
     93                    if (!overflowNode)
     94                        break;
    8195
    82     LOG_WITH_STREAM(Scrolling, stream << "ScrollingTreeStickyNode " << scrollingNodeID() << " constrainingRect " << constrainingRect << " constrainingRectAtLastLayout " << m_constraints.constrainingRectAtLastLayout() << " last layer pos " << m_constraints.layerPositionAtLastLayout() << " layerPosition " << layerPosition);
     96                    auto position = computeLayerPositionForScrollingNode(*overflowNode);
    8397
    84     [m_layer _web_setLayerTopLeftPosition:layerPosition];
     98                    if (positioningAncestor.layer() == m_layer) {
     99                        // We'll also do the adjustment the positioning node would do.
     100                        position -= positioningAncestor.scrollOffsetSinceLastCommit();
     101                    }
     102                   
     103                    return position;
     104                }
     105            }
     106            if (is<ScrollingTreeScrollingNode>(*ancestor))
     107                return computeLayerPositionForScrollingNode(*ancestor);
     108
     109            if (is<ScrollingTreeFixedNode>(*ancestor) || is<ScrollingTreeStickyNode>(*ancestor)) {
     110                // FIXME: Do we need scrolling tree nodes at all for nested cases?
     111                return m_constraints.layerPositionAtLastLayout();
     112            }
     113        }
     114        ASSERT_NOT_REACHED();
     115        return m_constraints.layerPositionAtLastLayout();
     116    };
     117
     118    auto layerPosition = computeLayerPosition();
     119
     120    LOG_WITH_STREAM(Scrolling, stream << "ScrollingTreeStickyNode " << scrollingNodeID() << " constrainingRectAtLastLayout " << m_constraints.constrainingRectAtLastLayout() << " last layer pos " << m_constraints.layerPositionAtLastLayout() << " layerPosition " << layerPosition);
     121
     122    [m_layer _web_setLayerTopLeftPosition:layerPosition - m_constraints.alignmentOffset()];
    85123}
    86124
Note: See TracChangeset for help on using the changeset viewer.