Summary: | [iOS] Add a way to focus a text input and place a caret | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||||
Component: | WebKit Misc. | Assignee: | Daniel Bates <dbates> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | darin, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Local Build | ||||||||||
Hardware: | iPhone / iPad | ||||||||||
OS: | iOS 13 | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 211276 | ||||||||||
Attachments: |
|
Description
Daniel Bates
2020-04-16 10:46:59 PDT
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. |