Attachment 373235 Details for Bug 198217 – Approach #1

[patch] Approach #1

bug-198217-20190701080206.patch (text/plain), 12.76 KB, created by Wenson Hsieh on 2019-07-01 08:02:07 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Wenson Hsieh

Created: 2019-07-01 08:02:07 PDT

Size: 12.76 KB

patch

obsolete

>Subversion Revision: 246947
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 5e5bacdb18fb9788f8cfb0c2a6a97f5efd1a8c54..fd489edc41a882c65022dab03eb5903f54526c2b 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,21 @@
>+2019-07-01  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        iOS: REGRESSION(async scroll): Caret doesn't scroll when scrolling textarea
>+        https://bugs.webkit.org/show_bug.cgi?id=198217
>+        <rdar://problem/51097296>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add a ScrollingLayerPositionAction argument to ScrollingTreeScrollingNode::wasScrolledByDelegatedScrolling, and
>+        avoid bailing early in the case where ScrollingLayerPositionAction::Set is used. See the WebKit ChangeLog for
>+        more detail.
>+
>+        Test: editing/selection/ios/update-selection-after-overflow-scroll.html
>+
>+        * page/scrolling/ScrollingTreeScrollingNode.cpp:
>+        (WebCore::ScrollingTreeScrollingNode::wasScrolledByDelegatedScrolling):
>+        * page/scrolling/ScrollingTreeScrollingNode.h:
>+
> 2019-06-28  Zalan Bujtas  <zalan@apple.com>
> 
>         [Text autosizing][iPadOS] bing.com is hard to read even with boosted text because of the line height
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 8ead54ff6f4abefb58a286e19b91931d0abf45a2..cb423b7feab9ce4cca9f3e51581299f4cccce26f 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,26 @@
>+2019-07-01  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        iOS: REGRESSION(async scroll): Caret doesn't scroll when scrolling textarea
>+        https://bugs.webkit.org/show_bug.cgi?id=198217
>+        <rdar://problem/51097296>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        In iOS 12, when scrolling a text selection in an fast-scrolling container, editor state updates are scheduled
>+        under AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll after the end of the scrolling gesture,
>+        when the scrolling layer action is ScrollingLayerPositionAction::Set. This is no longer the case in iOS 13,
>+        because we now bail in ScrollingTreeScrollingNode::wasScrolledByDelegatedScrolling after scroll deceleration
>+        finishes since the scroll position didn't end up changing. Additionally, we no longer use
>+        ScrollingLayerPositionAction::Set in the case where scrolling finished decelerating, since
>+        ScrollingTreeScrollingNodeDelegateIOS::scrollViewDidScroll no longer uses to value of inUserInteraction to
>+        determine whether to Set or Sync scrolling layer positions.
>+
>+        To restore iOS 12 behavior, ensure that we send a scrolling tree update using ScrollingLayerPositionAction::Set
>+        after scrolling ends.
>+
>+        * UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:
>+        (WebKit::ScrollingTreeScrollingNodeDelegateIOS::scrollViewDidScroll):
>+
> 2019-06-28  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed build fix attempt after r246928.
>diff --git a/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp b/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp
>index 51b356a941f713a314fc0182ffdc4c4a60333194..44d6e8c667b66be9db33ba0f822764e10ee6f74e 100644
>--- a/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp
>+++ b/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp
>@@ -194,10 +194,10 @@ void ScrollingTreeScrollingNode::applyLayerPositions()
>     repositionRelatedLayers();
> }
> 
>-void ScrollingTreeScrollingNode::wasScrolledByDelegatedScrolling(const FloatPoint& position, Optional<FloatRect> overrideLayoutViewport)
>+void ScrollingTreeScrollingNode::wasScrolledByDelegatedScrolling(const FloatPoint& position, Optional<FloatRect> overrideLayoutViewport, ScrollingLayerPositionAction scrollingLayerPositionAction)
> {
>     bool scrollPositionChanged = !scrollPositionAndLayoutViewportMatch(position, overrideLayoutViewport);
>-    if (!scrollPositionChanged)
>+    if (!scrollPositionChanged && scrollingLayerPositionAction != ScrollingLayerPositionAction::Set)
>         return;
> 
>     m_currentScrollPosition = adjustedScrollPosition(position, ScrollPositionClamp::None);
>@@ -206,7 +206,7 @@ void ScrollingTreeScrollingNode::wasScrolledByDelegatedScrolling(const FloatPoin
>     repositionRelatedLayers();
> 
>     scrollingTree().notifyRelatedNodesAfterScrollPositionChange(*this);
>-    scrollingTree().scrollingTreeNodeDidScroll(*this);
>+    scrollingTree().scrollingTreeNodeDidScroll(*this, scrollingLayerPositionAction);
>     scrollingTree().didScrollByDelegatedScrolling();
> }
> 
>diff --git a/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h b/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h
>index b22ffa79225a929cb069a6730f1bed7918d32409..7e5e45ca3297eec0dbf1ed67797eac9e37e58ba0 100644
>--- a/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h
>+++ b/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h
>@@ -63,7 +63,7 @@ public:
>     void scrollTo(const FloatPoint&, ScrollType = ScrollType::User, ScrollPositionClamp = ScrollPositionClamp::ToContentEdges);
>     void scrollBy(const FloatSize&, ScrollPositionClamp = ScrollPositionClamp::ToContentEdges);
> 
>-    void wasScrolledByDelegatedScrolling(const FloatPoint& position, Optional<FloatRect> overrideLayoutViewport = { });
>+    void wasScrolledByDelegatedScrolling(const FloatPoint& position, Optional<FloatRect> overrideLayoutViewport = { }, ScrollingLayerPositionAction = ScrollingLayerPositionAction::Sync);
>     
>     const FloatSize& scrollableAreaSize() const { return m_scrollableAreaSize; }
>     const FloatSize& totalContentsSize() const { return m_totalContentsSize; }
>diff --git a/Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm b/Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm
>index 5ea0382d4c7e4514ca87fa81460f2a4a471c0d85..1e55e4ef94f3ade6d07dfb787bc1b56aa8fd4e26 100644
>--- a/Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm
>+++ b/Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm
>@@ -327,7 +327,7 @@ void ScrollingTreeScrollingNodeDelegateIOS::scrollViewDidScroll(const FloatPoint
>         return;
> 
>     auto scrollPosition = ScrollableArea::scrollPositionFromOffset(scrollOffset, toFloatSize(scrollOrigin()));
>-    scrollingNode().wasScrolledByDelegatedScrolling(scrollPosition);
>+    scrollingNode().wasScrolledByDelegatedScrolling(scrollPosition, { }, inUserInteraction ? ScrollingLayerPositionAction::Sync : ScrollingLayerPositionAction::Set);
> }
> 
> void ScrollingTreeScrollingNodeDelegateIOS::currentSnapPointIndicesDidChange(unsigned horizontal, unsigned vertical) const
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 125350d262ea011a896992ab26f7f9d6ba6c6b2e..a1e1887e00f8adabdfcda3abe5bc43806425998e 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2019-07-01  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        iOS: REGRESSION(async scroll): Caret doesn't scroll when scrolling textarea
>+        https://bugs.webkit.org/show_bug.cgi?id=198217
>+        <rdar://problem/51097296>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add a new layout test to check that the text selection views are updated after scrolling in a fast overflow
>+        scrolling container.
>+
>+        * editing/selection/ios/update-selection-after-overflow-scroll-expected.txt: Added.
>+        * editing/selection/ios/update-selection-after-overflow-scroll.html: Added.
>+
> 2019-06-28  Zalan Bujtas  <zalan@apple.com>
> 
>         [Text autosizing][iPadOS] bing.com is hard to read even with boosted text because of the line height
>diff --git a/LayoutTests/editing/selection/ios/update-selection-after-overflow-scroll-expected.txt b/LayoutTests/editing/selection/ios/update-selection-after-overflow-scroll-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..c0f3da8c44bb2a05501675669f506aa986747e4c
>--- /dev/null
>+++ b/LayoutTests/editing/selection/ios/update-selection-after-overflow-scroll-expected.txt
>@@ -0,0 +1,24 @@
>+This test verifies that a selection in a fast scrollable area is kept up to date after scrolling. To test manually, tap the button and scroll the editable area down; the selection move to account for the new scroll position.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+PASS selectionRectsBefore[0].top is 108
>+PASS selectionRectsBefore[0].width is 320
>+PASS selectionRectsBefore[0].left is 0
>+PASS selectionRectsBefore[0].height is 29
>+PASS selectionRectsBefore[1].top is 137
>+PASS selectionRectsBefore[1].width is 172
>+PASS selectionRectsBefore[1].left is 0
>+PASS selectionRectsBefore[1].height is 30
>+PASS selectionRectsAfter[0].top is 58
>+PASS selectionRectsAfter[0].width is 320
>+PASS selectionRectsAfter[0].left is 0
>+PASS selectionRectsAfter[0].height is 29
>+PASS selectionRectsAfter[1].top is 87
>+PASS selectionRectsAfter[1].width is 172
>+PASS selectionRectsAfter[1].left is 0
>+PASS selectionRectsAfter[1].height is 30
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/editing/selection/ios/update-selection-after-overflow-scroll.html b/LayoutTests/editing/selection/ios/update-selection-after-overflow-scroll.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..a2bdb6d1046708d0c8ef24038b9879dae7e4cfc7
>--- /dev/null
>+++ b/LayoutTests/editing/selection/ios/update-selection-after-overflow-scroll.html
>@@ -0,0 +1,103 @@
>+<!DOCTYPE html> 
>+<html>
>+<head>
>+<script src="../../../resources/js-test.js"></script>
>+<script src="../../../resources/ui-helper.js"></script>
>+<meta name=viewport content="width=device-width, initial-scale=1">
>+<style>
>+body, html {
>+    width: 100%;
>+    height: 100%;
>+    margin: 0;
>+}
>+
>+#editor {
>+    font-size: 24px;
>+    width: 320px;
>+    height: 200px;
>+    overflow: scroll;
>+}
>+
>+#console, #description {
>+    width: 100%;
>+}
>+</style>
>+<script>
>+jsTestIsAsync = true;
>+
>+function rectsAreEqual(rects, otherRects)
>+{
>+    if (rects.length != otherRects.length)
>+        return false;
>+
>+    for (let i = 0; i < rects.length; ++i) {
>+        if (rects[i].top != otherRects[i].top
>+            || rects[i].left != otherRects[i].left
>+            || rects[i].width != otherRects[i].width
>+            || rects[i].height != otherRects[i].height)
>+            return false;
>+    }
>+
>+    return true;
>+}
>+
>+async function waitForSelectionRectsToChange(fromRects)
>+{
>+    let rects = fromRects;
>+    while (rectsAreEqual(rects, fromRects))
>+        rects = await UIHelper.getUISelectionViewRects();
>+    return rects;
>+}
>+
>+addEventListener("load", async function() {
>+    description("This test verifies that a selection in a fast scrollable area is kept up to date after scrolling. To test manually, tap the button and scroll the editable area down; the selection move to account for the new scroll position.");
>+
>+    const editor = document.getElementById("editor");
>+    const button = document.querySelector("button");
>+    button.addEventListener("click", event => {
>+        editor.focus();
>+        getSelection().selectAllChildren(document.getElementById("select-target"));
>+        event.preventDefault();
>+    });
>+
>+    await UIHelper.activateElementAndWaitForInputSession(button);
>+    selectionRectsBefore = await waitForSelectionRectsToChange([]);
>+
>+    await UIHelper.immediateScrollElementAtContentPointToOffset(150, 100, 0, 50);
>+    selectionRectsAfter = await waitForSelectionRectsToChange(selectionRectsBefore);
>+
>+    shouldBe("selectionRectsBefore[0].top", "108");
>+    shouldBe("selectionRectsBefore[0].width", "320");
>+    shouldBe("selectionRectsBefore[0].left", "0");
>+    shouldBe("selectionRectsBefore[0].height", "29");
>+    shouldBe("selectionRectsBefore[1].top", "137");
>+    shouldBe("selectionRectsBefore[1].width", "172");
>+    shouldBe("selectionRectsBefore[1].left", "0");
>+    shouldBe("selectionRectsBefore[1].height", "30");
>+
>+    shouldBe("selectionRectsAfter[0].top", "58");
>+    shouldBe("selectionRectsAfter[0].width", "320");
>+    shouldBe("selectionRectsAfter[0].left", "0");
>+    shouldBe("selectionRectsAfter[0].height", "29");
>+    shouldBe("selectionRectsAfter[1].top", "87");
>+    shouldBe("selectionRectsAfter[1].width", "172");
>+    shouldBe("selectionRectsAfter[1].left", "0");
>+    shouldBe("selectionRectsAfter[1].height", "30");
>+
>+    editor.remove();
>+    button.remove();
>+    finishJSTest();
>+});
>+</script>
>+</head>
>+<body>
>+<div id="editor" contenteditable>
>+    <p>The quick brown fox jumped over the lazy dog.</p>
>+    <p id="select-target">The quick brown fox jumped over the lazy dog.</p>
>+    <p>The quick brown fox jumped over the lazy dog.</p>
>+</div>
>+<button>Click to select text</button>
>+<div id="description"></div>
>+<div id="console"></div>
>+</body>
>+</html>
>\ No newline at end of file

Actions: View | Formatted Diff | Diff

Attachments on bug 198217: 373198 | 373232 | 373235