Bug 210611 – [iOS] Add a way to focus a text input and place a caret

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

Add IPI to focus a text input context and also place the caret. This functionality will be used by code in WebKitAdditions.

[Radar WebKit Bug Importer]

<rdar://problem/61893062>

[Daniel Bates]

I will remove the existing focusTextInputContext SPI in a follow up patch so I can determine whether I salvage some or all of the single TestWebKitAPI test that covers its functionality.

[Daniel Bates]

Created attachment 396671 [details]
Patch and tests

[Daniel Bates]

Created attachment 396673 [details]
Patch and tests

[Darin Adler]

Comment on attachment 396673 [details]
Patch and tests

View in context: https://bugs.webkit.org/attachment.cgi?id=396673&action=review

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5088
> +    // We ignore bounding rect changes as the bounding rect of the focused element is not kept up-to-date.
> +    return self._hasFocusedElement && context && context._textInputContext.isSameElement(_focusedElementInformation.elementContext);

Why does this method null-check context instead of asserting?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5093
> +    ASSERT(context);

Why does this method assert context instead of checking for null?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4291
> +    RefPtr<Element> target = elementForContext(elementContext);

I think that the modern approach for this would be to have elementForContext return a RefPtr. Lacking that, then I suggest using auto x = makeRefPtr(y) instead of RefPtr<ClassName> x = y so that the RefPtr gets the appropriate type automatically instead of possible narrowing.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4292
> +    if (!target || !target->renderer()) {

Checking renderer for null without first updating layout is almost always a programming mistake.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4298
> +    Ref<Frame> targetFrame = *target->document().frame();

Similarly here, newest style would be:

    auto targetFrame = makeRef(*target->document().frame());

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4305
> +    if (!didFocus || !target->renderer()) {

Checking renderer for null without first updating layout is almost always a programming mistake.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4312
> +    VisiblePosition position = visiblePositionInFocusedNodeForPoint(targetFrame, point, true /* isInteractingWithFocusedElement */);

I suggest auto here.

[Daniel Bates]

Comment on attachment 396673 [details]
Patch and tests

View in context: https://bugs.webkit.org/attachment.cgi?id=396673&action=review

Thanks for the review.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5088
>> +    return self._hasFocusedElement && context && context._textInputContext.isSameElement(_focusedElementInformation.elementContext);
> 
> Why does this method null-check context instead of asserting?

Because I was debating whether to null check or not: half the code null checks, half does not. I'm going to assert.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5093
>> +    ASSERT(context);
> 
> Why does this method assert context instead of checking for null?

See above. I'm going to leave as-is.

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4291
>> +    RefPtr<Element> target = elementForContext(elementContext);
> 
> I think that the modern approach for this would be to have elementForContext return a RefPtr. Lacking that, then I suggest using auto x = makeRefPtr(y) instead of RefPtr<ClassName> x = y so that the RefPtr gets the appropriate type automatically instead of possible narrowing.

OK... fixing up elementForContext.

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4292
>> +    if (!target || !target->renderer()) {
> 
> Checking renderer for null without first updating layout is almost always a programming mistake.

I guess I could update layout then...

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4298
>> +    Ref<Frame> targetFrame = *target->document().frame();
> 
> Similarly here, newest style would be:
> 
>     auto targetFrame = makeRef(*target->document().frame());

OK.

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4305
>> +    if (!didFocus || !target->renderer()) {
> 
> Checking renderer for null without first updating layout is almost always a programming mistake.

I think I'm going to update layout then

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4312
>> +    VisiblePosition position = visiblePositionInFocusedNodeForPoint(targetFrame, point, true /* isInteractingWithFocusedElement */);
> 
> I suggest auto here.

OK

[Daniel Bates]

Created attachment 396687 [details]
To Land

[Daniel Bates]

Comment on attachment 396687 [details]
To Land

Clearing flags on attachment: 396687

Committed r260214: <https://trac.webkit.org/changeset/260214>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.