Add IPI to focus a text input context and also place the caret. This functionality will be used by code in WebKitAdditions.
<rdar://problem/61893062>
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.
Created attachment 396671 [details] Patch and tests
Created attachment 396673 [details] Patch and tests
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.
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
Created attachment 396687 [details] To Land
Comment on attachment 396687 [details] To Land Clearing flags on attachment: 396687 Committed r260214: <https://trac.webkit.org/changeset/260214>
All reviewed patches have been landed. Closing bug.