REGRESSION (r133807): Sticky-position review bar on bugzilla review page is jumpy

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Code review toolbar doesn't stick to the bottom while scrolling.

I suspect that this didn't start with <http://trac.webkit.org/r133807> immediately, but became observable with <http://trac.webkit.org/r136587>, because I first noticed this issue today.

[Alexey Proskuryakov]

<rdar://problem/12827187>

[Adam Barth]

Is this a bug in the code review tool or a bug with position:sticky in WebKit?

[Alexey Proskuryakov]

It's definitely a bug (regression) in WebKit. But it can also be seen as a bug in the review tool that it uses a feature that's broken in WebKit.

[Tony Chang]

Alexey: Can you use inspector and see what the CSS value for 'position' is on #toolbar?  Also, this is in Safari, right?

[Simon Fraser (smfr)]

The position:sticky element isn't getting a compositing layer, because I don't think we've hooked that up yet.

[Ojan Vafai]

(In reply to comment #5)
> The position:sticky element isn't getting a compositing layer, because I don't think we've hooked that up yet.

If the feature doesn't work, maybe it shouldn't be enabled? It works fine for me in my non-compositing enabled world.

[Simon Fraser (smfr)]

It's only jumpy on Mac in threaded scrolling mode. It's work in progress; no need to disable anything.

[Ojan Vafai]

(In reply to comment #7)
> It's only jumpy on Mac in threaded scrolling mode. It's work in progress; no need to disable anything.

Is there a way to feature detect this?  Should I just leave it as is until the sticky bug gets fixed?

[Simon Fraser (smfr)]

Ojan, have you actually seen the problem?

[Beth Dakin]

(In reply to comment #8)
> (In reply to comment #7)
> > It's only jumpy on Mac in threaded scrolling mode. It's work in progress; no need to disable anything.
> 
> Is there a way to feature detect this?  Should I just leave it as is until the sticky bug gets fixed?

There is not a way to feature detect this. And yes, I think you should leave it as it is.

The best way to resolve this will be to get position:sticky things scrolling on the scrolling thread. I'm eager to do that anyway.

[Alexey Proskuryakov]

This is super annoying when reviewing patches. I hope that this can be addressed one way or another reasonably soon.

[Alexey Proskuryakov]

This has been mostly fixed in bug 105245. One remaining case is jumpiness with rubberbanding, not sure what's the best way to track this. Is there a separate existing bug tracking the remaining issue?

[Simon Fraser (smfr)]

(In reply to comment #12)
> This has been mostly fixed in bug 105245. One remaining case is jumpiness with rubberbanding, not sure what's the best way to track this. Is there a separate existing bug tracking the remaining issue?

No. I filed bug 106946.

[Alexey Proskuryakov]

The remaining issue is that the bar hops when you click on it.

[Simon Fraser (smfr)]

I think this is caused by the order of things when we commit new tree state. We first update the scroll position of the root node, but this triggers a scroll position change on the sticky node child:

* thread #13: tid = 0x3c03, 0x0000000104c2737b WebCore`WebCore::ScrollingTreeStickyNode::parentScrollPositionDidChange(WebCore::IntRect const&, WebCore::FloatSize const&) + 491 at ScrollingTreeStickyNode.mm:78, stop reason = step over
    frame #0: 0x0000000104c2737b WebCore`WebCore::ScrollingTreeStickyNode::parentScrollPositionDidChange(WebCore::IntRect const&, WebCore::FloatSize const&) + 491 at ScrollingTreeStickyNode.mm:78
    frame #1: 0x0000000104c2530f WebCore`WebCore::ScrollingTreeScrollingNodeMac::setScrollLayerPosition(WebCore::IntPoint const&) + 927 at ScrollingTreeScrollingNodeMac.mm:311
    frame #2: 0x0000000104c24683 WebCore`WebCore::ScrollingTreeScrollingNodeMac::setScrollPositionWithoutContentEdgeConstraints(WebCore::IntPoint const&) + 131 at ScrollingTreeScrollingNodeMac.mm:290
    frame #3: 0x0000000104c245c8 WebCore`WebCore::ScrollingTreeScrollingNodeMac::setScrollPosition(WebCore::IntPoint const&) + 120 at ScrollingTreeScrollingNodeMac.mm:274
    frame #4: 0x0000000104c22e69 WebCore`WebCore::ScrollingTreeScrollingNodeMac::update(WebCore::ScrollingStateNode*) + 249 at ScrollingTreeScrollingNodeMac.mm:80
    frame #5: 0x0000000104c1bea3 WebCore`WebCore::ScrollingTree::updateTreeFromStateNode(WebCore::ScrollingStateNode*) + 243 at ScrollingTree.cpp:156
    frame #6: 0x0000000104c1bcca WebCore`WebCore::ScrollingTree::commitNewTreeState(WTF::PassOwnPtr<WebCore::ScrollingStateTree>) + 602 at ScrollingTree.cpp:145
    frame #7: 0x0000000104c0f9a6 WebCore`WTF::FunctionWrapper<void (WebCore::ScrollingTree::*)(WTF::PassOwnPtr<WebCore::ScrollingStateTree>)>::operator()(WebCore::ScrollingTree*, WTF::PassOwnPtr<WebCore::ScrollingStateTree>) + 150 at Functional.h:246
    frame #8: 0x0000000104c0f8d5 WebCore`WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (WebCore::ScrollingTree::*)(WTF::PassOwnPtr<WebCore::ScrollingStateTree>)>, void (WebCore::ScrollingTree*, WTF::PassOwnPtr<WebCore::ScrollingStateTree>)>::operator()() + 101 at Functional.h:522
    frame #9: 0x0000000104ba07b9 WebCore`WTF::Function<void ()>::operator()() const + 137 at Functional.h:704
    frame #10: 0x0000000104c19fda WebCore`WebCore::ScrollingThread::dispatchFunctionsFromScrollingThread() + 218 at ScrollingThread.cpp:117
    frame #11: 0x0000000104c1b13a WebCore`WebCore::ScrollingThread::threadRunLoopSourceCallback() + 74 at ScrollingThreadMac.mm:67
    frame #12: 0x0000000104c1b095 WebCore`WebCore::ScrollingThread::threadRunLoopSourceCallback(void*) + 21 at ScrollingThreadMac.mm:61
    frame #13: 0x00007fff85bb3101 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
    frame #14: 0x00007fff85bb2a25 CoreFoundation`__CFRunLoopDoSources0 + 245
    frame #15: 0x00007fff85bd5dc5 CoreFoundation`__CFRunLoopRun + 789
    frame #16: 0x00007fff85bd56b2 CoreFoundation`CFRunLoopRunSpecific + 290
    frame #17: 0x00007fff85be4371 CoreFoundation`CFRunLoopRun + 97
    frame #18: 0x0000000104c1b037 WebCore`WebCore::ScrollingThread::initializeRunLoop() + 503 at ScrollingThreadMac.mm:50
    frame #19: 0x0000000104c19ef5 WebCore`WebCore::ScrollingThread::threadBody() + 21 at ScrollingThread.cpp:102
    frame #20: 0x0000000104c19ed5 WebCore`WebCore::ScrollingThread::threadCallback(void*) + 21 at ScrollingThread.cpp:97
    frame #21: 0x0000000102911200 JavaScriptCore`threadEntryPoint + 144 at Threading.cpp:69
    frame #22: 0x0000000102911bf8 JavaScriptCore`wtfThreadEntryPoint + 104 at ThreadingPthreads.cpp:196
    frame #23: 0x00007fff8715b742 libsystem_c.dylib`_pthread_start + 327
    frame #24: 0x00007fff87148181 libsystem_c.dylib`thread_start + 13

However at this point the sticky node has stale viewport constraints; those aren't updated until we're done updating the root node.

[Simon Fraser (smfr)]

Fixing the commit ordering fixes this. In general, I think a tree update should never end up touching layer properties; they should already be up-to-date because the main thread telling us that stuff changed.

So ScrollingTreeScrollingNodeMac::update() should updating the scroll position, not setting it, and this should not recur into children. It's unclear to me whether ScrollingTree's m_mainFrameScrollPosition needs to be updated (why is this on the tree and not the root node?).

[Simon Fraser (smfr)]

Hacky patch:

diff --git a/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm b/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm
index 300a12d28d13ab44ff47a5ad76d42bae9d25ae37..df0f1166f3d03c261acbf4cea4e116c98e06a8fa 100644
--- a/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm
+++ b/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm
@@ -440,6 +440,7 @@ void ScrollingCoordinatorMac::updateViewportConstrainedNode(ScrollingNodeID node
         break;
     }
     }
+    scheduleTreeStateCommit();
 }
 
 void ScrollingCoordinatorMac::scheduleTreeStateCommit()
diff --git a/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.h b/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.h
index fe1c2420b9109500d1fc4bbede85d3dc01cc482a..192e4207108be60b615f3d15a04e95af692b6f58 100644
--- a/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.h
+++ b/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.h
@@ -72,6 +72,7 @@ private:
     void scrollBy(const IntSize&);
     void scrollByWithoutContentEdgeConstraints(const IntSize&);
 
+    void updateScrollPosition(const IntPoint&);
     void updateMainFramePinState(const IntPoint& scrollPosition);
 
     void logExposedUnfilledArea();
diff --git a/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm b/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm
index 663c7341b68f8baf911f4620f61f172e59d0b291..8acddd12fbb54220291cf931fed58366c66d2675 100644
--- a/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm
+++ b/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm
@@ -77,7 +77,7 @@ void ScrollingTreeScrollingNodeMac::update(ScrollingStateNode* stateNode)
         m_counterScrollingLayer = state->counterScrollingPlatformLayer();
 
     if (state->changedProperties() & ScrollingStateScrollingNode::RequestedScrollPosition)
-        setScrollPosition(state->requestedScrollPosition());
+        updateScrollPosition(state->requestedScrollPosition());
 
     if (state->scrollLayerDidChange() || state->changedProperties() & (ScrollingStateScrollingNode::ContentsSize | ScrollingStateScrollingNode::ViewportRect))
         updateMainFramePinState(scrollPosition());
@@ -336,6 +336,20 @@ void ScrollingTreeScrollingNodeMac::scrollByWithoutContentEdgeConstraints(const
     setScrollPositionWithoutContentEdgeConstraints(scrollPosition() + offset);
 }
 
+void ScrollingTreeScrollingNodeMac::updateScrollPosition(const IntPoint& scrollPosition)
+{
+    IntPoint newScrollPosition = scrollPosition;
+    newScrollPosition = newScrollPosition.shrunkTo(maximumScrollPosition());
+    newScrollPosition = newScrollPosition.expandedTo(minimumScrollPosition());
+
+    updateMainFramePinState(newScrollPosition);
+
+//    {
+//        MutexLocker lock(m_mutex);
+//        m_mainFrameScrollPosition = newScrollPosition;
+//    }
+}
+
 void ScrollingTreeScrollingNodeMac::updateMainFramePinState(const IntPoint& scrollPosition)
 {
     bool pinnedToTheLeft = scrollPosition.x() <= minimumScrollPosition().x();

[Simon Fraser (smfr)]

*** Bug 109347 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

Created attachment 187683 [details]
Patch

[Tim Horton]

Comment on attachment 187683 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187683&action=review

> Source/WebCore/ChangeLog:35
> +        scroll position request, but if only the viewport constaints change, we also need

constraints.

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/142520