| Summary: | -requestDocumentContext always returns 1 text unit more granularity than requested | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||
| Component: | WebKit Misc. | Assignee: | Daniel Bates <dbates> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | darin, ews-watchlist, mifenton, webkit-bug-importer, wenson_hsieh | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Local Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 196127 | ||||||||
| Attachments: |
|
||||||||
(In reply to Daniel Bates from comment #0) > Consider this unit test that requests 1 sentence of granularity: > > [[ > TEST(WebKit, DocumentEditingContextDebug2) > { > auto webView = adoptNS([[TestWKWebView alloc] > initWithFrame:NSMakeRect(0, 0, 800, 600)]); > [webView synchronouslyLoadHTMLString:applyStyle(@"<p id='text' > contenteditable>The first sentence. The second sentence. The third > sentence.</p>")]; > [webView > stringByEvaluatingJavaScript:@"getSelection().setBaseAndExtent(text. > firstChild, text.firstChild.length, text.firstChild, > text.firstChild.length)"]; // Will focus <p>. > > auto *context = [webView > synchronouslyRequestDocumentContext:makeRequest(UIWKDocumentRequestText, > UITextGranularitySentence, 1)]; > EXPECT_NOT_NULL(context); > EXPECT_NSSTRING_EQ("The second sentence.", context.contextBefore); Typo....should be EXPECT_NSSTRING_EQ("The third sentence.", context.contextBefore); Created attachment 385620 [details]
Patch and unit tests
FYI (mostly for me) the fix for <rdar://problem/6501422> added -atBoundaryOfGranularity: Comment on attachment 385620 [details] Patch and unit tests View in context: https://bugs.webkit.org/attachment.cgi?id=385620&action=review > Source/WebCore/ChangeLog:13 > + Fix up the code to actaully determine if the specified position is at a sentence > + boundary. Currently the code will always return false when asking whether the > + specified position is at a sentence boundary because it compares it to the position > + of the end of the next sentence or the beginning of the current sentence when selecting > + forward or backwards, respecitvely. Why does this same problem not affect the other granularities? > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3880 > static VisiblePosition moveByGranularityRespectingWordBoundary(const VisiblePosition& position, TextGranularity granularity, uint64_t granularityCount, SelectionDirection direction) Strange to have this code in an iOS-specific source file. Does this operation only come up with implementing some iOS-specific feature? Feels to me like a remnant of old times when we had a separate team working on an iOS overlay for WebKit. I’d love to see more of this in core code and tested on all platforms, etc. But I suppose if it’s really a unique requirement of iOS UI then that’s a different story. Comment on attachment 385620 [details] Patch and unit tests View in context: https://bugs.webkit.org/attachment.cgi?id=385620&action=review >> Source/WebCore/ChangeLog:13 >> + forward or backwards, respecitvely. > > Why does this same problem not affect the other granularities? Nit - "respectively". > Source/WebKit/ChangeLog:15 > + Additionally, added assertions to ensure function invariants, including that we expect to me passed Nit - "expect to be" > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3909 > + // Then moving 1 line of granularity forward will return the postion after the 'e' in sentence. Nit - "the position" Thanks for the review. (In reply to Darin Adler from comment #5) > Comment on attachment 385620 [details] > Patch and unit tests > > View in context: > https://bugs.webkit.org/attachment.cgi?id=385620&action=review > > > Source/WebCore/ChangeLog:13 > > + Fix up the code to actaully determine if the specified position is at a sentence > > + boundary. Currently the code will always return false when asking whether the > > + specified position is at a sentence boundary because it compares it to the position > > + of the end of the next sentence or the beginning of the current sentence when selecting > > + forward or backwards, respecitvely. > > Why does this same problem not affect the other granularities? > Because those granularities may yield a position that truncates a word. This function always wants to return a position that respects word boundaries: only whole words. Sentence granularity guarantees this invariant by definition of what a sentence is. So, no need to fix things up. > > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3880 > > static VisiblePosition moveByGranularityRespectingWordBoundary(const VisiblePosition& position, TextGranularity granularity, uint64_t granularityCount, SelectionDirection direction) > > Strange to have this code in an iOS-specific source file. Does this > operation only come up with implementing some iOS-specific feature? > Yes, as of right now. But why it was placed here to begin with 🤷♂️ or agree with. I wouldn't put it here. It is not iOS-specific, but I didn't want to move it in this patch to keep it smaller and make it easier to see the changes I am already making. > Feels to me like a remnant of old times when we had a separate team working > on an iOS overlay for WebKit. Bingo! Well moveByGranularityRespectingWordBoundary() is new code and I didn't write it or place it where it is and I wouldn't have done so. But atBoundaryOfGranularity() is old code from the the old separate team days (2009! see <rdar://problem/6501422>). > I’d love to see more of this in core code and > tested on all platforms, etc. But I suppose if it’s really a unique > requirement of iOS UI then that’s a different story. Only used by iOS at the moment, but useful in general. Thanks for the review. Will fix. (In reply to Wenson Hsieh from comment #6) > Comment on attachment 385620 [details] > Patch and unit tests > > View in context: > https://bugs.webkit.org/attachment.cgi?id=385620&action=review > > >> Source/WebCore/ChangeLog:13 > >> + forward or backwards, respecitvely. > > > > Why does this same problem not affect the other granularities? > > Nit - "respectively". > > > Source/WebKit/ChangeLog:15 > > + Additionally, added assertions to ensure function invariants, including that we expect to me passed > > Nit - "expect to be" > > > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3909 > > + // Then moving 1 line of granularity forward will return the postion after the 'e' in sentence. > > Nit - "the position" Comment on attachment 385620 [details] Patch and unit tests View in context: https://bugs.webkit.org/attachment.cgi?id=385620&action=review >>>>> Source/WebCore/ChangeLog:13 >>>>> + forward or backwards, respecitvely. >>>> >>>> Why does this same problem not affect the other granularities? >>> >>> Nit - "respectively". >> >> Because those granularities may yield a position that truncates a word. This function always wants to return a position that respects word boundaries: only whole words. Sentence granularity guarantees this invariant by definition of what a sentence is. So, no need to fix things up. > > Oops! I misread your question. To answer your question: endOfSentence(), startOfSentence() doesn't take a "LeftWordIfOnBoundary"/"RightWordIfOnBoundary" like argument like endOfWord(), startOfWord() do to know what direction to look. So, this fix is effectively implementing "LeftWordIfOnBoundary"/"RightWordIfOnBoundary"-like functionality but I'm just inlining it in at the one call site. Maybe should be in those functions. 🤷♂️ Comment on attachment 385620 [details] Patch and unit tests View in context: https://bugs.webkit.org/attachment.cgi?id=385620&action=review >>>>>> Source/WebCore/ChangeLog:13 >>>>>> + forward or backwards, respecitvely. >>>>> >>>>> Why does this same problem not affect the other granularities? >>>> >>>> Nit - "respectively". >>> >>> Because those granularities may yield a position that truncates a word. This function always wants to return a position that respects word boundaries: only whole words. Sentence granularity guarantees this invariant by definition of what a sentence is. So, no need to fix things up. >> >> > > Oops! I misread your question. To answer your question: endOfSentence(), startOfSentence() doesn't take a "LeftWordIfOnBoundary"/"RightWordIfOnBoundary" like argument like endOfWord(), startOfWord() do to know what direction to look. So, this fix is effectively implementing "LeftWordIfOnBoundary"/"RightWordIfOnBoundary"-like functionality but I'm just inlining it in at the one call site. Maybe should be in those functions. 🤷♂️ I never tested document granularity. So, maybe there is a bug there. For line granularity it seems to work as-is. Maybe the affinity setting code fixes it. Created attachment 385777 [details]
To land
Committed r253561: <https://trac.webkit.org/changeset/253561> |
Consider this unit test that requests 1 sentence of granularity: [[ TEST(WebKit, DocumentEditingContextDebug2) { auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600)]); [webView synchronouslyLoadHTMLString:applyStyle(@"<p id='text' contenteditable>The first sentence. The second sentence. The third sentence.</p>")]; [webView stringByEvaluatingJavaScript:@"getSelection().setBaseAndExtent(text.firstChild, text.firstChild.length, text.firstChild, text.firstChild.length)"]; // Will focus <p>. auto *context = [webView synchronouslyRequestDocumentContext:makeRequest(UIWKDocumentRequestText, UITextGranularitySentence, 1)]; EXPECT_NOT_NULL(context); EXPECT_NSSTRING_EQ("The second sentence.", context.contextBefore); EXPECT_NULL(context.selectedText); EXPECT_NULL(context.contextAfter); } ]] It fails: [[ TestWebKitAPI.WebKit.DocumentEditingContextDebug2 LEAK: 1 WebProcessPool LEAK: 1 WebPageProxy /.../DocumentEditingContext.mm:174 Expected equality of these values: "The second sentence." (NSString *)context.contextBefore Which is: "The second sentence. The third sentence." ]] ^^^ Two sentences were returned!