UrlBar and suggestion improvements #5046
Conversation
| @@ -204,8 +197,13 @@ class UrlBar extends ImmutableComponent { | |||
| case KeyCodes.BACKSPACE: | |||
| this.hideAutoComplete() | |||
| break | |||
| // Do not trigger rendering of suggestions if you are pressing alt or shift | |||
| case KeyCodes.ALT: | |||
| case KeyCodes.SHIFT: | |||
There was a problem hiding this comment.
shouldn't this also include most other modifier keys?
There was a problem hiding this comment.
I have added two more. If you think I missed some, please let me know I will add :)
| @@ -88,20 +87,5 @@ describe('urlBar', function () { | |||
| .then((val) => val === 'aboutx') | |||
| }) | |||
| }) | |||
|
|
|||
| it('it does not change input after focus until keydown', function * () { | |||
There was a problem hiding this comment.
please change this test to test that focus doesn't bring up any suggestions, thanks
| @@ -281,6 +275,9 @@ class UrlBar extends ImmutableComponent { | |||
| this.clearSearchEngine() | |||
| this.detectSearchEngine(e.target.value) | |||
| this.keyPressed = false | |||
| if (e.target.value.length === 0) { | |||
There was a problem hiding this comment.
just to be paranoid-safe, check that e.target.value exists before calling e.target.value.length
| @@ -38,16 +38,14 @@ class UrlBar extends ImmutableComponent { | |||
| this.onFocus = this.onFocus.bind(this) | |||
| this.onBlur = this.onBlur.bind(this) | |||
| this.onKeyDown = this.onKeyDown.bind(this) | |||
| this.onCut = this.onCut.bind(this) | |||
| this.onKeyUp = this.onKeyUp.bind(this) | |||
| this.onChange = this.onChange.bind(this) | |||
There was a problem hiding this comment.
this is minor, but the s/onChange/onKeyUp change should be kept because it helps readability
|
this seems to have introduced a bug where the URL bar suggestions appear and flicker when the tab is switched. str:
probably because shouldRenderUrlBarSuggestions needs to check that the urlbar is active |
|
@diracdeltas Yes I know I am preparing commit atm. Will let you know when it is ready for review. Thanks |
|
@diracdeltas All fixed now, added tests as well :) |
|
Updated the PR with the fix for #5043 and closed the other PR so all is in one place. |
| .ipcSend('shortcut-focus-url') | ||
| .getValue(urlInput).then((val) => assert(val === 'https://brave.com/')) | ||
| .keys('\uE015') | ||
| it('does not show suggestions on focus', function * () { |
There was a problem hiding this comment.
i think you want to type some letters before focusing, otherwise it wouldn't show suggestions anyway
There was a problem hiding this comment.
Ah, I am so sleepy. Fixed, thanks
Sh1d0w
changed the title
| @@ -89,7 +89,9 @@ describe('urlBar', function () { | |||
| }) | |||
|
|
|||
| it('does not show suggestions on focus', function * () { | |||
| yield this.app.client.ipcSend('shortcut-focus-url') | |||
| yield this.app.client | |||
| .keys('brave') | |||
There was a problem hiding this comment.
just to be sure this actually brings up suggestions, please add
.waitUntil(function () {
return this.isExisting(urlBarSuggestions).then((exists) => exists === true)
here
|
otherwise lgtm once this is rebased, assuming travis mostly passes. thanks! |
|
@diracdeltas rebased, no worries. The Url bar feels so fast and slick now, I've built a custom package for me with it so I can test it :) |
|
@Sh1d0w i think there are some travis test failures related to this change. specifically 14-17 in https://travis-ci.org/brave/browser-laptop/builds/169690847 |
|
@diracdeltas Travis constanlty fails different tests. See for example here https://travis-ci.org/brave/browser-laptop/builds/169702712 which is from this PR (not related to any navbar changes) #5056 If I execute the tests locally everything passes, also if I manually test the failed tests (for example typing the first char) it works just ok. |
|
@Sh1d0w yeah, travis is not reliable yet so i mostly look at tests which seem like they may be related. in this case |
|
actually all those failures from this PR exist in https://travis-ci.org/brave/browser-laptop/builds/169696262 as well except for: so i think that one is probably related, but the others aren't. |
|
@diracdeltas I will look into this and will get back to you with my findings. Thanks |
|
@diracdeltas
|
|
@diracdeltas The bug is fixed this test should pass now. Let me know if you find any other issue. |
|
@diracdeltas @bbondy Because of the changes made in #3049 most of the tests regarding UrlBar Suggestions were failing because now the order of history entries have changed. I fixed those tests in my PR, but please make sure in future when merging stuff that tests pass, because as you can see then we have trouble in the following PR's Thanks guys. |
|
LGTM, thanks! |
git rebase -ito squash commits (if needed).Fixes #5025
FIxes #4941
Fixes #5043
Fixes #4663
cc @bbondy @diracdeltas