Attachment 373232 Details for Bug 198217 – Approach #2

[patch] Approach #2

bug-198217-20190701073625.patch (text/plain), 11.54 KB, created by Wenson Hsieh on 2019-07-01 07:36:26 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Wenson Hsieh

Created: 2019-07-01 07:36:26 PDT

Size: 11.54 KB

patch

obsolete

>Subversion Revision: 246947
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 5e5bacdb18fb9788f8cfb0c2a6a97f5efd1a8c54..5b45f3179091581241118edd0c4e3ddb95b07253 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,29 @@
>+2019-06-30  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.
>+
>+        Instead, we can fix this by having async scrolling schedule editor state updates, but only when the selection is
>+        either a range, or a caret in editable content (in which case up-to-date selection information is needed).
>+
>+        Test: editing/selection/ios/update-selection-after-overflow-scroll.html
>+
>+        * editing/VisibleSelection.h:
>+        * page/scrolling/AsyncScrollingCoordinator.cpp:
>+        (WebCore::AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll):
>+
> 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..eea32bfb8cfbfa8428ac3286cbf2d6c67019eef1 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,17 @@
>+2019-06-30  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!).
>+
>+        Don't bother scheduling editor state updates during overflow scrolling in the case where we don't have a
>+        selection, or the selection is a caret in non-editable content. See the WebCore ChangeLog for more details.
>+
>+        * WebProcess/WebPage/WebPage.cpp:
>+        (WebKit::WebPage::didChangeOverflowScrollPosition):
>+
> 2019-06-28  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed build fix attempt after r246928.
>diff --git a/Source/WebCore/editing/VisibleSelection.h b/Source/WebCore/editing/VisibleSelection.h
>index a943da8a7772d6cccbfe2ac7d8effa147b1f8c1a..0d342be3768538c7461a92af2554246da4403617 100644
>--- a/Source/WebCore/editing/VisibleSelection.h
>+++ b/Source/WebCore/editing/VisibleSelection.h
>@@ -98,7 +98,7 @@ public:
>     
>     WEBCORE_EXPORT Element* rootEditableElement() const;
>     WEBCORE_EXPORT bool isContentEditable() const;
>-    bool hasEditableStyle() const;
>+    WEBCORE_EXPORT bool hasEditableStyle() const;
>     WEBCORE_EXPORT bool isContentRichlyEditable() const;
>     // Returns a shadow tree node for legacy shadow trees, a child of the
>     // ShadowRoot node for new shadow trees, or 0 for non-shadow trees.
>diff --git a/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp b/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
>index ca68f7630765cbcbe598d05e70ffeaf10237dac2..aa4c93189601741852a55ffdad41c6b057ec2b4b 100644
>--- a/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
>+++ b/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
>@@ -339,7 +339,7 @@ void AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll(ScrollingNo
>         scrollableArea->scrollToOffsetWithoutAnimation(ScrollableArea::scrollOffsetFromPosition(scrollPosition, toFloatSize(scrollableArea->scrollOrigin())));
>         scrollableArea->setCurrentScrollType(previousScrollType);
> 
>-        if (scrollingLayerPositionAction == ScrollingLayerPositionAction::Set)
>+        if (scrollingLayerPositionAction == ScrollingLayerPositionAction::Set || scrollingLayerPositionAction == ScrollingLayerPositionAction::Sync)
>             m_page->editorClient().overflowScrollPositionChanged();
> 
> #if PLATFORM(COCOA)
>diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>index 957fff1ab7238772c97debe76fbe9f34c9910ce8..b03779fd01b7f965ad79f0bfe3c25c55896b8ded 100644
>--- a/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>+++ b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>@@ -5359,6 +5359,13 @@ void WebPage::didChangeContents()
> 
> void WebPage::didChangeOverflowScrollPosition()
> {
>+    auto frame = makeRef(m_page->focusController().focusedOrMainFrame());
>+    if (frame->selection().isNone() || frame->editor().ignoreSelectionChanges())
>+        return;
>+
>+    if (frame->selection().isCaret() && !frame->selection().selection().hasEditableStyle())
>+        return;
>+
>     didChangeSelectionOrOverflowScrollPosition();
> }
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 125350d262ea011a896992ab26f7f9d6ba6c6b2e..721c9627720bf9041c2098ef8b6c3d7adec7ad4f 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2019-06-30  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

Flags:

simon.fraser: review-

Actions: View | Formatted Diff | Diff

Attachments on bug 198217: 373198 | 373232 | 373235