Changeset 170198 in webkit


Ignore:
Timestamp:
Jun 20, 2014 12:16:30 PM (10 years ago)
Author:
Simon Fraser
Message:

[iOS WebKit2] Make -webkit-overflow-scrolling:touch work in iframes (breaks MSWord previews)
​https://bugs.webkit.org/show_bug.cgi?id=134085

Source/WebCore:
<rdar://problem/16440586>

Reviewed by Tim Horton.

When nodes were detached from the scrolling tree, we would previously throw away
all descendant nodes, expecting that they would be re-attached as we walk the compositing
layer tree in RenderLayerCompositor.

However, this doesn't work across frame boundaries; the subframe may never update
its compositing layers again, so would lose all its scrolling nodes.

Fix by having ScrollingStateTree::detachNode() by default set aside subframe nodes
into a hash map. On reattach, we'll look in the hash map and pull out an existing node
(with its children intact) if possible.

Tests: platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-gain-scrolling-ancestor.html

platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-lose-scrolling-ancestor.html
platform/mac-wk2/tiled-drawing/scrolling/frames/remove-coordinated-frame.html

  • page/scrolling/ScrollingStateNode.cpp:

(WebCore::ScrollingStateNode::ScrollingStateNode):

  • page/scrolling/ScrollingStateTree.cpp:

(WebCore::ScrollingStateTree::attachNode):
(WebCore::ScrollingStateTree::detachNode):
(WebCore::ScrollingStateTree::clear):
(WebCore::ScrollingStateTree::removeNodeAndAllDescendants):
(WebCore::ScrollingStateTree::recursiveNodeWillBeRemoved):

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

(WebCore::ScrollingTree::commitNewTreeState): Go back to removing the deleted
nodes from m_nodeMap first.
(WebCore::ScrollingTree::removeDestroyedNodes): There is no need for this to
actually make use of ScrollingTreeNode* any more; the ASSERT(!node->parent())
is bogus because it can fire when whole subtrees are removed, and to clear the
latched node we just need the ID.

Source/WebKit2:
<rdar://problem/16440586>

Reviewed by Tim Horton.

Add some debug-only assertions that check that the number of nodes we encoded is
the expected number.

  • Shared/Scrolling/RemoteScrollingCoordinatorTransaction.cpp:

(WebKit::encodeNodeAndDescendants):
(WebKit::RemoteScrollingCoordinatorTransaction::encode):

LayoutTests:

Reviewed by Tim Horton.

Tests that add and remove a fixed container of a scroll-coordinated iframe.

  • platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-expected.txt:
  • platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-gain-scrolling-ancestor-expected.txt: Added.
  • platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-gain-scrolling-ancestor.html: Added.
  • platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-in-fixed-expected.txt:
  • platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-lose-scrolling-ancestor-expected.txt: Added.
  • platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-lose-scrolling-ancestor.html: Added.
  • platform/mac-wk2/tiled-drawing/scrolling/frames/remove-coordinated-frame-expected.txt: Added.
  • platform/mac-wk2/tiled-drawing/scrolling/frames/remove-coordinated-frame.html: Added.
  • platform/mac-wk2/tiled-drawing/scrolling/frames/resources/doc-with-sticky.html:
Location:
trunk
Files:
6 added
11 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r170145 r170198  
     12014-06-19  Simon Fraser  <simon.fraser@apple.com>
     2
     3        [iOS WebKit2] Make -webkit-overflow-scrolling:touch work in iframes (breaks MSWord previews)
     4        https://bugs.webkit.org/show_bug.cgi?id=134085
     5
     6        Reviewed by Tim Horton.
     7       
     8        Tests that add and remove a fixed container of a scroll-coordinated iframe.
     9
     10        * platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-expected.txt:
     11        * platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-gain-scrolling-ancestor-expected.txt: Added.
     12        * platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-gain-scrolling-ancestor.html: Added.
     13        * platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-in-fixed-expected.txt:
     14        * platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-lose-scrolling-ancestor-expected.txt: Added.
     15        * platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-lose-scrolling-ancestor.html: Added.
     16        * platform/mac-wk2/tiled-drawing/scrolling/frames/remove-coordinated-frame-expected.txt: Added.
     17        * platform/mac-wk2/tiled-drawing/scrolling/frames/remove-coordinated-frame.html: Added.
     18        * platform/mac-wk2/tiled-drawing/scrolling/frames/resources/doc-with-sticky.html:
     19
    1202014-06-19  Mario Sanchez Prada  <mario.prada@samsung.com>
    221

  • trunk/LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-expected.txt

    r169745 r170198  
    55    (Frame scrolling node
    66      (scrollable area size 485 300)
    7       (contents size 485 1868)
     7      (contents size 485 420)
    88      (children 1
    99        (Sticky node
    10           (anchor edges: AnchorEdgeTop )
     10          (anchor edges: AnchorEdgeTop AnchorEdgeBottom)
    1111          (top offset 10.00)
    12           (containing block rect 8.00, 10.00 469.00 x 1850.00)
    13           (sticky box rect 8.00 830.00 100.00 100.00)
     12          (bottom offset 10.00)
     13          (containing block rect 8.00, 8.00 469.00 x 404.00)
     14          (sticky box rect 8.00 312.00 100.00 100.00)
    1415          (constraining rect 0.00 0.00 485.00 300.00)
    15           (sticky offset at last layout 0.00 0.00)
    16           (layer position at last layout 8.00 830.00)
     16          (sticky offset at last layout 0.00 -122.00)
     17          (layer position at last layout 8.00 190.00)
    1718        )
    1819      )

  • trunk/LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-in-fixed-expected.txt

    r169745 r170198  
    1010        (Frame scrolling node
    1111          (scrollable area size 485 300)
    12           (contents size 485 1868)
     12          (contents size 485 420)
    1313          (children 1
    1414            (Sticky node
    15               (anchor edges: AnchorEdgeTop )
     15              (anchor edges: AnchorEdgeTop AnchorEdgeBottom)
    1616              (top offset 10.00)
    17               (containing block rect 8.00, 10.00 469.00 x 1850.00)
    18               (sticky box rect 8.00 830.00 100.00 100.00)
     17              (bottom offset 10.00)
     18              (containing block rect 8.00, 8.00 469.00 x 404.00)
     19              (sticky box rect 8.00 312.00 100.00 100.00)
    1920              (constraining rect 0.00 0.00 485.00 300.00)
    20               (sticky offset at last layout 0.00 0.00)
    21               (layer position at last layout 8.00 830.00)
     21              (sticky offset at last layout 0.00 -122.00)
     22              (layer position at last layout 8.00 190.00)
    2223            )
    2324          )

  • trunk/LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/frames/resources/doc-with-sticky.html

    r169733 r170198  
    1010        }
    1111       
     12        .scrolling {
     13            height: 300px;
     14            width: 400px;
     15            overflow: scroll;
     16            -webkit-overflow-scrolling: touch;
     17            border: 2px solid black;
     18        }
     19       
    1220        .spacer {
    1321            height: 400px;
    … …  
    1826            position: -webkit-sticky;
    1927            top: 10px;
     28            bottom: 10px;
    2029        }
    2130       
    … …  
    2635</head>
    2736<body>
    28     <div class="spacer"></div>
    29     <div class="spacer"></div>
     37    <div class="composited scrolling">
     38        <div class="spacer"></div>
     39        <div class="spacer"></div>
     40        <div class="sticky box"></div>
     41        <div class="spacer"></div>
     42        <div class="spacer"></div>
     43    </div>
    3044    <div class="sticky box"></div>
    31     <div class="spacer"></div>
    32     <div class="spacer"></div>
    33     <div class="composited box"></div>
     45
    3446</body>
    3547</html>

  • trunk/Source/WebCore/ChangeLog

    r170197 r170198  
     12014-06-19  Simon Fraser  <simon.fraser@apple.com>
     2
     3        [iOS WebKit2] Make -webkit-overflow-scrolling:touch work in iframes (breaks MSWord previews)
     4        https://bugs.webkit.org/show_bug.cgi?id=134085
     5        <rdar://problem/16440586>
     6
     7        Reviewed by Tim Horton.
     8       
     9        When nodes were detached from the scrolling tree, we would previously throw away
     10        all descendant nodes, expecting that they would be re-attached as we walk the compositing
     11        layer tree in RenderLayerCompositor.
     12       
     13        However, this doesn't work across frame boundaries; the subframe may never update
     14        its compositing layers again, so would lose all its scrolling nodes.
     15       
     16        Fix by having ScrollingStateTree::detachNode() by default set aside subframe nodes
     17        into a hash map. On reattach, we'll look in the hash map and pull out an existing node
     18        (with its children intact) if possible.
     19
     20        Tests: platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-gain-scrolling-ancestor.html
     21               platform/mac-wk2/tiled-drawing/scrolling/frames/coordinated-frame-lose-scrolling-ancestor.html
     22               platform/mac-wk2/tiled-drawing/scrolling/frames/remove-coordinated-frame.html
     23
     24        * page/scrolling/ScrollingStateNode.cpp:
     25        (WebCore::ScrollingStateNode::ScrollingStateNode):
     26        * page/scrolling/ScrollingStateTree.cpp:
     27        (WebCore::ScrollingStateTree::attachNode):
     28        (WebCore::ScrollingStateTree::detachNode):
     29        (WebCore::ScrollingStateTree::clear):
     30        (WebCore::ScrollingStateTree::removeNodeAndAllDescendants):
     31        (WebCore::ScrollingStateTree::recursiveNodeWillBeRemoved):
     32        * page/scrolling/ScrollingStateTree.h:
     33        * page/scrolling/ScrollingTree.cpp:
     34        (WebCore::ScrollingTree::commitNewTreeState): Go back to removing the deleted
     35        nodes from m_nodeMap first.
     36        (WebCore::ScrollingTree::removeDestroyedNodes): There is no need for this to
     37        actually make use of ScrollingTreeNode* any more; the ASSERT(!node->parent())
     38        is bogus because it can fire when whole subtrees are removed, and to clear the
     39        latched node we just need the ID.
     40
    1412014-06-19  Simon Fraser  <simon.fraser@apple.com>
    242

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

    r170131 r170198  
    4242    , m_changedProperties(0)
    4343    , m_scrollingStateTree(scrollingStateTree)
    44     , m_parent(0)
     44    , m_parent(nullptr)
    4545{
    4646}
    … …  
    5353    , m_changedProperties(stateNode.changedProperties())
    5454    , m_scrollingStateTree(adoptiveTree)
    55     , m_parent(0)
     55    , m_parent(nullptr)
    5656{
    5757    if (hasChangedProperty(ScrollLayer))

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

    r170197 r170198  
    6868}
    6969
     70PassRefPtr<ScrollingStateNode> ScrollingStateTree::createNode(ScrollingNodeType nodeType, ScrollingNodeID nodeID)
     71{
     72    switch (nodeType) {
     73    case FixedNode:
     74        return ScrollingStateFixedNode::create(*this, nodeID);
     75    case StickyNode:
     76        return ScrollingStateStickyNode::create(*this, nodeID);
     77    case FrameScrollingNode:
     78        return ScrollingStateFrameScrollingNode::create(*this, nodeID);
     79    case OverflowScrollingNode:
     80        return ScrollingStateOverflowScrollingNode::create(*this, nodeID);
     81    }
     82    ASSERT_NOT_REACHED();
     83    return nullptr;
     84}
     85
    7086ScrollingNodeID ScrollingStateTree::attachNode(ScrollingNodeType nodeType, ScrollingNodeID newNodeID, ScrollingNodeID parentID)
    7187{
    … …  
    8399
    84100        // The node is being re-parented. To do that, we'll remove it, and then re-create a new node.
    85         removeNodeAndAllDescendants(node);
     101        removeNodeAndAllDescendants(node, SubframeNodeRemoval::Orphan);
    86102    }
    87103
    … …  
    99115            return 0;
    100116
    101         switch (nodeType) {
    102         case FixedNode: {
    103             RefPtr<ScrollingStateFixedNode> fixedNode = ScrollingStateFixedNode::create(*this, newNodeID);
    104             newNode = fixedNode.get();
    105             parent->appendChild(fixedNode.release());
    106             break;
     117        if (nodeType == FrameScrollingNode && parentID) {
     118            if (RefPtr<ScrollingStateNode> orphanedNode = m_orphanedSubframeNodes.take(newNodeID)) {
     119                newNode = orphanedNode.get();
     120                parent->appendChild(orphanedNode.release());
     121            }
    107122        }
    108         case StickyNode: {
    109             RefPtr<ScrollingStateStickyNode> stickyNode = ScrollingStateStickyNode::create(*this, newNodeID);
    110             newNode = stickyNode.get();
    111             parent->appendChild(stickyNode.release());
    112             break;
    113         }
    114         case FrameScrollingNode: {
    115             RefPtr<ScrollingStateFrameScrollingNode> scrollingNode = ScrollingStateFrameScrollingNode::create(*this, newNodeID);
    116             newNode = scrollingNode.get();
    117             parent->appendChild(scrollingNode.release());
    118             break;
    119         }
    120         case OverflowScrollingNode: {
    121             RefPtr<ScrollingStateOverflowScrollingNode> scrollingNode = ScrollingStateOverflowScrollingNode::create(*this, newNodeID);
    122             newNode = scrollingNode.get();
    123             parent->appendChild(scrollingNode.release());
    124             break;
    125         }
     123
     124        if (!newNode) {
     125            RefPtr<ScrollingStateNode> stateNode = createNode(nodeType, newNodeID);
     126            newNode = stateNode.get();
     127            parent->appendChild(stateNode.release());
    126128        }
    127129    }
    … …  
    142144        return;
    143145
    144     removeNodeAndAllDescendants(node);
     146    removeNodeAndAllDescendants(node, SubframeNodeRemoval::Orphan);
    145147}
    146148
    147149void ScrollingStateTree::clear()
    148150{
    149     removeNodeAndAllDescendants(rootStateNode());
     151    if (rootStateNode())
     152        removeNodeAndAllDescendants(rootStateNode());
     153
    150154    ASSERT(m_stateNodeMap.isEmpty());
    151155    m_stateNodeMap.clear();
     156    m_orphanedSubframeNodes.clear();
    152157}
    153158
    154159PassOwnPtr<ScrollingStateTree> ScrollingStateTree::commit(LayerRepresentation::Type preferredLayerRepresentation)
    155160{
     161    m_orphanedSubframeNodes.clear();
     162
    156163    // This function clones and resets the current state tree, but leaves the tree structure intact.
    157164    OwnPtr<ScrollingStateTree> treeStateClone = ScrollingStateTree::create();
    … …  
    179186}
    180187
    181 void ScrollingStateTree::removeNodeAndAllDescendants(ScrollingStateNode* node)
    182 {
    183     if (!node)
    184         return;
    185 
    186     recursiveNodeWillBeRemoved(node);
     188void ScrollingStateTree::removeNodeAndAllDescendants(ScrollingStateNode* node, SubframeNodeRemoval subframeNodeRemoval)
     189{
     190    ScrollingStateNode* parent = node->parent();
     191
     192    recursiveNodeWillBeRemoved(node, subframeNodeRemoval);
    187193
    188194    if (node == m_rootStateNode)
    189195        m_rootStateNode = nullptr;
    190     else if (ScrollingStateNode* parent = node->parent()) {
     196    else if (parent) {
    191197        ASSERT(parent->children() && parent->children()->find(node) != notFound);
    192198        if (auto children = parent->children()) {
    … …  
    198204}
    199205
    200 void ScrollingStateTree::recursiveNodeWillBeRemoved(ScrollingStateNode* currNode)
    201 {
     206void ScrollingStateTree::recursiveNodeWillBeRemoved(ScrollingStateNode* currNode, SubframeNodeRemoval subframeNodeRemoval)
     207{
     208    currNode->setParent(nullptr);
     209    if (subframeNodeRemoval == SubframeNodeRemoval::Orphan && currNode != m_rootStateNode && currNode->isFrameScrollingNode()) {
     210        m_orphanedSubframeNodes.add(currNode->scrollingNodeID(), currNode);
     211        return;
     212    }
     213
    202214    willRemoveNode(currNode);
    203215
    204216    if (auto children = currNode->children()) {
    205217        for (auto& child : *children)
    206             recursiveNodeWillBeRemoved(child.get());
     218            recursiveNodeWillBeRemoved(child.get(), subframeNodeRemoval);
    207219    }
    208220}

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

    r170197 r170198  
    8282    void setRootStateNode(PassRefPtr<ScrollingStateFrameScrollingNode> rootStateNode) { m_rootStateNode = rootStateNode; }
    8383    void addNode(ScrollingStateNode*);
    84     void removeNodeAndAllDescendants(ScrollingStateNode*);
    8584
    86     void recursiveNodeWillBeRemoved(ScrollingStateNode* currNode);
     85    PassRefPtr<ScrollingStateNode> createNode(ScrollingNodeType, ScrollingNodeID);
     86   
     87    enum class SubframeNodeRemoval {
     88        Delete,
     89        Orphan
     90    };
     91    void removeNodeAndAllDescendants(ScrollingStateNode*, SubframeNodeRemoval = SubframeNodeRemoval::Delete);
     92
     93    void recursiveNodeWillBeRemoved(ScrollingStateNode* currNode, SubframeNodeRemoval);
    8794    void willRemoveNode(ScrollingStateNode*);
    8895
    … …  
    9198    RefPtr<ScrollingStateFrameScrollingNode> m_rootStateNode;
    9299    HashSet<ScrollingNodeID> m_nodesRemovedSinceLastCommit;
     100    HashMap<ScrollingNodeID, RefPtr<ScrollingStateNode>> m_orphanedSubframeNodes;
    93101    bool m_hasChangedProperties;
    94102    bool m_hasNewRootStateNode;

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

    r170197 r170198  
    150150    TemporaryChange<bool> changeHandlingProgrammaticScroll(m_isHandlingProgrammaticScroll, scrollRequestIsProgammatic);
    151151
    152     {
    153         OrphanScrollingNodeMap orphanNodes;
    154         updateTreeFromStateNode(rootNode, orphanNodes);
    155     }
    156 
    157152    removeDestroyedNodes(*scrollingStateTree);
     153
     154    OrphanScrollingNodeMap orphanNodes;
     155    updateTreeFromStateNode(rootNode, orphanNodes);
    158156}
    159157
    … …  
    217215void ScrollingTree::removeDestroyedNodes(const ScrollingStateTree& stateTree)
    218216{
    219     for (const auto& removedNode : stateTree.removedNodes()) {
    220         ScrollingTreeNode* node = m_nodeMap.take(removedNode);
    221         if (!node)
    222             continue;
    223 
    224         if (node->scrollingNodeID() == m_latchedNode)
     217    for (const auto& removedNodeID : stateTree.removedNodes()) {
     218        m_nodeMap.remove(removedNodeID);
     219        if (removedNodeID == m_latchedNode)
    225220            clearLatchedNode();
    226 
    227         // We should have unparented this node already via updateTreeFromStateNode().
    228         ASSERT(!node->parent());
    229221    }
    230222}

  • trunk/Source/WebKit2/ChangeLog

    r170197 r170198  
     12014-06-19  Simon Fraser  <simon.fraser@apple.com>
     2
     3        [iOS WebKit2] Make -webkit-overflow-scrolling:touch work in iframes (breaks MSWord previews)
     4        https://bugs.webkit.org/show_bug.cgi?id=134085
     5        <rdar://problem/16440586>
     6
     7        Reviewed by Tim Horton.
     8       
     9        Add some debug-only assertions that check that the number of nodes we encoded is
     10        the expected number.
     11
     12        * Shared/Scrolling/RemoteScrollingCoordinatorTransaction.cpp:
     13        (WebKit::encodeNodeAndDescendants):
     14        (WebKit::RemoteScrollingCoordinatorTransaction::encode):
     15
    1162014-06-19  Simon Fraser  <simon.fraser@apple.com>
    217

  • trunk/Source/WebKit2/Shared/Scrolling/RemoteScrollingCoordinatorTransaction.cpp

    r170197 r170198  
    318318namespace WebKit {
    319319
    320 static void encodeNodeAndDescendants(IPC::ArgumentEncoder& encoder, const ScrollingStateNode& stateNode)
    321 {
     320static void encodeNodeAndDescendants(IPC::ArgumentEncoder& encoder, const ScrollingStateNode& stateNode, int& encodedNodeCount)
     321{
     322    ++encodedNodeCount;
     323
    322324    switch (stateNode.nodeType()) {
    323325    case FrameScrollingNode:
    … …  
    339341
    340342    for (const auto& child : *stateNode.children())
    341         encodeNodeAndDescendants(encoder, *child.get());
     343        encodeNodeAndDescendants(encoder, *child.get(), encodedNodeCount);
    342344}
    343345
    … …  
    353355        encoder << m_scrollingStateTree->hasChangedProperties();
    354356
     357        int numNodesEncoded = 0;
    355358        if (const ScrollingStateNode* rootNode = m_scrollingStateTree->rootStateNode())
    356             encodeNodeAndDescendants(encoder, *rootNode);
    357 
     359            encodeNodeAndDescendants(encoder, *rootNode, numNodesEncoded);
     360
     361        ASSERT_UNUSED(numNodesEncoded, numNodesEncoded == numNodes);
    358362        encoder << m_scrollingStateTree->removedNodes();
    359363    } else
Note: See TracChangeset for help on using the changeset viewer.