Bug 1433475 — Keyboard — enable up and down arrows in location bar. #3706

jhugman · 2018-03-05T11:51:12Z

This PR allows the user to select URL completions (from bookmarks and history) to be selected via the keyboard.

Desktop leaves the cursor in the location field, which means than horizontal cursor movement is possible.

We aren't able to do this, given that the search suggestions for the query appear horizontally.

Please file follow up usability bugs.

r? for @farhan.

https://bugzilla.mozilla.org/show_bug.cgi?id=1433475

mozillamobilebot · 2018-03-05T11:58:44Z

SwiftLint found issues

Warnings

File	Line	Reason
BrowserViewController.swift	842	Force casts should be avoided.
SearchViewController.swift	608	Force casts should be avoided.
SearchViewController.swift	634	Force casts should be avoided.
SearchViewController.swift	455	if,for,while,do statements shouldn't wrap their conditionals in parentheses.
SearchViewController.swift	426	Explicitly calling .init() should be avoided.

Generated by 🚫 Danger

jhugman · 2018-03-05T11:53:35Z

Client/Frontend/Browser/BrowserViewController.swift

@@ -1753,6 +1753,7 @@ extension BrowserViewController: SearchViewControllerDelegate {
    }

    func searchViewControllerDidFinishHighlighting(_ searchViewController: SearchViewController) {
+        searchViewController.resignFirstResponder()
    }


This is called when the user has typed DOWN, then UP — i.e. focus should move back to the location bar.

jhugman · 2018-03-05T11:56:47Z

Client/Frontend/Browser/SearchViewController.swift

+    }
+
+    func handleKeyCommands(sender: UIKeyCommand) {
+        let initialSection = SearchListSection.bookmarksAndHistory.rawValue


We might change this value when we get we get to Bug 1433476.

jhugman · 2018-03-05T11:59:00Z

Client/Frontend/Browser/SearchViewController.swift

+            return
+        }
+
+        tableView(tableView, didSelectRowAtIndexPath: current)


Calling this delegate here feels weird. How do we perform the same action a tap performs?

jhugman · 2018-03-05T11:59:42Z

Client/Frontend/Browser/SearchViewController.swift

+            }
+        }
+        let next = IndexPath(item: nextItem, section: nextSection)
+        self.tableView(tableView, didHighlightRowAt: next)


Calling this delegate here feels weird. tableView.selectRow does the highlighting.

jhugman · 2018-03-05T12:01:30Z

Client/Frontend/Browser/BrowserViewController.swift

+    }
+
+    func searchViewControllerDidFinishHighlighting(_ searchViewController: SearchViewController) {
+        searchViewController.resignFirstResponder()


This is called when the user has typed DOWN, then UP — i.e. focus should move back to the location bar.

Also tried: explicitly setting BVC or the location bar as first responder.

farhanpatel · 2018-03-06T20:19:24Z

I think to simplify this. It might be easier to never change the focus from the AutocompleteTextField. Instead just pass the up/down keypress from the textfield down to the SearchViewController. That way you dont have to worry about show/hiding the keyboard.

Where this gets tricky is handling the search suggestions. Because they are shown horizontally, tapping left/right to switch between them wouldnt work because on the textfield that would move your cursor. I think this is why safari/chrome/firefox desktop show autocomplete suggestions as a list instead of the way we do.

jhugman · 2018-03-06T21:24:46Z

It might be easier to never change the focus from the AutocompleteTextField.
Because they are shown horizontally, tapping left/right to switch between them wouldnt work because on the textfield that would move your cursor.

Yeah, I think you've answered your own question about why we can't do this.

That way you dont have to worry about show/hiding the keyboard.

Ug. I wasn't considering the soft keyboard for UIKeyCodes.

jhugman · 2018-03-07T12:38:30Z

Ok, I think I've cracked it.

Looking at desktop, they seem to be leaving the cursor with the text view, and you can move it around with the cursor.

I'm not altogether happy with what we've got here — but we're limited by the design of the search auto-complete. I'm leaving the horizontal keys for the next follow up bug.

farhanpatel · 2018-03-08T23:36:10Z

I'm getting a pretty bad crash that happens when there are no urls in the searchsuggestions tableview.

Type until there are no url suggestions
press up/down on the keyboard
crashes 😢

I think in the optimal case I dont think we should move focus away from the keyboard. I think this should work exactly like Desktop. For example. if you want to go to reddit.com/r/ios and you already have reddit.com in your history.
you should be able to type re, arrow down to reddit.com and then press right array to get to the end of the url where you can continue typing /r/ios

right now. if you want to continue typing for any reason you have to arrow up all the way to the urlbar.

This doesn't do things in the right way however: the search controller should be providing the shortcuts, not the BVC.

Hightlighted URL is shown in URL bar.

…field.

Previously the autocomplete delegate's `didEnterText` was being called explicitly was being called both explicitly and implicitly (from `textDidChange`). This allows us to set the location updating the search suggestions or history/bookmarks.

farhanpatel

Besides that one unused var. looks awesome! Can't wait for this to land.

farhanpatel · 2018-03-12T22:20:46Z

Client/Frontend/Browser/SearchViewController.swift

@@ -69,6 +71,10 @@ class SearchViewController: SiteTableViewController, KeyboardHelperDelegate, Loa

    static var userAgent: String?

+    var isCellHighlighted: Bool {


I dont think this is used anywhere.

…3706) r=farhan This bug allows the user to select URL completions (from bookmarks and history) to be selected via the keyboard. Desktop leaves the cursor in the location field, which means than horizontal cursor movement is possible. We try to replicate that. There do seem to be some difficulty supporting search suggestions, so this is rolled into another bug.

jhugman commented Mar 5, 2018

View reviewed changes

jhugman force-pushed the jhugman/Bug1433475—keyboard—up-and-down-in-urlbar branch from 55112c0 to 2271022 Compare March 7, 2018 12:35

jhugman added 12 commits March 12, 2018 16:55

Bug 1433475 — Move selection up and down with arrow keys

c4747c2

This doesn't do things in the right way however: the search controller should be providing the shortcuts, not the BVC.

Bug 1433475 — Move detection of arrows to the searchViewController.

e6b8836

Bug 1433475 — Enter sends tab to correct the URL

b30d2d5

Hightlighted URL is shown in URL bar.

Bug 1433475 — What to do when you go back to the location bar?

8ea7f8c

Bug 1433475 — Keep arrow commands for text editing.

2d8f816

Bug 1433475 — Simulate tap on the location bar to return to the text …

42db2ed

…field.

Bug 1433475

55cd3f0

Bug 1433475 — Code cleanup.

75d5aa2

Bug 1439622 — location bar is always firstResponder.

6ce927a

Bug 1433475 — Tidy

0bda6cf

Bug 1433475 — Fixup farhan's crash.

d56f583

jhugman force-pushed the jhugman/Bug1433475—keyboard—up-and-down-in-urlbar branch from 2271022 to d56f583 Compare March 12, 2018 19:45

farhanpatel approved these changes Mar 12, 2018

View reviewed changes

Bug 1433475 — Review feedback: Remove redundant code.

ac14527

jhugman merged commit 4fa18a3 into master Mar 13, 2018

jhugman deleted the jhugman/Bug1433475—keyboard—up-and-down-in-urlbar branch March 13, 2018 12:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1433475 — Keyboard — enable up and down arrows in location bar. #3706

Bug 1433475 — Keyboard — enable up and down arrows in location bar. #3706

jhugman commented Mar 5, 2018 •

edited

mozillamobilebot commented Mar 5, 2018 •

edited

jhugman Mar 5, 2018

jhugman Mar 5, 2018

jhugman Mar 5, 2018

jhugman Mar 5, 2018

jhugman Mar 5, 2018

farhanpatel commented Mar 6, 2018

jhugman commented Mar 6, 2018 •

edited

jhugman commented Mar 7, 2018

farhanpatel commented Mar 8, 2018 •

edited

farhanpatel left a comment

farhanpatel Mar 12, 2018

		@@ -69,6 +71,10 @@ class SearchViewController: SiteTableViewController, KeyboardHelperDelegate, Loa

		static var userAgent: String?

		var isCellHighlighted: Bool {

Bug 1433475 — Keyboard — enable up and down arrows in location bar. #3706

Bug 1433475 — Keyboard — enable up and down arrows in location bar. #3706

Conversation

jhugman commented Mar 5, 2018 • edited

mozillamobilebot commented Mar 5, 2018 • edited

SwiftLint found issues

Warnings

jhugman Mar 5, 2018

Choose a reason for hiding this comment

jhugman Mar 5, 2018

Choose a reason for hiding this comment

jhugman Mar 5, 2018

Choose a reason for hiding this comment

jhugman Mar 5, 2018

Choose a reason for hiding this comment

jhugman Mar 5, 2018

Choose a reason for hiding this comment

farhanpatel commented Mar 6, 2018

jhugman commented Mar 6, 2018 • edited

jhugman commented Mar 7, 2018

farhanpatel commented Mar 8, 2018 • edited

farhanpatel left a comment

Choose a reason for hiding this comment

farhanpatel Mar 12, 2018

Choose a reason for hiding this comment

jhugman commented Mar 5, 2018 •

edited

mozillamobilebot commented Mar 5, 2018 •

edited

jhugman commented Mar 6, 2018 •

edited

farhanpatel commented Mar 8, 2018 •

edited