Title: [290834] trunk
- Revision
- 290834
- Author
- [email protected]
- Date
- 2022-03-04 09:16:56 -0800 (Fri, 04 Mar 2022)
Log Message
[iOS] Unable to scroll to a found text range when there is an existing selection
https://bugs.webkit.org/show_bug.cgi?id=237407
rdar://89653213
Reviewed by Wenson Hsieh.
Source/WebCore:
Scrolling to a found text range is performed by creating a
`TemporarySelectionChange` and using the `RevealSelectionBounds`
`TemporarySelectionOption`. When a `TemporarySelectionChange` is
destroyed, the original selection is restored.
Currently, the selection is restored using the same set of selection
options used to make the temporary selection. Consequently, whenever a
"reveal" option is specified, WebKit scrolls to reveal the temporary
selection, and then scrolls again to reveal the original selection.
This behavior means that an attempt to scroll to a found text range
will fail if the document has an existing selection.
To fix, do not add any of the "reveal" options to the set of selection
options when restoring the original selection during a
`TemporarySelectionChange`. The only other features that use
`TemporarySelectionChange` with "reveal" options are App Highlights
and Scroll To Text Fragment. Neither of these features require WebKit
to scroll to the original selection.
* editing/Editor.cpp:
(WebCore::TemporarySelectionChange::TemporarySelectionChange):
(WebCore::TemporarySelectionChange::~TemporarySelectionChange):
(WebCore::TemporarySelectionChange::setSelection):
* editing/Editor.h:
Tools:
Add an API test that sets a selection in the document, finds some
text outside the viewport, and scrolls to make the found text
visible.
* TestWebKitAPI/Tests/WebKitCocoa/FindInPage.mm:
(-[TestScrollViewDelegate init]):
(-[TestScrollViewDelegate scrollViewDidEndScrollingAnimation:]):
(TEST):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (290833 => 290834)
--- trunk/Source/WebCore/ChangeLog 2022-03-04 16:51:37 UTC (rev 290833)
+++ trunk/Source/WebCore/ChangeLog 2022-03-04 17:16:56 UTC (rev 290834)
@@ -1,3 +1,36 @@
+2022-03-04 Aditya Keerthi <[email protected]>
+
+ [iOS] Unable to scroll to a found text range when there is an existing selection
+ https://bugs.webkit.org/show_bug.cgi?id=237407
+ rdar://89653213
+
+ Reviewed by Wenson Hsieh.
+
+ Scrolling to a found text range is performed by creating a
+ `TemporarySelectionChange` and using the `RevealSelectionBounds`
+ `TemporarySelectionOption`. When a `TemporarySelectionChange` is
+ destroyed, the original selection is restored.
+
+ Currently, the selection is restored using the same set of selection
+ options used to make the temporary selection. Consequently, whenever a
+ "reveal" option is specified, WebKit scrolls to reveal the temporary
+ selection, and then scrolls again to reveal the original selection.
+ This behavior means that an attempt to scroll to a found text range
+ will fail if the document has an existing selection.
+
+ To fix, do not add any of the "reveal" options to the set of selection
+ options when restoring the original selection during a
+ `TemporarySelectionChange`. The only other features that use
+ `TemporarySelectionChange` with "reveal" options are App Highlights
+ and Scroll To Text Fragment. Neither of these features require WebKit
+ to scroll to the original selection.
+
+ * editing/Editor.cpp:
+ (WebCore::TemporarySelectionChange::TemporarySelectionChange):
+ (WebCore::TemporarySelectionChange::~TemporarySelectionChange):
+ (WebCore::TemporarySelectionChange::setSelection):
+ * editing/Editor.h:
+
2022-03-04 Tyler Wilcock <[email protected]>
AX ITM: Updating m_pendingLoadingProgress can cause deadlock on AXIsolatedTree::m_changeLogLock
Modified: trunk/Source/WebCore/editing/Editor.cpp (290833 => 290834)
--- trunk/Source/WebCore/editing/Editor.cpp 2022-03-04 16:51:37 UTC (rev 290833)
+++ trunk/Source/WebCore/editing/Editor.cpp 2022-03-04 17:16:56 UTC (rev 290834)
@@ -230,7 +230,7 @@
if (temporarySelection) {
m_selectionToRestore = document.selection().selection();
- setSelection(temporarySelection.value());
+ setSelection(temporarySelection.value(), IsTemporarySelection::Yes);
}
}
@@ -237,7 +237,7 @@
TemporarySelectionChange::~TemporarySelectionChange()
{
if (m_selectionToRestore)
- setSelection(m_selectionToRestore.value());
+ setSelection(m_selectionToRestore.value(), IsTemporarySelection::No);
if (m_options & TemporarySelectionOption::IgnoreSelectionChanges) {
auto revealSelection = m_options & TemporarySelectionOption::RevealSelection ? Editor::RevealSelection::Yes : Editor::RevealSelection::No;
@@ -250,19 +250,23 @@
#endif
}
-void TemporarySelectionChange::setSelection(const VisibleSelection& selection)
+void TemporarySelectionChange::setSelection(const VisibleSelection& selection, IsTemporarySelection isTemporarySelection)
{
auto options = FrameSelection::defaultSetSelectionOptions();
if (m_options & TemporarySelectionOption::DoNotSetFocus)
options.add(FrameSelection::DoNotSetFocus);
- if (m_options & TemporarySelectionOption::RevealSelection)
- options.add(FrameSelection::RevealSelection);
- if (m_options & TemporarySelectionOption::DelegateMainFrameScroll)
- options.add(FrameSelection::DelegateMainFrameScroll);
- if (m_options & TemporarySelectionOption::SmoothScroll)
- options.add(FrameSelection::SmoothScroll);
- if (m_options & TemporarySelectionOption::RevealSelectionBounds)
- options.add(FrameSelection::RevealSelectionBounds);
+
+ if (isTemporarySelection == IsTemporarySelection::Yes) {
+ if (m_options & TemporarySelectionOption::RevealSelection)
+ options.add(FrameSelection::RevealSelection);
+ if (m_options & TemporarySelectionOption::DelegateMainFrameScroll)
+ options.add(FrameSelection::DelegateMainFrameScroll);
+ if (m_options & TemporarySelectionOption::SmoothScroll)
+ options.add(FrameSelection::SmoothScroll);
+ if (m_options & TemporarySelectionOption::RevealSelectionBounds)
+ options.add(FrameSelection::RevealSelectionBounds);
+ }
+
m_document->selection().setSelection(selection, options);
}
Modified: trunk/Source/WebCore/editing/Editor.h (290833 => 290834)
--- trunk/Source/WebCore/editing/Editor.h 2022-03-04 16:51:37 UTC (rev 290833)
+++ trunk/Source/WebCore/editing/Editor.h 2022-03-04 17:16:56 UTC (rev 290834)
@@ -133,8 +133,10 @@
WEBCORE_EXPORT ~TemporarySelectionChange();
private:
- void setSelection(const VisibleSelection&);
+ enum class IsTemporarySelection { No, Yes };
+ void setSelection(const VisibleSelection&, IsTemporarySelection);
+
RefPtr<Document> m_document;
OptionSet<TemporarySelectionOption> m_options;
bool m_wasIgnoringSelectionChanges;
Modified: trunk/Tools/ChangeLog (290833 => 290834)
--- trunk/Tools/ChangeLog 2022-03-04 16:51:37 UTC (rev 290833)
+++ trunk/Tools/ChangeLog 2022-03-04 17:16:56 UTC (rev 290834)
@@ -1,3 +1,20 @@
+2022-03-04 Aditya Keerthi <[email protected]>
+
+ [iOS] Unable to scroll to a found text range when there is an existing selection
+ https://bugs.webkit.org/show_bug.cgi?id=237407
+ rdar://89653213
+
+ Reviewed by Wenson Hsieh.
+
+ Add an API test that sets a selection in the document, finds some
+ text outside the viewport, and scrolls to make the found text
+ visible.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/FindInPage.mm:
+ (-[TestScrollViewDelegate init]):
+ (-[TestScrollViewDelegate scrollViewDidEndScrollingAnimation:]):
+ (TEST):
+
2022-03-04 Philippe Normand <[email protected]>
[Flatpak SDK] Upgrade from llvm12 to llvm13
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/FindInPage.mm (290833 => 290834)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/FindInPage.mm 2022-03-04 16:51:37 UTC (rev 290833)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/FindInPage.mm 2022-03-04 17:16:56 UTC (rev 290834)
@@ -329,6 +329,30 @@
#if HAVE(UIFINDINTERACTION)
+@interface TestScrollViewDelegate : NSObject<UIScrollViewDelegate> {
+ @public bool _finishedScrolling;
+}
+@end
+
+@implementation TestScrollViewDelegate
+
+- (instancetype)init
+{
+ if (!(self = [super init]))
+ return nil;
+
+ _finishedScrolling = false;
+
+ return self;
+}
+
+- (void)scrollViewDidEndScrollingAnimation:(UIScrollView *)scrollView
+{
+ _finishedScrolling = true;
+}
+
+@end
+
@interface TestTextSearchOptions : NSObject
@property (nonatomic) _UITextSearchMatchMethod wordMatchMethod;
@property (nonatomic) NSStringCompareOptions stringCompareOptions;
@@ -561,4 +585,20 @@
TestWebKitAPI::Util::run(&done);
}
+TEST(WebKit, ScrollToFoundRangeWithExistingSelection)
+{
+ auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 200, 200)]);
+ [webView synchronouslyLoadHTMLString:@"<meta name='viewport' content='width=device-width,initial-scale=1'><div contenteditable><p>Top</p><p style='margin-top: 800px'>Bottom</p></div>"];
+ [webView objectByEvaluatingJavaScript:@"let p = document.querySelector('p'); document.getSelection().setBaseAndExtent(p, 0, p, 1)"];
+
+ auto scrollViewDelegate = adoptNS([[TestScrollViewDelegate alloc] init]);
+ [webView scrollView].delegate = scrollViewDelegate.get();
+
+ auto ranges = textRangesForQueryString(webView.get(), @"Bottom");
+ [webView scrollRangeToVisible:[ranges firstObject] inDocument:nil];
+
+ TestWebKitAPI::Util::run(&scrollViewDelegate->_finishedScrolling);
+ EXPECT_TRUE(CGPointEqualToPoint([webView scrollView].contentOffset, CGPointMake(0, 664)));
+}
+
#endif // HAVE(UIFINDINTERACTION)
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
