1109656 - IOS-49 - Search - Auto domain completion

Status: RESOLVED FIXED

(Firefox for iOS :: Browser, defect)

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Let's create a generic auto-completing URL field that can take a completion suggestion from its delegate. If we keep this generic then we can ultimately hook it up to any kind of datasource, including our temporary Bookmarks/History providers we have now.

Comment 1

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: x-github-pullrequest

First iteration of location completion.

Comment 2

[Brian Nicholson (:bnicholson)]

Comment on attachment 8538044 [details] [review]
x-github-pullrequest

Looks OK, though I'd like to see an updated PR with nits fixed and a rebase (since this code is completely bitrotted). Ideally we'd also fix the behavior at [1], but I'd be OK with handling that in a follow-up bug if you prefer.

[1] https://github.com/mozilla/firefox-ios/pull/55#issuecomment-67572155

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 8538044 [details] [review]
x-github-pullrequest

I think this looks pretty good as a first draft (with lots of little follow ups to land after, but I had some comments and ideas. What do you think?

Comment 5

[Jenn Chaulk (:jchaulk)]

Removed the [META] from the name since this rolled in under the Awesome Screen Meta.

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: Pull request

Comment 8

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8587690 [details] [review]
Pull request

This looks pretty good but I noticed two issues:


The first is a blocker:

(Assuming you have news.ycombinator.com in your history)

1) Start typing 'ne'
2) URL is autocompleted to news.ycombinator.com
3) I hit either Go on the onscreen keyboard or just Enter on my bluetooth kbd
4) Yahoo search opens searching for 'ne'

Expected: news.ycombinator.com should open




The second is just a little odd:

Also, one thing that I liked from the original implementation, which is on parity with Safari, is this:

(Assuming you have news.ycombinator.com in your history)

1) Start typing 'ne'
2) See ne[ws.ycombinator.com] appear (last bit is selected without bars)
3) tap anywhere in the field

Expected: the 'completion selection' turns into a text selection with handlebars. Ie, it would look like: ne[bar]ws.combinator.com[bar]

Actual: all text is deselected and the cursor is placed at what seems some random place in the url. In my case the cursor appears a bit before the end while i tapped on the 'news' part of the url.

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 8587690 [details] [review]
Pull request

Updated with review comments addressed.

Comment 10

[Brian Nicholson (:bnicholson)]

(In reply to Stefan Arentz [:st3fan] from comment #8)

Oops, I didn't notice you commented here, so I flipped the flag to wesj since I spent awhile addressing his comments in the PR. Feel free to review the updated version if you want!

> The first is a blocker:
> ...

Hm, thanks. I'll take a look.

> Also, one thing that I liked from the original implementation, which is on
> parity with Safari, is this:
> ...

Yeah, I noticed that in the Safari too, but I wanted to make it so that tapping in the field would end the selection so you could immediately type the remaining characters. Maybe it's just because I'm used to Android, but what's the benefit of selecting the completion portion? It just seemed like an annoyance to me, requiring two taps to get back into editing mode.

But I definitely didn't intend for the cursor to go to "random place in the url"; in my testing, it went to the end. That's strange that it was a bit before the end for you...guess I'll have to play around with it some more.

Comment 11

[Brian Nicholson (:bnicholson)]

Comment on attachment 8587690 [details] [review]
Pull request

Stefan, I updated the PR with your issues addressed.

Comment 12

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I can't get this going without immediately crashing. This is what I did:

1) Start clean simulator. iPhone 5S
2) Tap on location bar
3) Type www.reddit.com
4) Hit Go

Stack trace below:

* thread #1: tid = 0x2e66989, 0x000000010ab4fcd8 Shared`Shared.KeyboardHelper.SELkeyboardWillHide (notification=0x00007fd208db5410, self=0x00007fd208c40630)(ObjectiveC.NSNotification) -> () + 1928 at KeyboardHelper.swift:84, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
  * frame #0: 0x000000010ab4fcd8 Shared`Shared.KeyboardHelper.SELkeyboardWillHide (notification=0x00007fd208db5410, self=0x00007fd208c40630)(ObjectiveC.NSNotification) -> () + 1928 at KeyboardHelper.swift:84
    frame #1: 0x000000010ab4fd5a Shared`@objc Shared.KeyboardHelper.SELkeyboardWillHide (Shared.KeyboardHelper)(ObjectiveC.NSNotification) -> () + 58 at KeyboardHelper.swift:0
    frame #2: 0x000000010bf73cec CoreFoundation`__CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 12
    frame #3: 0x000000010be738a4 CoreFoundation`_CFXNotificationPost + 2484
    frame #4: 0x000000010b3906b8 Foundation`-[NSNotificationCenter postNotificationName:object:userInfo:] + 66
    frame #5: 0x000000010d059c33 UIKit`-[UIInputWindowController postStartNotifications:withInfo:] + 340
    frame #6: 0x000000010d05b342 UIKit`__77-[UIInputWindowController moveFromPlacement:toPlacement:starting:completion:]_block_invoke556 + 375
    frame #7: 0x000000010c9fb362 UIKit`+[UIView(UIViewAnimationWithBlocks) _setupAnimationWithDuration:delay:view:options:factory:animations:start:animationStateGenerator:completion:] + 473
    frame #8: 0x000000010c9fb578 UIKit`+[UIView(UIViewAnimationWithBlocks) _animateWithDuration:delay:options:animations:start:completion:] + 53
    frame #9: 0x000000010d05b04d UIKit`-[UIInputWindowController moveFromPlacement:toPlacement:starting:completion:] + 972
    frame #10: 0x000000010d05edc2 UIKit`-[UIInputWindowController setInputViewSet:] + 599
    frame #11: 0x000000010d05aa7a UIKit`-[UIInputWindowController performOperations:withAnimationStyle:] + 50
    frame #12: 0x000000010ce4abce UIKit`-[UIPeripheralHost(UIKitInternal) setInputViews:animationStyle:] + 1054
    frame #13: 0x000000010cb003f1 UIKit`-[UIResponder _finishResignFirstResponder] + 141
    frame #14: 0x000000010cb004f5 UIKit`-[UIResponder resignFirstResponder] + 236
    frame #15: 0x000000010d0bd2d4 UIKit`-[UITextField resignFirstResponder] + 114
    frame #16: 0x000000010a62e742 Client`Client.URLBarView.finishEditing (self=0x00007fd20a019900)() -> () + 274 at URLBarView.swift:343
    frame #17: 0x000000010a7465f3 Client`Client.BrowserViewController.(url=0x00007fd208d6f7f0, self=0x00007fd208d304f0) (Client.BrowserViewController)(ObjectiveC.NSURL) -> () + 707 at BrowserViewController.swift:257
    frame #18: 0x000000010a74d0ce Client`Client.BrowserViewController.urlBar (urlBar=0x00007fd20a019900, text=Swift.String at 0x00007fff555fdd78, self=0x00007fd208d304f0)(Client.URLBarView, didSubmitText : Swift.String) -> () + 1262 at BrowserViewController.swift:386
    frame #19: 0x000000010a74d167 Client`@objc Client.BrowserViewController.urlBar (Client.BrowserViewController)(Client.URLBarView, didSubmitText : Swift.String) -> () + 87 at BrowserViewController.swift:0
    frame #20: 0x000000010a77b908 Client`dynamic Client.BrowserViewController.urlBar (urlBar=0x00007fd20a019900, text=Swift.String at 0x00007fff555fde30, self=0x00007fd208d304f0)(Client.URLBarView, didSubmitText : Swift.String) -> () + 88 at BrowserViewController.swift:372
    frame #21: 0x000000010a74d6a1 Client`protocol witness for Client.URLBarDelegate.urlBar <A : Client.URLBarDelegate>(Client.URLBarDelegate.Self)(Client.URLBarView, didSubmitText : Swift.String) -> () in conformance Client.BrowserViewController : Client.URLBarDelegate + 17 at BrowserViewController.swift:372
    frame #22: 0x000000010a62d961 Client`Client.URLBarView.autocompleteTextFieldShouldReturn (autocompleteTextField=0x00007fd208d4b640, self=0x00007fd20a019900)(Client.AutocompleteTextField) -> Swift.Bool + 1457 at URLBarView.swift:309
    frame #23: 0x000000010a633a85 Client`protocol witness for Client.AutocompleteTextFieldDelegate.autocompleteTextFieldShouldReturn <A : Client.AutocompleteTextFieldDelegate>(Client.AutocompleteTextFieldDelegate.Self)(Client.AutocompleteTextField) -> Swift.Bool in conformance Client.URLBarView : Client.AutocompleteTextFieldDelegate + 37 at URLBarView.swift:308
    frame #24: 0x000000010a720a77 Client`Client.AutocompleteTextField.textFieldShouldReturn (textField=0x00007fd208d4b640, self=0x00007fd208d4b640)(ObjectiveC.UITextField) -> Swift.Bool + 295 at AutocompleteTextField.swift:134
    frame #25: 0x000000010a720b4a Client`@objc Client.AutocompleteTextField.textFieldShouldReturn (Client.AutocompleteTextField)(ObjectiveC.UITextField) -> Swift.Bool + 58 at AutocompleteTextField.swift:0
    frame #26: 0x000000010d0c5aee UIKit`-[UITextField keyboardInput:shouldInsertText:isMarkedText:] + 128
    frame #27: 0x000000010cb764ba UIKit`-[UIKeyboardImpl callShouldInsertText:] + 109
    frame #28: 0x000000010cb81a73 UIKit`-[UIKeyboardImpl addWordTerminator:afterSpace:elapsedTime:executionContext:] + 72
    frame #29: 0x000000010d0e8469 UIKit`-[UIKeyboardTaskExecutionContext returnExecutionToParentWithInfo:] + 264
    frame #30: 0x000000010cb80d5a UIKit`-[UIKeyboardImpl acceptAutocorrectionForWordTerminator:executionContextPassingTIKeyboardCandidate:] + 257
    frame #31: 0x000000010cb8066b UIKit`-[UIKeyboardImpl addInputEvent:executionContext:] + 1312
    frame #32: 0x000000010cb800c9 UIKit`__60-[UIKeyboardImpl addInputString:withFlags:executionContext:]_block_invoke + 36
    frame #33: 0x000000010d0e8469 UIKit`-[UIKeyboardTaskExecutionContext returnExecutionToParentWithInfo:] + 264
    frame #34: 0x000000010cb84393 UIKit`-[UIKeyboardImpl acceptCurrentCandidateIfSelectedWithExecutionContext:] + 224
    frame #35: 0x000000010cb80096 UIKit`-[UIKeyboardImpl addInputString:withFlags:executionContext:] + 507
    frame #36: 0x000000010cb8dc8f UIKit`-[UIKeyboardImpl handleKeyWithString:forKeyEvent:executionContext:] + 628
    frame #37: 0x000000010cb8d8a0 UIKit`-[UIKeyboardImpl handleKeyEvent:executionContext:] + 1573
    frame #38: 0x000000010d0e8914 UIKit`-[UIKeyboardTaskQueue continueExecutionOnMainThread] + 332
    frame #39: 0x000000010cb8d1db UIKit`-[UIKeyboardImpl handleKeyEvent:] + 216
    frame #40: 0x000000010c9a7046 UIKit`_UIApplicationHandleEventFromQueueEvent + 2467
    frame #41: 0x000000010c986103 UIKit`_UIApplicationHandleEventQueue + 1961
    frame #42: 0x000000010bed9551 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
    frame #43: 0x000000010becf41d CoreFoundation`__CFRunLoopDoSources0 + 269
    frame #44: 0x000000010becea54 CoreFoundation`__CFRunLoopRun + 868
    frame #45: 0x000000010bece486 CoreFoundation`CFRunLoopRunSpecific + 470
    frame #46: 0x00000001101f59f0 GraphicsServices`GSEventRunModal + 161
    frame #47: 0x000000010c989420 UIKit`UIApplicationMain + 1282
    frame #48: 0x000000010a6c6b1e Client`top_level_code + 78 at AppDelegate.swift:11
    frame #49: 0x000000010a6c6b5a Client`main + 42 at AppDelegate.swift:0
    frame #50: 0x000000010ea67145 libdyld.dylib`start + 1

Comment 13

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

In another simulator where I have history, the domain completes correctly but I still get the same crash when I hit Go. (Or enter when using an external keyboard)

Comment 14

[Brian Nicholson (:bnicholson)]

(In reply to Stefan Arentz [:st3fan] from comment #12)
> I can't get this going without immediately crashing. This is what I did:

Looks like this was a result of pulling WeakList out of KeyboardHelper. This looks like yet another Swift generics bug, so I reverted the changes to KeyboardHelper and left that class alone. Things should be working now.

Comment 15

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8587690 [details] [review]
Pull request

Brian sorry for the delay. This looks great! No crashers. No comments on the code.