Bug 205142 – -requestDocumentContext always returns 1 text unit more granularity than requested

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

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!

[Radar WebKit Bug Importer]

<rdar://problem/57858236>

[Daniel Bates]

(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);

[Daniel Bates]

Created attachment 385620 [details]
Patch and unit tests

[Daniel Bates]

FYI (mostly for me) the fix for <rdar://problem/6501422> added -atBoundaryOfGranularity:

[Darin Adler]

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.

[Wenson Hsieh]

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"

[Daniel Bates]

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.

[Daniel Bates]

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"

[Daniel Bates]

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. 🤷‍♂️

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 385777 [details]
To land

[Daniel Bates]

Committed r253561: <https://trac.webkit.org/changeset/253561>