Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Start suggestions after first character not second fixes #3235 #3238

Merged
merged 4 commits into from
Oct 18, 2016
Merged

Start suggestions after first character not second fixes #3235 #3238

merged 4 commits into from
Oct 18, 2016

Conversation

Sh1d0w
Copy link

@Sh1d0w Sh1d0w commented Aug 18, 2016

@bbondy
Copy link
Member

bbondy commented Aug 18, 2016

I think these tests might be breaking with this:

  2) navigationBar typing shortcut-focus-url selects the text:
     Error: Promise was rejected with the following reason: timeout

  3) navigationBar change tabs switch to typing tab preserves typing state:
     Error: Promise was rejected with the following reason: timeout

  4) navigationBar clicking on navbar blurred selects the text:
     Error: Promise was rejected with the following reason: timeout

Please also adjust the urlbar suggestions code to test that it comes up after a single char.
But otherwise it looks good to go.

@bsclifton
Copy link
Member

Search engine shortcuts require at least 2 chars ("a " for amazon, "w " for wikipedia, etc) but this doesn't look like it would break that 😄 Something you might manually test (in addition to checking out the broken tests)

@Sh1d0w
Copy link
Author

Sh1d0w commented Aug 19, 2016

@bbondy @bsclifton I have adjusted the tests to fit the new behaviour. Also added a new test.

Btw there was a test that was failing on master as well, because the element that was tested did not exists. I fixed that as well.

@bbondy
Copy link
Member

bbondy commented Aug 19, 2016

I think these ones are still failing on travis:

  4) navigationBar change tabs switch to typing tab preserves typing state:
     Error: Promise was rejected with the following reason: timeout

  5) navigationBar clicking on navbar blurred selects the text:
     Error: Promise was rejected with the following reason: timeout

It's possible they are intermittent but I haven't seen them before.

@Sh1d0w
Copy link
Author

Sh1d0w commented Aug 19, 2016

@bbondy Locally all tests pass, I guess it is travis problem? I saw it earlier and ran the whole test suite 3 times in a row locally, but could not find any problem.

@Sh1d0w
Copy link
Author

Sh1d0w commented Aug 20, 2016

This is the result when I execute all tests at once on my branch (2 test failing but it is the same on master as well and not related to my changes):

@bbondy
Copy link
Member

bbondy commented Aug 20, 2016

I guess it is travis problem?

Can you see if it came up before this PR in build history there?
Because even if it's only a travis problem, if it is introduced here we can't land because of it until it is fixed.

https://travis-ci.org/brave/browser-laptop

Possibly you'll find evidence it's not related to this though.

@Sh1d0w
Copy link
Author

Sh1d0w commented Oct 13, 2016

@bbondy Can you review the PR please. All of the failing tests are failing on master as well and its because of timeout ... if you execute them locally they won't fail

@bbondy
Copy link
Member

bbondy commented Oct 14, 2016

I think this looks good but I'm going to wait for 0.12.6 to merge. Sorry this had to wait so long.

@bbondy
Copy link
Member

bbondy commented Oct 18, 2016

merging finally, thanks for your patience on this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants